Skip to content

Comments

Introduce Rolling forward to snapshot with Presto Procedure on Iceberg table#21073

Closed
Hank27-zhang wants to merge 1 commit intoprestodb:masterfrom
Hank27-zhang:rollforward
Closed

Introduce Rolling forward to snapshot with Presto Procedure on Iceberg table#21073
Hank27-zhang wants to merge 1 commit intoprestodb:masterfrom
Hank27-zhang:rollforward

Conversation

@Hank27-zhang
Copy link

@Hank27-zhang Hank27-zhang commented Oct 9, 2023

Description

The current existing pedicure rollback_to_snapshot (https://prestodb.io/docs/current/connector/iceberg.html#id2) in the Iceberg connector allows only rolling forward to an old snapshot but we can't roll forward to a different snapshot of ta Iceberg Table. Update the API to support roll-forward as well.
Related issue: #20881

Test Plan

update IcebergDistributedSmokeTestBase.testRollbackSnapshot()

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 Changes
*Rolling forward to snapshot with Presto Procedure on Iceberg table

If release note is NOT required, use:

== NO RELEASE NOTE ==


@Hank27-zhang Hank27-zhang requested a review from a team as a code owner October 9, 2023 06:25
@linux-foundation-easycla
Copy link

CLA Not Signed

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.

On looking at spark procedures in the official iceberg documentation, I feel Presto should be in sync with them so that the users who use spark as well are not confused with the working of each of them.

Spark has two separate procedures for rollback_to_snapshot (https://iceberg.apache.org/docs/1.3.1/spark-procedures/#rollback_to_snapshot) and set_current_snapshot (https://iceberg.apache.org/docs/1.3.1/spark-procedures/#set_current_snapshot).

I had a look at their implementation and they use manageSnapshots().rollbackTo in the case of rollback_to_snapshot and manageSnapshots().setCurrentSnapshot in the other case.

rollback_to_snapshot: https://github.com/apache/iceberg/blob/master/spark/v3.1/spark/src/main/java/org/apache/iceberg/spark/procedures/RollbackToSnapshotProcedure.java#L88C67-L88C67

set_current_snapshot: https://github.com/apache/iceberg/blob/master/spark/v3.1/spark/src/main/java/org/apache/iceberg/spark/procedures/SetCurrentSnapshotProcedure.java#L89

IMO we should also introduce another procedure for setCurrentSnapshot rather than updating the current procedure.

@imjalpreet
Copy link
Member

CLA Not Signed

Also, please sign the CLA.

@agrawalreetika
Copy link
Member

Hi @Hank27-zhang,
Even I think adding a separate procedure set_current_snapshot for setting snapshot would be better and keep the existing one as it is. Please let us know what you think about that.

@tdcmeehan
Copy link
Contributor

@Hank27-zhang are you still working on this?

@wanglinsong wanglinsong requested a review from hantangwangd as a code owner July 6, 2024 04:32
@agrawalreetika
Copy link
Member

Closing this in favour of #23567

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants