Spark: REPLACE BRANCH SQL implementation #6638

amogh-jahagirdar · 2023-01-22T02:02:23Z

Co-authored-by: liliwei [email protected]
Co-authored-by: xuwei [email protected]
Co-authored-by: chidayong [email protected]

CC: @jackye1995 @hililiwei @flyrain

amogh-jahagirdar · 2023-01-22T02:07:18Z

...sions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4

    | ALTER TABLE multipartIdentifier CREATE BRANCH identifier (AS OF VERSION snapshotId)? (RETAIN snapshotRefRetain snapshotRefRetainTimeUnit)? (snapshotRetentionClause)?    #createBranch
+    | ALTER TABLE multipartIdentifier REPLACE BRANCH identifier (AS OF VERSION snapshotId)? (RETAIN snapshotRefRetain snapshotRefRetainTimeUnit)? (snapshotRetentionClause)?  #replaceBranch


After writing this out, starting to change my mind :) maybe we combine {CREATE | REPLACE} in the same definition if that's possible?

Agree, I think in this case there are a few arguments for combining it:

syntax is exactly the same, and adding replace does not complicate the logic too much

CREATE/REPLACE table follows a similar pattern

At API level, we also have similar thing like createOrReplaceTableTransaction

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java

...ensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ReplaceBranchExec.scala

...sions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4

...src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateOrReplaceBranchExec.scala

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java

...sions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java

...src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateOrReplaceBranchExec.scala

...3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestCreateBranch.java

jackye1995 · 2023-01-31T03:10:52Z

...src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateOrReplaceBranchExec.scala

+            return Nil
+          }
+
+          manageSnapshots.createBranch(branch, snapshotId)


looks like we are implicitly letting the API throw exception for branch already exists. It's fine to me, but would like to know what other people think.

That's right, it's implicits since the API provides those guarantees. We could also throw our own before calling createBranch but, I'll let @hililiwei @yyanyy @rdblue provide their thoughts.

sounds good to me

jackye1995

Mostly looks good to me, just 1 doubt regarding create existing branch. @hililiwei @yyanyy @rdblue any thoughts?

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java

...src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateOrReplaceBranchExec.scala

flyrain

LGTM. Thanks for working on it.
Not related to this PR directly, neither a blocker. Do we need to delete a branch? I didn't go through all design details, but I assume this is how a branch is deleted now. We relies on the retain to keep the branch. A branch will be clean up by procedure expireSnapshots once it expired. Could you confirm?

Co-authored-by: liliwei [email protected] Co-authored-by: xuwei [email protected] Co-authored-by: chidayong [email protected]

amogh-jahagirdar · 2023-02-02T22:09:51Z

Thanks for the review @flyrain really appreciate it!

So there are a few operations:

1.) replaceBranch: Replace branch will change the snapshot that the branch points to. Nothing will be removed here. The same principle applies for replaceTag.

2.) The removeBranch API (DROP BRANCH in SQL): This will remove the branch from the references in metadata. When this is committed it's an immediate removal of the reference.

Correct that a branch will be cleaned up during snapshot expiration if it is past the reference retention age maxRefAgeMs in the code. The updated expiration algorithm which considers branching and tagging can be found here https://iceberg.apache.org/spec/#snapshot-retention-policy

I'm also working on a PR for a dedicated branching/tagging doc page which has some details there. #6723 appreciate any feedback on this as well if anything is unclear!

jackye1995 · 2023-02-02T23:08:01Z

Looks like we have enough votes and all comments are addressed. I will go ahead to merge this, and we can address further comments in subsequent PRs like #6637

Thanks @amogh-jahagirdar and @hililiwei for the work, thanks @yyanyy and @flyrain for the review!

amogh-jahagirdar · 2023-02-02T23:12:17Z

Thanks for the reviews @flyrain @jackye1995 @yyanyy @hililiwei!

Co-authored-by: liliwei [email protected] Co-authored-by: xuwei [email protected] Co-authored-by: chidayong [email protected]

github-actions bot added the spark label Jan 22, 2023

amogh-jahagirdar commented Jan 22, 2023

View reviewed changes

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java Show resolved Hide resolved

amogh-jahagirdar force-pushed the replace-branch branch from f1ca19f to 78b000f Compare January 22, 2023 03:27

jackye1995 mentioned this pull request Jan 23, 2023

Support Iceberg version travel by reference name trinodb/trino#15646

Closed

