-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Managed Iceberg] Add support for TIMESTAMP, TIME, and DATE types #32688
Conversation
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
assign set of reviewers |
Assigning reviewers. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergUtils.java
Outdated
Show resolved
Hide resolved
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergUtils.java
Show resolved
Hide resolved
Thanks for taking an initial look @DanielMorales9. This PR is ready for another review |
Beam ( I added support for String timestamps to be parsed to OffsetDateTime. But I wonder if it makes sense to allow other types as well (LocalDateTime, joda DateTime, long) and resolve all these types to OffsetDateTime at UTC. This will allow us to have a more unified approach to timestamps - and Iceberg always resolves to UTC anyways so there's no harm in doing it from our side |
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergUtils.java
Outdated
Show resolved
Hide resolved
…ql.datetime and timestamptTZ returns datetime
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergUtils.java
Outdated
Show resolved
Hide resolved
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergUtils.java
Show resolved
Hide resolved
Thanks for the LGTM -- will merge when tests pass |
Fixes #32680