Repository navigation
[Go SDK] Add portable logical type registration and standard date and micros_instant types - #40154
Conversation
30780dc to
56e380e
Compare
|
Assigning reviewers: R: @jrmccluskey 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: @jrmccluskey |
56e380e to
718ade5
Compare
718ade5 to
db75cf6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40154 +/- ##
=============================================
- Coverage 56.16% 28.76% -27.41%
=============================================
Files 1124 381 -743
Lines 178336 71129 -107207
Branches 1489 0 -1489
=============================================
- Hits 100170 20458 -79712
+ Misses 75645 48822 -26823
+ Partials 2521 1849 -672
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:
|
|
Hey there! I'm starting a review pass, if you would go ahead and rebase this PR now that the two diffbase PRs are merged that would make this a little bit smoother as far as reviewing and merging. Thanks! |
… micros_instant types schema.RegisterLogicalTypeConversion registers a logical type identified by a URN together with the conversions between its Go type and its storage type, and derives the row coder from the storage type. schema.RegisterLogicalTypeCoder does the same for logical types whose wire encoding differs from the row encoding of the storage type. The standard beam:logical_type:date:v1 and beam:logical_type:micros_instant:v1 logical types are registered by default as schema.Date and schema.MicrosInstant. The standard coder regression test now builds the expected row from the schema in the coder payload, decodes payload bytes above 0x7f, counts encoding mismatches as failures, and runs the micros_instant case. Part of apache#39684.
db75cf6 to
2689ec7
Compare
|
Thanks a lot for reviewing this series, Jack, and for merging #40152 and #40153! I rebased this one onto master, so the diff now only shows its own changes. I also pushed a small commit that swaps Happy to adjust anything you spot. |
jrmccluskey
left a comment
There was a problem hiding this comment.
A few notes from the initial pass
| Year int | ||
| Month time.Month | ||
| Day int | ||
| } |
There was a problem hiding this comment.
Date doesn't have a valid zero value (e.g. Date{} won't encode and will be rejected.) I would suggest treating Date{} as the epoch for clarity.
There was a problem hiding this comment.
Done in e7c4f6c. The zero Date now stands for the epoch, so Date{} encodes as day 0 and prints as 1970-01-01. Decoding day 0 still returns the explicit fields.
There was a problem hiding this comment.
Now that users will be interacting with this code more directly, it's probably a good idea to include a duplication check here (I am okay with this being a follow-up.)
There was a problem hiding this comment.
Added in e7c4f6c. RegisterLogicalType now panics on conflicting registrations for a Go type or an identifier and argument. Registering the same logical type again has no effect.
| // the seconds and the microseconds since the epoch. Encoding a value with | ||
| // sub microsecond precision fails; truncate such values with time.Time.Truncate | ||
| // first. | ||
| type MicrosInstant time.Time |
There was a problem hiding this comment.
Consider implementing String() and Equals() methods on this type for ease of use.
There was a problem hiding this comment.
Added in e7c4f6c. String prints RFC 3339 with microseconds and Equals wraps time.Time.Equal.
# Conflicts: # CHANGES.md
…heck duplicate registrations The zero Date now stands for the epoch, MicrosInstant has String and Equals methods, and RegisterLogicalType panics when a Go type or an identifier and argument are already registered for a different logical type.
|
Thanks for the notes. I pushed e7c4f6c with the three changes and merged master to clear the CHANGES.md conflict. |
Part of #39684.
The Go SDK could only support a portable logical type by registering it in three places (
schema.RegisterLogicalType,coder.RegisterSchemaProviders, and thebeam.SchemaProviderglue) with hand written encoders, and none of the standard logical types from schema.proto were registered. A schema from another SDK with abeam:logical_type:date:v1orbeam:logical_type:micros_instant:v1field failed withunknown logical type.Registration API, in package
schema:RegisterLogicalTypeConversion[GoT, StorageT](urn, toStorage, fromStorage)registers the URN, the Go type and the storage type, and builds the row coder forGoTfrom the row field coder ofStorageTplus the two conversion functions.RegisterParameterizedLogicalTypeConversionis the same for a logical type with an argument; each argument value gets its own Go type.RegisterLogicalTypeCoder[GoT, StorageT](urn, enc, dec)is for logical types whose wire encoding differs from the row encoding of the storage type, such asmillis_instantanddecimal(following PRs).coder.RowFieldEncoderForTypeandcoder.RowFieldDecoderForTypeexpose the row field encoding of a single type, which the registration builds on. Encoding an untyped nil value or a typed nil pointer returns an error. Pointer storage types are rejected at registration because the row coder decides nullability from the Go type of the field, and interface Go types are rejected because the row coder matches every implementation of an interface and the schema matches only the interface itself.Standard types, registered by default:
schema.Date(beam:logical_type:date:v1), a calendar date stored as days since the epoch. Encoding requires a valid date with a year from -999999999 to 999999999.schema.MicrosInstant(beam:logical_type:micros_instant:v1), atime.Timestored as a row of seconds and microseconds. Encoding a value with sub microsecond precision returns an error, as in the Java SDK. Decoding requires the micros field to be in [0, 999999].The standard types are registered with
runtime.RegisterType, so pipeline graphs with these types in a transform signature serialize. The row decoder also uses a provider registered for a value type when it decodes a top level pointer to that type, as the row encoder already does.time.Timekeeps its current Go specific encoding; the standard types are separate Go types, so existing pipelines are unaffected. Because the Go row coder is built from the Go type alone, each registered logical type needs its own Go type, and a parameterized one needs a Go type per argument value.Regression test changes in
sdks/go/test/regression/coders/fromyaml:beam:coder:row:v1case is built from the schema in the coder payload throughschema.ToType, so logical type fields get their registered Go types. This replaces the name based type table.timestamp:v1cases) are read correctly.micros_instantcase runs. The remaining logical type cases are skipped by URN with a reason pointing at [Task]: Go SDK portable logical type support #39684.Tests: schema conversion and byte level row encoding for
DateandMicrosInstant(the bytes are themicros_instantexample of standard_coders.yaml), date conversion at the epoch boundaries, the sub microsecond error,RegisterLogicalTypeConversionend to end with a user type, andRowFieldEncoderForType/RowFieldDecoderForTypefor atomic, slice and struct types. Tests also cover interface Go type rejection, the date and micros storage ranges, a nil pointer value, a top level pointer with a value provider, and graph serialization of the standard 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.