Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Mismatch in single-precision float encoding between Java and Go RowCoder implementations #22629

Closed
TheNeuralBit opened this issue Aug 8, 2022 · 3 comments · Fixed by #22664 or #22716
Assignees
Labels
bug done & done Issue has been reviewed after it was closed for verification, followups, etc. go P2

Comments

@TheNeuralBit
Copy link
Member

What happened?

Java uses FloatCoder for encoding the FLOAT type, which encodes as a 4 byte single-precision floating point number. Go currently shunts FLOAT through the same path as DOUBLE:

case reflect.Float32, reflect.Float64:
return typeEncoderFieldReflect{encode: func(rv reflect.Value, w io.Writer) error {
return EncodeDouble(rv.Float(), w)
}}, nil

I've currently proposed to make the Python implementation the same as Java: #22626

I think we should do this in Go as well.

Issue Priority

Priority: 2

Issue Component

Component: sdk-go

@TheNeuralBit
Copy link
Member Author

Re-opening this to track getting the standard_coders.yaml test passing

@TheNeuralBit TheNeuralBit reopened this Aug 12, 2022
@TheNeuralBit
Copy link
Member Author

Failure log when re-enabling the #22626 test:

ok      github.com/apache/beam/sdks/v2/go/test/regression       (cached)                                                                                                                                                                                       
2022/08/12 11:28:05 skipping unnested coder spec: {beam:coder:bytes:v1  [] false}                                                                                                                                                                              
2022/08/12 11:28:05 skipping unnested coder spec: {beam:coder:string_utf8:v1  [] false}                              
2022/08/12 11:28:05 skipping unnested coder spec: {beam:coder:kv:v1  [{beam:coder:bytes:v1  [] false} {beam:coder:bytes:v1  [] false}] false}
2022/08/12 11:28:05 skipping unimplemented coder urn: beam:coder:timer:v1                                           
2022/08/12 11:28:05 skipping unimplemented coder urn: beam:coder:param_windowed_value:v1                                                                                                                                                                       
2022/08/12 11:28:05 skipping unimplemented coder urn: beam:coder:param_windowed_value:v1
2022/08/12 11:28:05 skipping coder case. Unsupported in the Go SDK for now: https://github.com/apache/beam/issues/21206: Support encoding position. Payload: 
                                                               
                                                                                                                               
str(
                                                                                                                               
f_boo(

                                                               
i3$30ea5a25-dcd8-4cdb-abeb-5332d15ab4b9 
2022/08/12 11:28:05 skipping coder case. Unsupported in the Go SDK for now: BEAM-9615: Support logical types Payload: 

                                                               
f_timestampp:n                              
#beam:logical_type:micros_instant:v1G2E                     
C
                                                               
seconds                                               
                                                               
                                                               
micros$4d3f6e8f-7412-4ad7-bfd9-b424a1664aef
                                                               
f_string
                                                               
                                                               
f_int$33dafd37-397c-4083-a84e-42177d122221
2022/08/12 11:28:05 skipping unimplemented coder urn: beam:coder:sharded_key:v1                                                                                                                                                                                
2022/08/12 11:28:05 skipping unimplemented test coverage for beam:coder:state_backed_iterable:v1. https://github.com/apache/beam/issues/21324
2022/08/12 11:28:05 skipping unimplemented test coverage for beam:coder:state_backed_iterable:v1. https://github.com/apache/beam/issues/21324
2022/08/12 11:28:05 skipping unimplemented test coverage for beam:coder:state_backed_iterable:v1. https://github.com/apache/beam/issues/21324
2022/08/12 11:28:05 skipping unimplemented test coverage for beam:coder:state_backed_iterable:v1. https://github.com/apache/beam/issues/21324
2022/08/12 11:28:05 skipping unimplemented test coverage for beam:coder:state_backed_iterable:v1. https://github.com/apache/beam/issues/21324
2022/08/12 11:28:05 skipping unimplemented test coverage for beam:coder:state_backed_iterable:v1. https://github.com/apache/beam/issues/21324
2022/08/12 11:28:05 skipping unimplemented coder urn: beam:coder:custom_window:v1
--- FAIL: TestStandardCoders (0.00s)
    --- FAIL: TestStandardCoders/beam:coder:row:v1#06 (0.00s)
        fromyaml_test.go:42: Failed "{beam:coder:row:v1 
             
            f_float$8c97b6c5-69e5-4733-907b-26cd8edae612 [] false}": panicked on coder R;0[struct { F_float float32 "beam:\"f_float\"" }] || {beam:coder:row:v1 
             
            f_float$8c97b6c5-69e5-4733-907b-26cd8edae612 [] false}:
                reflect.StructOf: field 0 has no type :
            goroutine 39 [running]:
            runtime/debug.Stack()
                /usr/local/google/home/bhulette/sdk/go1.18.1/src/runtime/debug/stack.go:24 +0x65
            github.com/apache/beam/sdks/v2/go/test/regression/coders/fromyaml.(*Spec).testStandardCoder.func1()
                /usr/local/google/home/bhulette/working_dir/beam/sdks/go/test/regression/coders/fromyaml/fromyaml.go:124 +0x66
            panic({0x965640, 0xc0003a8560})
                /usr/local/google/home/bhulette/sdk/go1.18.1/src/runtime/panic.go:838 +0x207
            reflect.StructOf({0xc0003b62a0, 0x1, 0xc0000e9770?})
                /usr/local/google/home/bhulette/sdk/go1.18.1/src/reflect/type.go:2457 +0x2899
            github.com/apache/beam/sdks/v2/go/test/regression/coders/fromyaml.diff({{0xc0000372d8, 0x11}, {0xc000380800, 0x35}, {0x0, 0x0, 0x0}, 0x0}, 0xc0003b6230, {{0x965640, ...}, ...})
                /usr/local/google/home/bhulette/working_dir/beam/sdks/go/test/regression/coders/fromyaml/fromyaml.go:317 +0x162c
            github.com/apache/beam/sdks/v2/go/test/regression/coders/fromyaml.(*Spec).testStandardCoder(0xc000375a80)
                /usr/local/google/home/bhulette/working_dir/beam/sdks/go/test/regression/coders/fromyaml/fromyaml.go:148 +0x8ad 
            github.com/apache/beam/sdks/v2/go/test/regression/coders/fromyaml.TestStandardCoders.func1(0xc0003ac1a0)
                /usr/local/google/home/bhulette/working_dir/beam/sdks/go/test/regression/coders/fromyaml/fromyaml_test.go:41 +0x2e
            testing.tRunner(0xc0003ac1a0, 0xc0003a8490)
                /usr/local/google/home/bhulette/sdk/go1.18.1/src/testing/testing.go:1439 +0x102
            created by testing.(*T).Run
                /usr/local/google/home/bhulette/sdk/go1.18.1/src/testing/testing.go:1486 +0x35f
FAIL
FAIL    github.com/apache/beam/sdks/v2/go/test/regression/coders/fromyaml       0.054s
FAIL

> Task :sdks:go:goTest FAILED

@lostluck
Copy link
Contributor

lostluck commented Aug 12, 2022

Ah whoops. The problem is that there are two separate parts for Row Coding: actually encoding from the Go structs (which Jacks PR did) and converting those struct types to the schema proto representation. The latter should be what's missing, under graphx.

@chamikaramj chamikaramj added the done & done Issue has been reviewed after it was closed for verification, followups, etc. label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug done & done Issue has been reviewed after it was closed for verification, followups, etc. go P2
Projects
None yet
4 participants