Repository navigation
[Go SDK] Encode INT16 row fields as big endian to match Java and Python - #40152
Conversation
The reflection based row coder used the varint encoding for int16 and uint16 fields. Java and Python encode the INT16 schema type as 2 big endian bytes, so rows with such fields could not be exchanged with the other SDKs. Fixes apache#40151. Part of apache#39684.
525272f to
15745eb
Compare
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
| case reflect.String: | ||
| return typeDecoderFieldReflect{decode: reflectDecodeString}, nil | ||
| case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: | ||
| case reflect.Int, reflect.Int8, reflect.Int32, reflect.Int64: |
There was a problem hiding this comment.
Int8 (BYTE) likely has similar inconsistency. Consider a same fix here, or could be a follow up
edit: It's fine: BYTE is backed by uint8. See https://github.com/Eliaaazzz/beam/blob/15745eb513417942b6cfe563961edb508804bd8a/sdks/go/pkg/beam/core/runtime/graphx/schema/schema.go#L658
|
Assigning reviewers: R: @shunping for label go. This pull request likely touches a core component ("core" label). Please review with scrutiny. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Reminder, please take a look at this pr: @shunping |
|
@Eliaaazzz please consider iterating on this PR soon, as you have a number of code changes in a chain that we'd like to review and get in! |
|
@jrmccluskey Thanks for the reminder, and sorry for the delay. Yi's CHANGES.md comment is addressed in b9b7d27, so this one is ready for another look. #40153 and #40154 build on it and are ready for review as well. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40152 +/- ##
============================================
+ Coverage 55.09% 56.51% +1.42%
Complexity 1722 1722
============================================
Files 1076 1076
Lines 171812 160948 -10864
Branches 1262 1262
============================================
- Hits 94658 90961 -3697
+ Misses 74880 67693 -7187
- Partials 2274 2294 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #40151. Part of #39684.
The reflection based row coder encoded
int16anduint16struct fields with the varint encoding. Java (BigEndianShortCoder) and Python (BigEndianShortCoder) encode the INT16 schema type as 2 big endian bytes, so rows with such fields could not be exchanged with the other SDKs. Thebeam:logical_type:timestamp:v1cases instandard_coders.yamldepend on this encoding for precisions below 5.Changes:
coder.EncodeInt16,DecodeInt16,EncodeUint16andDecodeUint16.int16anduint16fields.uint16is stored as an INT16 schema field.int16oruint16fields.This is the first PR of a series for #39684 (Go SDK portable logical types).
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.