-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Add support to query Iceberg table by branch/tag name #23539
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
Add support to query Iceberg table by branch/tag name #23539
Conversation
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! (docs)
Pull branch, local doc build, looks good. Thanks!
Nit, update the PR number in the release note entry from the default
|
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.
Nice work!
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java
Outdated
Show resolved
Hide resolved
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.
code and tests LGTM, one suggestion on the error message
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java
Outdated
Show resolved
Hide resolved
a496bc2
to
f8bcd71
Compare
f8bcd71
to
e935a0a
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.
Should we add some test cases about query on schema changes? Referring to: https://iceberg.apache.org/docs/nightly/branching/#schema-selection-with-branches-and-tags. It seems that Iceberg has some differences in handling queries on branch and tag.
If any schema changes are performed after tag creation, Tag based query access uses the snapshot's schema. |
OK, got it. |
e935a0a
to
e066ac7
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.
Thanks for adding this feature. Change looks good to me since we have not yet decide how to choose the schema of queries on tag and snapshot.
@tdcmeehan @ZacBlanco Can you check on this one? |
@steveburnett Could you please review the docs changes? |
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! (docs)
Pull updated branch, new local doc build, looks good. Thanks!
Description
Add support to query Iceberg table by branch/tag name
Motivation and Context
Resolves #22029
Impact
Resolves #22029
Test Plan
Added tests
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.