-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Allow for date -> time promotion in v3 #14266
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
base: main
Are you sure you want to change the base?
Conversation
e610f65
to
28dccba
Compare
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.
I see few things are missing:
- Add partition field validation logic
- Add test for partition field validation
- Add negative test: v2 tables rejecting date→timestamp
- Add negative test: v3 tables rejecting date→timestamptz
- update formatVersion parameter in JavaDoc
- Please update adding integration test
- Add promotion test on TestSchemaUnionByFieldName.java
- add tests for Parquet another data formats that verifies that data written with old types can be read correctly after promotion.
api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java
Outdated
Show resolved
Hide resolved
One more thing Did you have a chance to run revapi ? @rambleraptor https://iceberg.apache.org/contribute/#checking-for-api-breakages |
28dccba
to
89754a4
Compare
@talatuyarer revapi is passing |
@talatuyarer changes made! I'm unsure what a Parquet test would look like in practice. Doesn't look like we have an existing test that I can work off of. |
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.
LGTM, I took a similar approach in my abandoned PR.
@nandorKollar thanks so much for the review! |
Do we account for metrics written as the pre promotion type? I think this code is already part of metrics evaluation but we should test to make sure a promoted field still filters properly |
@RussellSpitzer @talatuyarer I just added a metrics test for Parquet, which I think covers the use case you're describing. Let me know if it doesn't. |
parquet/src/main/java/org/apache/iceberg/parquet/ParquetConversions.java
Outdated
Show resolved
Hide resolved
f0d5d23
to
a4ba53a
Compare
parquet/src/main/java/org/apache/iceberg/parquet/ParquetConversions.java
Outdated
Show resolved
Hide resolved
This adds promotion logic for date->timestamp, as well as proper plumbing to ensure that these promotions only occur for Table Format v3.
a4ba53a
to
d108715
Compare
Closes #14265
This adds promotion logic for date->timestamp, as well as proper plumbing to ensure that these promotions only occur for Table Format v3