Skip to content

[Go SDK] Add the standard millis_instant and decimal logical types - #40155

Draft
Eliaaazzz wants to merge 4 commits into
apache:masterfrom
Eliaaazzz:go-logical-types-instants-decimal
Draft

Eliaaazzz wants to merge 4 commits into
apache:masterfrom
Eliaaazzz:go-logical-types-instants-decimal

Conversation

@Eliaaazzz

@Eliaaazzz Eliaaazzz commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Part of #39684, and the Go side of #19817. Depends on #40154 (the first three commits); review the last commit.

Both types have a wire encoding that differs from the row encoding of their representation type, so they use schema.RegisterLogicalTypeCoder:

  • schema.MillisInstant (beam:logical_type:millis_instant:v1), a time.Time with millisecond precision. The representation is INT64, but Java (InstantCoder) and Python (TimestampCoder, special cased in row_coder.py) encode it as 8 big endian bytes shifted by the minimum int64, so the byte order matches the chronological order. The Go encoding uses the same shift as coder.EncodeEventTime. Encoding a value with sub millisecond precision returns an error.
  • schema.Decimal (beam:logical_type:decimal:v1), an arbitrary precision decimal with Unscaled *big.Int and Scale int32. The representation is BYTES, encoded as Java's BigDecimalCoder and Python's DecimalCoder do: the scale as an unsigned 32 bit varint (the INT32 varint form, so negative scales such as 1E+3 round trip with Java) followed by the length prefixed two's complement bytes of the unscaled value. ParseDecimal and String convert to and from plain notation.

The millis_instant and decimal cases of standard_coders.yaml now run in the Go regression test.

MillisInstant and Decimal are registered with runtime.RegisterType like the other standard types.

Tests: byte level row encoding against the yaml examples for both types (including the negative decimal and the shifted millis value), two's complement conversion at the byte boundaries, decimal parsing and formatting, and the sub millisecond error.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: 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, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

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)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@Eliaaazzz

Copy link
Copy Markdown
Contributor Author

Closing this for now to keep the series at three open PRs while #40152, #40153 and #40154 are reviewed. The branch stays in my fork.

@Eliaaazzz Eliaaazzz closed this Sep 17, 2026
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.
@Eliaaazzz Eliaaazzz reopened this Oct 7, 2026
@Eliaaazzz
Eliaaazzz force-pushed the go-logical-types-instants-decimal branch from 5672f19 to 31d5ef9 Compare October 7, 2026 22:53
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

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 assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Logical types registered with an argument now emit argument_type and
argument in their FieldType, and schemas from other SDKs are matched on
the URN and the argument value. A logical type registered without an
argument still matches any argument, so pass through types keep
working. Top level logical types carry the argument in a schema option.

Part of apache#39684.
… 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.
schema.MillisInstant encodes as the Java InstantCoder does: 8 big endian
bytes of the milliseconds shifted by the minimum int64. schema.Decimal
encodes as the Java BigDecimalCoder does: the scale as a varint followed
by the length prefixed two's complement bytes of the unscaled value.
Both cases of standard_coders.yaml now run in the Go regression test.

Part of apache#39684.

@shunping shunping left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, @Eliaaazzz!

Since the prior PRs in this series are still being reviewed, I'd prefer to wait until those go through the pipeline first to avoid any cascading changes you might have to make here.

@Eliaaazzz
Eliaaazzz marked this pull request as draft October 8, 2026 01:18
@Eliaaazzz

Copy link
Copy Markdown
Contributor Author

Thanks, that makes sense. I moved this PR and #40156 to draft until #40152 to #40154 are merged, and I will rebase them then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants