Spark: Spark SQL Extensions for create branch #6617

hililiwei · 2023-01-18T15:16:00Z

Co-authored-by: Amogh Jahagirdar [email protected]
Co-authored-by: chidayong [email protected]

What is the purpose of the change

Implement the syntax in the following documents:
https://docs.google.com/document/d/1tbATFPrKF3vNlzkgZQdaW8CAJmbjvryfrlg6C2Ci_aA/edit

As suggested by @jackye1995, further split the DDL into smaller PR.

Create Branches

ALTER TABLE tableName
{CREATE | REPLACE} BRANCH branchName [AS OF {VERSION snapshotId}]
[RETAIN interval {DAYS | HOURS | MINUTES}]
[WITH SNAPSHOT RETENTION {[num_snapshots SNAPSHOTS] [interval {DAYS | HOURS | MINUTES}]}]}]

e.g.

ALTER TABLE prod.db.my_table 
CREATE BRANCH test_branch AS OF VERSION 100 
RETAIN 12 MONTHS
WITH SNAPSHOT RETENTION 5 SNAPSHOTS 7 DAYS

cc @jackye1995 @amogh-jahagirdar

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

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

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

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

jackye1995 · 2023-01-18T16:06:41Z

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

+snapshotRetentionClause
+    : WITH SNAPSHOT RETENTION numSnapshots SNAPSHOTS
+    | WITH SNAPSHOT RETENTION snapshotRetain snapshotRetainTimeUnit
+    | WITH SNAPSHOT RETENTION numSnapshots SNAPSHOTS snapshotRetain snapshotRetainTimeUnit


Can we simplify these 3 cases to be WITH SNAPSHOT RETENTION (numSnapshots SNAPSHOTS)? (snapshotRetain snapshotRetainTimeUnit)??

This skips illegal statements : WITH SNAPSHOT RETENTION
or we can use
WITH SNAPSHOT RETENTION ((numSnapshots SNAPSHOTS)? (snapshotRetain snapshotRetainTimeUnit)? | numSnapshots SNAPSHOTS snapshotRetain snapshotRetainTimeUnit )?
But it's not intuitive

I see, I will leave it here to see if anyone has better suggestions. I am not an Antlr expert 😝

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

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

...ark-extensions/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/CreateBranch.scala

amogh-jahagirdar · 2023-01-18T18:49:37Z

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

+      case table =>
+        throw new UnsupportedOperationException(s"Cannot add branch to non-Iceberg table: $table")


I think this case will be common for all the reference based operations. We may want to see about extracting to a common parent. Not needed at this point, but we may revisit in later DDL implementations.

This seems like an existing pattern for all extensions, so I think it is probably fine to leave it like this.

Co-authored-by: Amogh Jahagirdar <[email protected]> Co-authored-by: chidayong <[email protected]>

jackye1995 · 2023-01-20T00:38:31Z

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

+    : MONTHS
+    | DAYS
+    | HOURS
+    | MINUTES


missing SECONDS?

Should we support it? I prefer at least the minute-level.

sure, I am fine with minute level. We can always add more if needed.

...n/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSqlExtensionsAstBuilder.scala

jackye1995 · 2023-01-20T00:42:21Z

Ping some people for thoughts around the syntax: @rdblue @RussellSpitzer @nastra

...n/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSqlExtensionsAstBuilder.scala

jackye1995

looks good to me! Waiting for some feedback from other committers

amogh-jahagirdar

Took another pass, LGTM! thanks for contributing this

jackye1995 · 2023-01-20T22:08:29Z

I think this PR is mostly ready to go. I see there is a comment in design doc from @flyrain:

"VERSION" is used in Iceberg to indicate any table changes including table property changes, which doesn't create a new snapshot. So versions and snapshots are mismatch. Can we call it "SNAPSHOT" to remove the ambiguity?

Requesting one more review from him

jackye1995 · 2023-01-21T08:27:59Z

Looks like there are quite a few duplicates for #6637 without merging this one. In that case given there are 2 committer votes, I will first merge this one to unblock that PR, and will request @flyrain 's review on the next PR. Thanks for the work @hililiwei , and thanks for all the reviews @yyanyy and @amogh-jahagirdar !

rdblue · 2023-01-23T00:00:40Z

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

+    : number
+    ;
+
+snapshotRefRetain


Why are there so many aliases for number? Are these rules useful?

@jackye1995 asked the same question.

I originally added snapshotRefRetain and snapshotRetain to make the statement parsing code more readable. Removing it is technically feasible. In the new version, I have removed (including create branch).
ref: #6637 (comment)

rdblue · 2023-01-23T00:08:20Z

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

+
+public class TestCreateBranch extends SparkExtensionsTestBase {
+
+  @Parameterized.Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}")


Why is this testing with multiple catalogs? This is a table-level operation that shouldn't be affected by the catalog, so it should test just one.

yes, we just test SparkCatalogConfig.SPARK catalog.

@Parameterized.Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}") public static Object[][] parameters() { return new Object[][] { { SparkCatalogConfig.SPARK.catalogName(), SparkCatalogConfig.SPARK.implementation(), SparkCatalogConfig.SPARK.properties() } }; }

rdblue · 2023-01-23T00:09:48Z

.../main/scala/org/apache/spark/sql/execution/datasources/v2/ExtendedDataSourceV2Strategy.scala

      AddPartitionFieldExec(catalog, ident, transform, name) :: Nil

+    case CreateBranch(IcebergCatalogAndIdentifier(catalog, ident), _, _, _, _, _) =>
+      CreateBranchExec(catalog, ident, plan.asInstanceOf[CreateBranch]) :: Nil


Why does this pass the logical plan rather than passing the necessary information? Is it just to avoid a longer line?

I passed in the CreateBranch instance directly to reduce the argument count.

rdblue · 2023-01-23T00:11:39Z

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

+    sql("ALTER TABLE %s CREATE BRANCH %s", tableName, branchName);
+    table.refresh();
+    SnapshotRef ref = table.refs().get(branchName);
+    Assert.assertNotNull(ref);


This should assert the state of the branch. I think it would use the current snapshot of main, right?

updated in #6637.

rdblue · 2023-01-23T00:12:14Z

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

+  @Test
+  public void testCreateBranch() throws NoSuchTableException {
+    Table table = createDefaultTableAndInsert2Row();
+    long snapshotId = table.currentSnapshot().snapshotId();


This should use a snapshot other than the default to test the clause.

Makes sense. updated in #6637.

rdblue · 2023-01-23T00:12:35Z

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

+        tableName, branchName, snapshotId, maxRefAge, minSnapshotsToKeep, maxSnapshotAge);
+    table.refresh();
+    SnapshotRef ref = table.refs().get(branchName);
+    Assert.assertNotNull(ref);


This needs an assertion about the snapshot referenced by the branch.

updated in #6637.

rdblue · 2023-01-23T00:13:53Z

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