amogh-jahagirdar commented Jan 23, 2023

View reviewed changes

...ensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ReplaceBranchExec.scala Outdated Show resolved Hide resolved

amogh-jahagirdar force-pushed the replace-branch branch from 78b000f to 3320eb8 Compare January 23, 2023 22:00

amogh-jahagirdar commented Jan 23, 2023

View reviewed changes

...sions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4 Show resolved Hide resolved

amogh-jahagirdar commented Jan 23, 2023

View reviewed changes

...src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateOrReplaceBranchExec.scala Outdated Show resolved Hide resolved

yyanyy reviewed Jan 24, 2023

View reviewed changes

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java Show resolved Hide resolved

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java Outdated Show resolved Hide resolved

jackye1995 reviewed Jan 24, 2023

View reviewed changes

...sions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4 Outdated Show resolved Hide resolved

amogh-jahagirdar force-pushed the replace-branch branch 4 times, most recently from 71206d9 to 245cf1b Compare January 29, 2023 09:06

jackye1995 reviewed Jan 30, 2023

View reviewed changes

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java Outdated Show resolved Hide resolved

jackye1995 reviewed Jan 30, 2023

View reviewed changes

...src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateOrReplaceBranchExec.scala Outdated Show resolved Hide resolved

hililiwei reviewed Jan 30, 2023

View reviewed changes

...3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestCreateBranch.java Outdated Show resolved Hide resolved

amogh-jahagirdar force-pushed the replace-branch branch from 245cf1b to 4c22fb6 Compare January 30, 2023 23:53

jackye1995 reviewed Jan 31, 2023

View reviewed changes

jackye1995 approved these changes Jan 31, 2023

View reviewed changes

amogh-jahagirdar force-pushed the replace-branch branch from 4c22fb6 to 3f663fb Compare February 2, 2023 00:14

yyanyy approved these changes Feb 2, 2023

View reviewed changes

jackye1995 requested review from flyrain and hililiwei and removed request for hililiwei February 2, 2023 05:52

flyrain reviewed Feb 2, 2023

View reviewed changes

....3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestReplaceBranch.java Outdated Show resolved Hide resolved

flyrain reviewed Feb 2, 2023

View reviewed changes

...src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateOrReplaceBranchExec.scala Outdated Show resolved Hide resolved

flyrain approved these changes Feb 2, 2023

View reviewed changes

Spark: REPLACE BRANCH SQL implementation

59e5c7d

Co-authored-by: liliwei [email protected] Co-authored-by: xuwei [email protected] Co-authored-by: chidayong [email protected]

amogh-jahagirdar force-pushed the replace-branch branch from 3f663fb to 59e5c7d Compare February 2, 2023 22:03

amogh-jahagirdar mentioned this pull request Feb 2, 2023

Spark: Spark SQL Extensions for create tag #6637

Merged

jackye1995 merged commit 8b6c777 into apache:master Feb 2, 2023

amogh-jahagirdar mentioned this pull request Feb 3, 2023

Spark: Fix isIcebergCommand check and style issues from #6638 #6728

Merged

krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023

Spark 3.3: REPLACE BRANCH SQL implementation (apache#6638)

e6cb8db

Co-authored-by: liliwei [email protected] Co-authored-by: xuwei [email protected] Co-authored-by: chidayong [email protected]

		\| ALTER TABLE multipartIdentifier CREATE BRANCH identifier (AS OF VERSION snapshotId)? (RETAIN snapshotRefRetain snapshotRefRetainTimeUnit)? (snapshotRetentionClause)? #createBranch
		\| ALTER TABLE multipartIdentifier REPLACE BRANCH identifier (AS OF VERSION snapshotId)? (RETAIN snapshotRefRetain snapshotRefRetainTimeUnit)? (snapshotRetentionClause)? #replaceBranch

Spark: REPLACE BRANCH SQL implementation #6638

Spark: REPLACE BRANCH SQL implementation #6638

Uh oh!

Conversation

amogh-jahagirdar commented Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amogh-jahagirdar Jan 22, 2023

Choose a reason for hiding this comment

Uh oh!

jackye1995 Jan 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackye1995 Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

yyanyy Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackye1995 commented Feb 2, 2023

Uh oh!

amogh-jahagirdar commented Feb 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amogh-jahagirdar commented Jan 22, 2023 •

edited

Loading

amogh-jahagirdar commented Feb 2, 2023 •

edited

Loading