Skip to content

Conversation

agrawalreetika
Copy link
Member

@agrawalreetika agrawalreetika commented Aug 28, 2024

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

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Iceberg Connector Changes
* Add support to query Iceberg table by branch/tag name :pr:`23539`

steveburnett
steveburnett previously approved these changes Aug 28, 2024
Copy link
Contributor

@steveburnett steveburnett left a 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!

@steveburnett
Copy link
Contributor

Nit, update the PR number in the release note entry from the default 12345.

== RELEASE NOTES ==

Iceberg Connector Changes
* Add support to query Iceberg table by branch name or tag name :pr:`23539`

@tdcmeehan tdcmeehan requested a review from gupteaj August 28, 2024 18:41
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@tdcmeehan tdcmeehan self-assigned this Aug 28, 2024
Copy link
Contributor

@ZacBlanco ZacBlanco left a 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

Copy link
Member

@hantangwangd hantangwangd left a 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.

@agrawalreetika
Copy link
Member Author

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.
Since, currently we always use current snapshot schema tag based test with schema changes are blocked.
Will add the tests for tags access with schema changes based on how we decide to handle schema behaviour here

@hantangwangd
Copy link
Member

Will add the tests for tags access with schema changes based on how we decide to handle schema behavior

OK, got it.

Copy link
Member

@hantangwangd hantangwangd left a 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.

@agrawalreetika agrawalreetika self-assigned this Sep 1, 2024
@agrawalreetika
Copy link
Member Author

@tdcmeehan @ZacBlanco Can you check on this one?

@agrawalreetika
Copy link
Member Author

@steveburnett Could you please review the docs changes?

Copy link
Contributor

@steveburnett steveburnett left a 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!

@agrawalreetika agrawalreetika merged commit ffd30da into prestodb:master Sep 3, 2024
57 checks passed
@agrawalreetika agrawalreetika deleted the iceberg-tag-branch branch September 3, 2024 19:07
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
@tdcmeehan tdcmeehan added the from:IBM PR from IBM label Dec 13, 2024
@prestodb-ci prestodb-ci requested review from a team, imjalpreet and infvg and removed request for a team December 13, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for querying branches and tags in Iceberg
5 participants