+    AssertHelpers.assertThrows(
+        "Illegal statement",
+        IllegalFormatConversionException.class,
+        "d != java.lang.String",


This looks suspicious. Why is this not a mismatched input?

It looks like this isn't reaching the parser and is instead failing in String.format?

fixed in the latest version. #6637

rdblue · 2023-01-23T00:16:55Z

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

+  public void testCreateBranchUseCustomMinSnapshotsToKeepAndMaxSnapshotAge()
+      throws NoSuchTableException {
+    Integer minSnapshotsToKeep = 2;
+    long maxSnapshotAge = 2L;


When using variables like this, it really helps to name them with a unit. That makes it easier to validate the uses, like %d DAYS", ... maxSnapshotAgeDays, ... and TimeUnit.DAYS.toMillis(maxSnapshotAgeDays).

Changed the unit to a variable. Using a fixed unit suffix is not appropriate, any suggestions?

rdblue · 2023-01-23T00:17:24Z

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

+    AssertHelpers.assertThrows(
+        "Illegal statement",
+        IcebergParseException.class,
+        "mismatched input 'SECONDS' expecting {'DAYS', 'HOURS', 'MINUTES'}",


HOURS and MINUTES are never tested, but should be.

I tried using the fourth param to test different units of time.

@Parameterized.Parameters( name = "catalogName = {0}, implementation = {1}, config = {2}, timeUnit = {3}")

hililiwei · 2023-01-23T14:47:36Z

hi @rdblue, thank you for your review. I tried to address your comments in #6637. I can raise a separate PR if you think it's necessary, of course.

Co-authored-by: Amogh Jahagirdar <[email protected]> Co-authored-by: chidayong <[email protected]>

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

hililiwei force-pushed the spark_sql_extend_for_create_branch branch 2 times, most recently from 44b8082 to d8728d0 Compare January 18, 2023 15:27

jackye1995 reviewed Jan 18, 2023

View reviewed changes

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

jackye1995 reviewed Jan 18, 2023

View reviewed changes

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

jackye1995 reviewed Jan 18, 2023

View reviewed changes

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

jackye1995 reviewed Jan 18, 2023

View reviewed changes

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

jackye1995 reviewed Jan 18, 2023

View reviewed changes

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

amogh-jahagirdar reviewed Jan 18, 2023

View reviewed changes

hililiwei force-pushed the spark_sql_extend_for_create_branch branch from d8728d0 to 33375b0 Compare January 19, 2023 06:38

hililiwei and others added 3 commits January 19, 2023 14:41

Spark: Spark SQL Extensions for create branch

c188f50

Co-authored-by: Amogh Jahagirdar <[email protected]> Co-authored-by: chidayong <[email protected]>

Split test

4a65c9c

Change the position of RETAIN

d92e749

hililiwei force-pushed the spark_sql_extend_for_create_branch branch from 33375b0 to 5d815f6 Compare January 19, 2023 06:42

remove default value

7e72948

hililiwei force-pushed the spark_sql_extend_for_create_branch branch from 5d815f6 to 7e72948 Compare January 19, 2023 07:53

jackye1995 reviewed Jan 20, 2023

View reviewed changes

...n/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSqlExtensionsAstBuilder.scala Outdated Show resolved Hide resolved

time retain method

a29615b

hililiwei force-pushed the spark_sql_extend_for_create_branch branch from f7dc678 to a29615b Compare January 20, 2023 02:27

improve UT

f79f304

jackye1995 reviewed Jan 20, 2023

View reviewed changes

...n/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSqlExtensionsAstBuilder.scala Outdated Show resolved Hide resolved

jackye1995 reviewed Jan 20, 2023

View reviewed changes

...n/scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSqlExtensionsAstBuilder.scala Outdated Show resolved Hide resolved

remove time unit

e29c1a1

hililiwei force-pushed the spark_sql_extend_for_create_branch branch from 0149e01 to e29c1a1 Compare January 20, 2023 07:15

jackye1995 approved these changes Jan 20, 2023

View reviewed changes

format style

99c5885

amogh-jahagirdar approved these changes Jan 20, 2023

View reviewed changes

yyanyy approved these changes Jan 20, 2023

View reviewed changes

jackye1995 requested a review from flyrain January 20, 2023 22:08

jackye1995 merged commit fc07921 into apache:master Jan 21, 2023

rdblue reviewed Jan 23, 2023

View reviewed changes

jackye1995 mentioned this pull request Jan 23, 2023

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

Closed

hililiwei deleted the spark_sql_extend_for_create_branch branch February 11, 2023 02:21

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

Spark: SQL Extensions for create branch (apache#6617)

fb16703

Co-authored-by: Amogh Jahagirdar <[email protected]> Co-authored-by: chidayong <[email protected]>

zhangbutao mentioned this pull request Apr 11, 2023

HIVE-27234: Iceberg: CREATE BRANCH SQL implementation apache/hive#4216

Merged

		case table =>
		throw new UnsupportedOperationException(s"Cannot add branch to non-Iceberg table: $table")


		public class TestCreateBranch extends SparkExtensionsTestBase {

		@Parameterized.Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}")

Spark: Spark SQL Extensions for create branch #6617

Spark: Spark SQL Extensions for create branch #6617

Uh oh!

Conversation

hililiwei commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Create Branches

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackye1995 Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jackye1995 commented Jan 20, 2023

Uh oh!

Uh oh!

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

jackye1995 commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackye1995 commented Jan 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hililiwei commented Jan 18, 2023 •

edited

Loading

jackye1995 Jan 18, 2023 •

edited

Loading

jackye1995 commented Jan 20, 2023 •

edited

Loading

rdblue Jan 23, 2023 •

edited

Loading

hililiwei commented Jan 23, 2023 •

edited

Loading