Skip to content

Spark 3.3: SQL Extensions for DROP TAG - #6807

Merged
jackye1995 merged 4 commits into
apache:masterfrom
hililiwei:drop_tag
Feb 14, 2023
Merged

Spark 3.3: SQL Extensions for DROP TAG#6807
jackye1995 merged 4 commits into
apache:masterfrom
hililiwei:drop_tag

Conversation

@hililiwei

@hililiweihililiwei commented Feb 11, 2023

Copy link
Copy Markdown
Contributor

Co-authored-by: Amogh Jahagirdar jahamogh@amazon.com
Co-authored-by: chidayong 247070443@qq.com

@hililiwei@amogh-jahagirdarchidayong
Spark 3.3: SQL Extensions for DROP TAG
65aec46
Co-authored-by: Amogh Jahagirdar <jahamogh@amazon.com>
Co-authored-by: chidayong <247070443@qq.com>
amogh-jahagirdar
amogh-jahagirdar reviewed Feb 13, 2023
View reviewed changes

@amogh-jahagirdaramogh-jahagirdar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly looks good to me thanks @hililiwei ! Just some test cases I think we can clean up

Comment threadspark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestTagDDL.java
Assert.assertEquals(first, ref.snapshotId());
}

@Test

@amogh-jahagirdaramogh-jahagirdarFeb 13, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should also have a test to ensure that DROP TAG does not work when the ref is a branch. If I missed the opposite case for DROP BRANCH, I think let's add that as well for completeness.

Comment threadspark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestTagDDL.java
Comment on lines +281 to +285
AssertHelpers.assertThrows(
"Non-conforming tag name",
IcebergParseException.class,
"mismatched input '123'",
() -> sql("ALTER TABLE %s DROP TAG %s", tableName, "123"));

@amogh-jahagirdaramogh-jahagirdarFeb 13, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's fine to include this test but could we separate it and have this test just focused on the happy case?

@jackye1995jackye1995Feb 13, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

jackye1995
jackye1995 reviewed Feb 13, 2023
View reviewed changes
Comment threadspark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestTagDDL.java Outdated
sql("ALTER TABLE %s DROP TAG %s", tableName, tagName);
table.refresh();
ref = table.refs().get(tagName);
Assert.assertNull(ref);

@jackye1995jackye1995Feb 13, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: we should prefer to add a description for all the assertions, there are many assertEquals and assertNull here that lack description

@jackye1995

jackye1995 commented Feb 13, 2023

Copy link
Copy Markdown
Contributor

Overall looks good to me, just some nit comments about the tests

singhpk234
singhpk234 reviewed Feb 14, 2023
View reviewed changes
Comment thread....3/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/DropTag.scala Outdated
Comment thread...sions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4
Comment on lines 78 to +79
| ALTER TABLE multipartIdentifier DROP BRANCH (IF EXISTS)? identifier #dropBranch
| ALTER TABLE multipartIdentifier DROP TAG (IF EXISTS)? identifier #dropTag

@singhpk234singhpk234Feb 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can this be written like :

Suggested change
| ALTERTABLE multipartIdentifier DROPBRANCH (IFEXISTS)? identifier #dropBranch
| ALTERTABLE multipartIdentifier DROPTAG (IFEXISTS)? identifier #dropTag
| ALTERTABLE multipartIdentifier DROP (BRANCH | TAG) (IFEXISTS)? identifier #dropBranchOrTag

@jackye1995jackye1995Feb 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we discussed this in #6637 (comment), and the conclusion was that it is more clear to have them separated.

The current way for separated DROP BRANCH and DROP TAG makes the logic consistent with CREATE.

Let me know what you think!

@singhpk234singhpk234Feb 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, was mostly coming from spark code base, considering this example :

https://github.com/apache/spark/blob/46a234125d3f125ba1f9ccd6af0ec1ba61016c1e/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4#L125

If we have reached the concencus, then it should be fine.

Jack Ye@singhpk234
Update spark/v3.3/spark-extensions/src/main/scala/org/apache/spark/sq…
5249fe6
…l/catalyst/plans/logical/DropTag.scala
Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com>
@jackye1995

jackye1995 commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

Restart CI

@jackye1995jackye1995 reopened this Feb 14, 2023
jackye1995
jackye1995 approved these changes Feb 14, 2023
View reviewed changes

@jackye1995jackye1995 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good to me!

yyanyy
yyanyy approved these changes Feb 14, 2023
View reviewed changes

@yyanyyyyanyy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm!

amogh-jahagirdar
amogh-jahagirdar approved these changes Feb 14, 2023
View reviewed changes

@amogh-jahagirdaramogh-jahagirdar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me! Thanks @hililiwei for the contribution!

singhpk234
singhpk234 approved these changes Feb 14, 2023
View reviewed changes

@singhpk234singhpk234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, Thanks @hililiwei !

Comment thread...sions/src/main/antlr/org.apache.spark.sql.catalyst.parser.extensions/IcebergSqlExtensions.g4
Comment on lines 78 to +79
| ALTER TABLE multipartIdentifier DROP BRANCH (IF EXISTS)? identifier #dropBranch
| ALTER TABLE multipartIdentifier DROP TAG (IF EXISTS)? identifier #dropTag

@singhpk234singhpk234Feb 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, was mostly coming from spark code base, considering this example :

https://github.com/apache/spark/blob/46a234125d3f125ba1f9ccd6af0ec1ba61016c1e/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4#L125

If we have reached the concencus, then it should be fine.

Jack Ye@amogh-jahagirdar
Update spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/s…
7c211da
…park/extensions/TestBranchDDL.java
Co-authored-by: Amogh Jahagirdar <jahamogh@amazon.com>
@jackye1995

jackye1995 commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

Looks like we have all comments addressed, I will go ahead to merge the PR. Thanks for the contribution @hililiwei , and thanks for everyone's review!

@jackye1995
jackye1995 merged commit 72adc7c into apache:masterFeb 14, 2023
@hililiwei

hililiwei commented Feb 15, 2023

Copy link
Copy Markdown
ContributorAuthor

Thank you all for review 😄

krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023
@hililiwei@amogh-jahagirdar
chidayong@krvikash
Spark 3.3: SQL Extensions for DROP TAG (apache#6807)
dd5f750
Co-authored-by: Amogh Jahagirdar <jahamogh@amazon.com>
Co-authored-by: chidayong <247070443@qq.com>
@dramaticllydramaticlly mentioned this pull request May 19, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amogh-jahagirdaramogh-jahagirdaramogh-jahagirdar approved these changes
@jackye1995jackye1995jackye1995 approved these changes
@singhpk234singhpk234singhpk234 approved these changes
@yyanyyyyanyyyyanyy approved these changes

Assignees

No one assigned

Labels

spark

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hililiwei@jackye1995@singhpk234@yyanyy@amogh-jahagirdar