add Timestamp.MICROS for iceberg timestamptz - #39592
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
cc: @ahmedabu98 |
|
Assigning reviewers: R: @kennknowles for label java. 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). |
ahmedabu98
left a comment
ahmedabu98
left a comment
There was a problem hiding this comment.
Thanks @aIbrahiim, I left a couple comments.
I think we should also add a TODO somewhere to make the Timestamp logical type the new default for SQL timestamps instead of primitive FieldType.DATETIME
| LocalDate.ofEpochDay(((Number) value).longValue() / MILLIS_PER_DAY), | ||
| LocalTime.ofNanoOfDay( | ||
| (((Number) value).longValue() % MILLIS_PER_DAY) * NANOS_PER_MILLISECOND)); | ||
| } else if (org.apache.beam.sdk.schemas.logicaltypes.Timestamp.IDENTIFIER.equals( |
There was a problem hiding this comment.
Can we add some unit tests (non-iceberg) to validate the additions in this file?
| Schema fromIceberg = IcebergUtils.icebergSchemaToBeamSchema(icebergTable.schema()); | ||
| assertEquals( | ||
| FieldType.logicalType(Timestamp.MICROS).withNullable(true), | ||
| fromIceberg.getField("c_timestamp").getType()); |
There was a problem hiding this comment.
I would remove the assertion here because it's misleading. The original assertion was intended to check the SQL table's schema
| return nullOr( | ||
| value, | ||
| Expressions.call( | ||
| Expressions.convert_(value, java.time.Instant.class), "toEpochMilli")); |
There was a problem hiding this comment.
Timestamp.MICROS can contain sub-millisecond values, but this conversion goes through epoch milliseconds. I reproduced this with SELECT * FROM PCOLLECTION: 2025-07-31T20:17:40.123456Z comes back as 2025-07-31T20:17:40.123Z. Could we preserve microsecond precision here, or reject values that Calcite cannot represent without loss?
3 participants
Fixes PreCommit/PostCommit SQL failures caused by #39344, which maps Iceberg timestamptz to Beam Timestamp.MICROS
Beam SQL did not handle that logical type, and one Iceberg SQL test compared the wrong schemas (SQL CREATE still uses DATETIME; Iceberg reload now returns Timestamp.MICROS).
#35514
#34314
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.