-
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
[IcebergIO] Use InternalRecordWrapper partition util #33701
[IcebergIO] Use InternalRecordWrapper partition util #33701
Conversation
assign set of reviewers |
Assigning reviewers. If you would like to opt out of this review, comment R: @m-trieu for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
@@ -465,6 +486,10 @@ public void testIdentityPartitioning() throws IOException { | |||
.identity("float") | |||
.identity("double") | |||
.identity("str") | |||
.identity("date") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my knowledge, it's unclear to me how testIdentityPartitioning()
was passing when identity should support Any source type. Is it worth adding an integration test to ensure just the Write
works when partitioning by just date
, as well as Any
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test used to not include date
and the other time types. This PR adds adds them to the test (your comment is actually positioned on the lines that add them).
Previously we were manually doing conversions to determine a record's partition. We recently found that Iceberg has existing public utils that do this; this change switches to use those utils.
Fixes #32865