Skip to content

Comments

[Iceberg] Support procedure set_current_snapshot for Iceberg#23567

Merged
agrawalreetika merged 1 commit intoprestodb:masterfrom
agrawalreetika:set_current_snapshot-procedure
Sep 6, 2024
Merged

[Iceberg] Support procedure set_current_snapshot for Iceberg#23567
agrawalreetika merged 1 commit intoprestodb:masterfrom
agrawalreetika:set_current_snapshot-procedure

Conversation

@agrawalreetika
Copy link
Member

@agrawalreetika agrawalreetika commented Aug 31, 2024

Description

Support for procedure set_current_snapshot for Iceberg

Motivation and Context

Support for procedure set_current_snapshot for Iceberg. https://iceberg.apache.org/docs/1.5.1/spark-procedures/#set_current_snapshot
Since Presto already has rollback_to_snapshot procedure to go back to a snapshot, there was no way to forward it after doing so. Here set_current_snapshot procedure would be useful to handle this scenario.

Impact

New Procedure is introduced which can be used something like -

  • Set current table snapshot ID for the given table to 10000:

    CALL iceberg.system.set_current_snapshot('schema_name', 'table_name', 10000);

  • Set current table snapshot ID for the given table to snapshot ID of branch1:

    CALL iceberg.system.set_current_snapshot('schema_name', 'table_name', 'branch1');

Test Plan

Added new tests with a different scenario

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

Iceberg Connector Changes
*  Support new procedure set_current_snapshot for Iceberg :pr:`23567`

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.

Overall looks good to me, one little thing about parameter checking and error message.

@tdcmeehan tdcmeehan self-assigned this Sep 3, 2024
@agrawalreetika agrawalreetika force-pushed the set_current_snapshot-procedure branch from 85da3a4 to ce037f3 Compare September 3, 2024 15:09
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.

Thanks for the doc! A few nits and suggestions of rephrasing and formatting.

Let me know if my suggestions change your intended meaning in a way that is not correct!

@agrawalreetika agrawalreetika force-pushed the set_current_snapshot-procedure branch from ce037f3 to d065c5b Compare September 3, 2024 18:01
steveburnett
steveburnett previously approved these changes Sep 3, 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 updated branch, new local doc build, looks good. Thanks!

Copy link
Member

@imjalpreet imjalpreet 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 the contribution, I have a few minor suggestions.

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 the fix. LGTM!

Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@agrawalreetika agrawalreetika merged commit a0e9758 into prestodb:master Sep 6, 2024
@agrawalreetika agrawalreetika deleted the set_current_snapshot-procedure branch September 6, 2024 18:18
@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 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.

6 participants