[SPARK-29643][SQL] ALTER TABLE/VIEW (DROP PARTITION) should look up catalog/table like v2 commands #26303

huaxingao · 2019-10-30T03:41:49Z

What changes were proposed in this pull request?

Add AlterTableDropPartitionStatement and make ALTER TABLE/VIEW ... DROP PARTITION go through the same catalog/table resolution framework of v2 commands.

Why are the changes needed?

It's important to make all the commands have the same table resolution behavior, to avoid confusing end-users. e.g.

USE my_catalog
DESC t // success and describe the table t from my_catalog
ALTER TABLE t DROP PARTITION (id=1)  // report table not found as there is no table t in the session catalog

Does this PR introduce any user-facing change?

Yes. When running ALTER TABLE/VIEW ... DROP PARTITION, Spark fails the command if the current catalog is set to a v2 catalog, or the table name specified a v2 catalog.

How was this patch tested?

Unit tests.

SparkQA · 2019-10-30T07:05:02Z

Test build #112880 has finished for PR 26303 at commit ea3c66c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2019-10-30T17:08:44Z

@viirya @imback82 Could you please review? Do I need to follow the implementation in #26304?

imback82 · 2019-10-30T17:17:38Z

This command doesn't need to follow #26041.

+@cloud-fan

imback82 · 2019-10-30T17:18:59Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

-    | ALTER TABLE tableIdentifier
+    | ALTER TABLE  multipartIdentifier
        DROP (IF EXISTS)? partitionSpec (',' partitionSpec)* PURGE?    #dropTablePartitions
    | ALTER VIEW tableIdentifier


Since view is not supported, can we just combine this with table as (TABLE | VIEW)? @cloud-fan can confirm.

Can we check if the feature sets are the same between table and view? I know some ALTER TABLE commands have a view version in the parser, but not sure if it applies to all ALTER TABLE commands.

If there are only a few ALTER VIEW parser rules, let's keep them separated.

ALTER TABLE ... DROP PARTITION has an optional PURGE in the end, but ALTER TABLE doesn't. I guess keep them separated? They use the same parser rule, though.

cloud-fan · 2019-10-31T02:53:32Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

    | ALTER TABLE tableIdentifier
        from=partitionSpec RENAME TO to=partitionSpec                  #renameTablePartition
-    | ALTER TABLE tableIdentifier
+    | ALTER TABLE  multipartIdentifier


nit: one space not two

cloud-fan · 2019-10-31T02:56:25Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

+    | ALTER TABLE  multipartIdentifier
        DROP (IF EXISTS)? partitionSpec (',' partitionSpec)* PURGE?    #dropTablePartitions
    | ALTER VIEW tableIdentifier
        DROP (IF EXISTS)? partitionSpec (',' partitionSpec)*           #dropTablePartitions


Sorry I missed it. These 2 parser rules share the same label, so they are essentially one rule.
I agree with @imback82 that it's better to merge them:

ALTER TABLE | VIEW multipartIdentifier DROP (IF EXISTS)? partitionSpec (',' partitionSpec)* PURGE?

viirya · 2019-10-31T06:14:40Z

Please also update the PR title (adding VIEW).

SparkQA · 2019-10-31T07:05:02Z

Test build #112992 has finished for PR 26303 at commit 12a8ad6.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-10-31T07:17:44Z

retest this please

SparkQA · 2019-10-31T11:22:53Z

Test build #113004 has finished for PR 26303 at commit 12a8ad6.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-10-31T11:50:36Z

retest this please

SparkQA · 2019-10-31T15:58:39Z

Test build #113026 has finished for PR 26303 at commit 12a8ad6.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2019-10-31T16:53:49Z

retest this please

viirya · 2019-10-31T17:50:32Z

retest this please.

SparkQA · 2019-10-31T21:37:27Z

Test build #113040 has finished for PR 26303 at commit 12a8ad6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-01T01:36:23Z

Test build #113049 has finished for PR 26303 at commit faa2361.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…g/table like v2 commands

SparkQA · 2019-11-01T10:10:37Z

Test build #113076 has finished for PR 26303 at commit 07dbe47.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-11-01T10:29:18Z

thanks, merging to master!

huaxingao · 2019-11-01T17:07:03Z

Thanks! @cloud-fan @viirya @imback82

dongjoon-hyun added the SQL label Oct 30, 2019

imback82 reviewed Oct 30, 2019

View reviewed changes

cloud-fan reviewed Oct 31, 2019

View reviewed changes

huaxingao force-pushed the spark-29643 branch from ea3c66c to 163e448 Compare October 31, 2019 04:41

viirya approved these changes Oct 31, 2019

View reviewed changes

huaxingao changed the title ~~[SPARK-29643][SQL] ALTER TABLE (DROP PARTITION) should look up catalog/table like v2 commands~~ [SPARK-29643][SQL] ALTER TABLE/VIEW (DROP PARTITION) should look up catalog/table like v2 commands Oct 31, 2019

cloud-fan approved these changes Oct 31, 2019

View reviewed changes

imback82 approved these changes Oct 31, 2019

View reviewed changes

huaxingao added 5 commits October 31, 2019 23:15

[SPARK-29643][SQL] ALTER TABLE (DROP PARTITION) should look up catalo…

650b74f

…g/table like v2 commands

add a space

4fb2527

address comments

29eaeea

remove alter view drop partition

e563ea0

address comments

07dbe47

huaxingao force-pushed the spark-29643 branch from faa2361 to 07dbe47 Compare November 1, 2019 06:19

cloud-fan closed this in 14337f6 Nov 1, 2019

huaxingao deleted the spark-29643 branch November 1, 2019 17:07

[SPARK-29643][SQL] ALTER TABLE/VIEW (DROP PARTITION) should look up catalog/table like v2 commands #26303

[SPARK-29643][SQL] ALTER TABLE/VIEW (DROP PARTITION) should look up catalog/table like v2 commands #26303

Uh oh!

Conversation

huaxingao commented Oct 30, 2019 • edited by HyukjinKwon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

huaxingao commented Oct 30, 2019

Uh oh!

imback82 commented Oct 30, 2019

Uh oh!

imback82 Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Oct 30, 2019 • 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!

cloud-fan Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

huaxingao Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

viirya commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

cloud-fan commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

cloud-fan commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

huaxingao commented Oct 31, 2019

Uh oh!

viirya commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

cloud-fan commented Nov 1, 2019

Uh oh!

huaxingao commented Nov 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

huaxingao commented Oct 30, 2019 •

edited by HyukjinKwon

Loading

cloud-fan Oct 30, 2019 •

edited

Loading