Skip to content

Support procedure fast_forward for iceberg#23589

Merged
agrawalreetika merged 1 commit intoprestodb:masterfrom
agrawalreetika:iceberg-fast-forward-procedure
Sep 6, 2024
Merged

Support procedure fast_forward for iceberg#23589
agrawalreetika merged 1 commit intoprestodb:masterfrom
agrawalreetika:iceberg-fast-forward-procedure

Conversation

@agrawalreetika
Copy link
Member

@agrawalreetika agrawalreetika commented Sep 5, 2024 •
edited
Loading

Description

Support procedure fast_forward for iceberg

Motivation and Context

Resolves #22032

Impact

Resolves #22032

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
* Support procedure fast_forward for iceberg :pr:`23589`

@agrawalreetika agrawalreetika self-assigned this Sep 5, 2024
steveburnett
steveburnett requested changes Sep 5, 2024
View reviewed 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.

Thanks for the doc! I made some suggestions that I think clarify your intended meaning.

If my suggestions change the meaning in a way you think is incorrect, suggest something more correct back to me, or help me understand.

@agrawalreetika agrawalreetika force-pushed the iceberg-fast-forward-procedure branch from 6d32e47 to dd29ecf Compare September 5, 2024 16:50
steveburnett
steveburnett previously approved these changes Sep 5, 2024
View reviewed 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)

Thanks for the quick revision! Pull updated branch, review new local doc build, looks good.

hantangwangd
hantangwangd reviewed Sep 5, 2024
View reviewed changes
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 procedure, overall looks good to me. Some little nits.

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated
Fast Forward Branch
^^^^^^^^^^^^^^^^^^^

This procedure advances the current branch to a more recent snapshot from another branch without replaying any intermediate snapshots.
Copy link
Member

@hantangwangd hantangwangd Sep 5, 2024

Choose a reason for hiding this comment

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

Should it be current snapshot of the specified branch rather than current branch? As I understand, the specified branch to fast-forward may not necessarily be the current branch.

...-iceberg/src/main/java/com/facebook/presto/iceberg/procedure/FastForwardBranchProcedure.java
ImmutableList.of(
new Procedure.Argument("schema", VARCHAR),
new Procedure.Argument("table_name", VARCHAR),
new Procedure.Argument("branch", VARCHAR),
Copy link
Member

@hantangwangd hantangwangd Sep 5, 2024

Choose a reason for hiding this comment

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

Here the argument branch is required, meanwhile in the document it is optional, please keep them consistent.

ZacBlanco
ZacBlanco reviewed Sep 5, 2024
View reviewed changes
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.

procedure looks good. Just some nits

@tdcmeehan tdcmeehan self-assigned this Sep 5, 2024
@agrawalreetika agrawalreetika force-pushed the iceberg-fast-forward-procedure branch from dd29ecf to acfeaec Compare September 5, 2024 19:29
hantangwangd
hantangwangd reviewed Sep 6, 2024
View reviewed changes
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, one little thing, otherwise LGTM.

ZacBlanco
ZacBlanco previously approved these changes Sep 6, 2024
View reviewed changes
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.

LGTM

@agrawalreetika agrawalreetika force-pushed the iceberg-fast-forward-procedure branch from acfeaec to 959ee6b Compare September 6, 2024 18:17
@agrawalreetika agrawalreetika force-pushed the iceberg-fast-forward-procedure branch from 959ee6b to a87dd74 Compare September 6, 2024 19:08
@agrawalreetika agrawalreetika merged commit 8964fed into prestodb:master Sep 6, 2024
@agrawalreetika agrawalreetika deleted the iceberg-fast-forward-procedure branch September 6, 2024 20:43
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
Merged
25 tasks
@tdcmeehan tdcmeehan added the from:IBM PR from IBM label Dec 13, 2024
@prestodb-ci prestodb-ci requested review from a team, bibith4 and wanglinsong 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

Reviewers

@steveburnett steveburnett steveburnett left review comments

@hantangwangd hantangwangd hantangwangd approved these changes

@ZacBlanco ZacBlanco ZacBlanco left review comments

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@bibith4 bibith4 Awaiting requested review from bibith4 bibith4 was automatically assigned from prestodb/ibm-reviewers

@wanglinsong wanglinsong Awaiting requested review from wanglinsong wanglinsong was automatically assigned from prestodb/ibm-reviewers

Assignees

@agrawalreetika agrawalreetika

@tdcmeehan tdcmeehan

Labels

from:IBM PR from IBM

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for fast forwarding branches

5 participants

@agrawalreetika @ZacBlanco @steveburnett @hantangwangd @tdcmeehan

Comments