Spark: Spark SQL Extensions for create tag #6637

hililiwei · 2023-01-21T06:45:51Z

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

Create TAG

ALTER TABLE table CREAT TAG tagName
[AS OF {VERSION snapshotId}]
[RETAIN interval {DAYS | HOURS | MINUTES}]

e.g.

ALTER TABLE table CREATE TAG historical_annual_snapshot
AS OF VERSION 100
RETAIN 12 DAYS

cc @jackye1995 @amogh-jahagirdar

hililiwei · 2023-01-21T06:50:28Z

In the original proposal, it was "[RETAIN For interval {DAYS | HOURS | MINUTES}]", but in keeping with CREATE BRANCH, I removed the" For" key. What do you think about that? @jackye1995 @amogh-jahagirdar

jackye1995 · 2023-01-21T07:03:46Z

Good point, +1 for only RETAIN because https://docs.databricks.com/sql/language-manual/delta-vacuum.html

amogh-jahagirdar · 2023-01-21T16:47:08Z

In the original proposal, it was "[RETAIN For interval {DAYS | HOURS | MINUTES}]", but in keeping with CREATE BRANCH, I removed the" For" key. What do you think about that? @jackye1995 @amogh-jahagirdar

Ah yes, we should remove the FOR clause. That's a mistake in the proposal on my part. Updated the proposal.

jackye1995

Overall looks good to me, I will ignore the duplicated code and do another pass after rebase. Added 2 comments for discussion.

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

amogh-jahagirdar · 2023-02-02T22:43:12Z

@hililiwei similar to #6638 could this PR encapsulate create/replace? We came to the conclusion on the replaceBranch PR it made more sense to just combine them. at the end of the day the logical plan needs to pass through whether it's a replace or not and it is a bit more concise in the grammar definition to combine them while still maintaining readability

jackye1995 · 2023-02-03T01:08:38Z

could this PR encapsulate create/replace?

+1

Let me know when this is updated, I will take another look!

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

jackye1995 · 2023-02-03T06:01:31Z

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

    ;

+tagOptions
+    : (AS OF VERSION snapshotId)? (refRetain)?;


hmm I just noticed this ; in the end, also in branchOptions. @amogh-jahagirdar is this required? It doesn't feel like required to me.

this is required. We can insert a new line, consistent with the rest.

jackye1995 · 2023-02-03T06:02:07Z

.../scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala

-            normalized.contains("replace branch")
+            normalized.contains("create branch") ||
+            normalized.contains("replace branch") ||
+            normalized.contains("create tag")))


I think there is a fix for this at #6728 , might want to rebase after that is merged

jackye1995 · 2023-02-03T06:02:59Z

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


+  /**
+   * Create an CREATE OR REPLACE BRANCH logical command.
+   */


nice catches! But can we avoid unrelated changes in the PR? @amogh-jahagirdar I think you should fix these separately

btw I am surprised that checkstyle did not catch that extra space, maybe it needs some more configuration.

For scala, our checkstyle needs to be improved. And auto-formatting doesn't work yet.

Great catch @hililiwei , I'll update #6728 to fix the styling issues. We should track an issue for scala checkstyle/autoformatting , admittedly I rely on that quite heavily to catch these kinds of issues.

Created #6736 and updated #6728 to include styling fixes

Thanks, I merged #6728

jackye1995 · 2023-02-03T19:06:59Z

Apart from the style changes for branch that can now be removed, I have no further comments. Let me know when this is rebased!

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

aokolnychyi · 2023-02-04T03:06:12Z

cc @RussellSpitzer @szehon-ho @flyrain @karuppayya

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

hililiwei · 2023-02-06T08:56:48Z

hililiwei requested review from jackye1995 and removed request for flyrain and yyanyy

Misclick. 😂

Apart from the style changes for branch that can now be removed, I have no further comments. Let me know when this is rebased!

PTAL, thx.

jackye1995

Mostly looks good to me! @amogh-jahagirdar @aokolnychyi any further comments?

amogh-jahagirdar · 2023-02-07T17:09:51Z

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

+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+public class TestReplaceTag extends SparkExtensionsTestBase {


Can we move all the tag tests to TestTagDDL based on the discussion here #6752 (comment)? Not blocking for this PR we can get in a follow on, but it's basically just to ensure the test runs aren't super slow with all the spark setup which will happen for every test class if we have a test class per statement.

amogh-jahagirdar

Thanks LiWei, one non-blocking comment on our test setup for all the branching/tagging DDLs. We can address on a follow on PR.

yyanyy

lgtm!

jackye1995 · 2023-02-10T21:59:33Z

All the comments are addressed and we have enough votes, so I will go ahead to merge it. I requested @aokolnychyi for further review and approval, but given it has been quite a few days, and there is a subsequent DROP TAG PR anyway, we can move forward and address any missing comments there.

Thanks for the contribution @hililiwei , and thanks everyone for the review!

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

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

jackye1995 mentioned this pull request Jan 21, 2023

Spark: Spark SQL Extensions for create branch #6617

Merged

jackye1995 requested review from flyrain, jackye1995 and yyanyy January 21, 2023 08:28

jackye1995 reviewed Jan 21, 2023

View reviewed changes

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

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

hililiwei force-pushed the spark_sql_extend_for_create_tag branch 3 times, most recently from effe7e9 to 6b94b9c Compare January 23, 2023 15:35

This was referenced Jan 23, 2023

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

Closed

Spark: REPLACE BRANCH SQL implementation #6638

Merged

hililiwei force-pushed the spark_sql_extend_for_create_tag branch 2 times, most recently from 9d16516 to 8f73631 Compare February 3, 2023 04:37

jackye1995 reviewed Feb 3, 2023

View reviewed changes

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

jackye1995 reviewed Feb 3, 2023

View reviewed changes

amogh-jahagirdar mentioned this pull request Feb 3, 2023

Improving Scala Checkstyle/Spotless configurations #6736

Closed

aokolnychyi reviewed Feb 4, 2023

View reviewed changes

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

hililiwei and others added 2 commits February 6, 2023 10:04

Spark: Spark SQL Extensions for create tag

41b4e04

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

add replace

72c1ee6

hililiwei force-pushed the spark_sql_extend_for_create_tag branch 2 times, most recently from 8a8c670 to 9dbdbed Compare February 6, 2023 07:02

hililiwei force-pushed the spark_sql_extend_for_create_tag branch 2 times, most recently from 03eacc8 to 3c60df2 Compare February 6, 2023 07:06

address comments

f51effa

hililiwei force-pushed the spark_sql_extend_for_create_tag branch from 3c60df2 to f51effa Compare February 6, 2023 07:07

hililiwei requested review from jackye1995 and removed request for flyrain and yyanyy February 6, 2023 08:46

jackye1995 approved these changes Feb 7, 2023

View reviewed changes

amogh-jahagirdar reviewed Feb 7, 2023

View reviewed changes

amogh-jahagirdar approved these changes Feb 7, 2023

View reviewed changes

test class name

68c0353

hililiwei force-pushed the spark_sql_extend_for_create_tag branch from 8da50f4 to 68c0353 Compare February 10, 2023 03:22

improve ut

463d042

yyanyy approved these changes Feb 10, 2023

View reviewed changes

jackye1995 merged commit 4c06b4c into apache:master Feb 10, 2023

hililiwei deleted the spark_sql_extend_for_create_tag branch February 11, 2023 01:59

jackye1995 mentioned this pull request Feb 14, 2023

Spark 3.3: SQL Extensions for DROP TAG #6807

Merged

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

Spark 3.3: SQL Extensions for CREATE TAG (apache#6637)

18fe6f2

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

dramaticlly mentioned this pull request May 19, 2023

Spark 3.2: backport Spark SQL extension on create/update/drop tags #7662

Merged

Spark: Spark SQL Extensions for create tag #6637

Spark: Spark SQL Extensions for create tag #6637

Uh oh!

Conversation

hililiwei commented Jan 21, 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 TAG

Uh oh!

hililiwei commented Jan 21, 2023

Uh oh!

jackye1995 commented Jan 21, 2023

Uh oh!

amogh-jahagirdar commented Jan 21, 2023

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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 3, 2023

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!

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!

jackye1995 commented Feb 3, 2023

Uh oh!

Uh oh!

aokolnychyi commented Feb 4, 2023

Uh oh!

hililiwei commented Feb 6, 2023

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

yyanyy left a comment

Choose a reason for hiding this comment

Uh oh!

jackye1995 commented Feb 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hililiwei commented Jan 21, 2023 •

edited

Loading

amogh-jahagirdar commented Feb 2, 2023 •

edited

Loading