Skip to content

[SPARK-29592][SQL] ALTER TABLE (set partition location) should look up catalog/table like v2 commands #26304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
imback82 wants to merge 3 commits into from
Closed

[SPARK-29592][SQL] ALTER TABLE (set partition location) should look up catalog/table like v2 commands #26304

imback82 wants to merge 3 commits into from

Conversation

@imback82
Copy link
Contributor

@imback82 imback82 commented Oct 30, 2019

What changes were proposed in this pull request?

Update AlterTableSetLocationStatement to store partitionSpec and make ALTER TABLE a.b.c PARTITION(...) SET LOCATION 'loc' fail if partitionSpec is set with unsupported message.

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 PARTITION(...) SET LOCATION 'loc' // report set location with partition spec is not supported.

Does this PR introduce any user-facing change?

yes. When running ALTER TABLE (set partition location), 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?

New unit tests

@imback82
Copy link
Contributor Author

imback82 commented Oct 30, 2019

cc: @cloud-fan @rdblue @viirya

imback82
imback82 commented Oct 30, 2019
View reviewed changes
sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
| ALTER TABLE multipartIdentifier SET locationSpec #setTableLocation
| ALTER TABLE tableIdentifier partitionSpec SET locationSpec #setPartitionLocation
| ALTER TABLE multipartIdentifier
(partitionSpec)? SET locationSpec #setTableLocation
Copy link
Contributor Author

@imback82 imback82 Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Please let me know if this needs to be parsed separately.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala
*/
case class AlterTableSetLocationStatement(
tableName: Seq[String],
partitionSpec: Option[TablePartitionSpec],
Copy link
Contributor Author

@imback82 imback82 Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I am piggybacking on the existing AlterTableSetLocationStatement (after looking at how AlterTableSetLocationCommand is doing). Please let me know if this is not what we want and I will create a separate statement for partition.

@SparkQA
Copy link

SparkQA commented Oct 30, 2019

Test build #112881 has finished for PR 26304 at commit 400525e.

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

@viirya
Copy link
Member

viirya commented Oct 30, 2019

seems a related test failure.

@cloud-fan
Copy link
Contributor

cloud-fan commented Oct 30, 2019

LGTM if tests pass

imback82
imback82 commented Oct 30, 2019
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala
// before resolving the relations.
assertAnalysisExceptionThrown(
s"ALTER TABLE $viewName PARTITION (a='4') SET LOCATION '/path/to/home'",
"ALTER TABLE SET LOCATION does not support partition for v2 tables")
Copy link
Contributor Author

@imback82 imback82 Oct 30, 2019

Choose a reason for hiding this comment

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

@cloud-fan, If we need to retain the same behavior to throw an exception with "'$viewName' is a view not a table" message, we need to pass partitionSpec to AlterTable, then check if partitionSpec is set inside AlterTableExec. I don't think it's necessary, but please let me know if that is preferred.

Copy link
Contributor

@cloud-fan cloud-fan Oct 30, 2019

Choose a reason for hiding this comment

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

looks reasonable to me

Copy link
Contributor Author

@imback82 imback82 Oct 30, 2019

Choose a reason for hiding this comment

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

I assume you meant the current approach looks reasonable, right?

Copy link
Contributor

@cloud-fan cloud-fan Oct 30, 2019

Choose a reason for hiding this comment

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

yea, the unsupported feature should be reported first.

@huaxingao huaxingao mentioned this pull request Oct 30, 2019
Closed
@SparkQA
Copy link

SparkQA commented Oct 30, 2019

Test build #112932 has finished for PR 26304 at commit bc76fdf.

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

@SparkQA
Copy link

SparkQA commented Oct 30, 2019

Test build #112942 has finished for PR 26304 at commit 1f59072.

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

@cloud-fan cloud-fan closed this in 3a06c12 Oct 31, 2019
@cloud-fan
Copy link
Contributor

cloud-fan commented Oct 31, 2019

thanks, merging to master!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@imback82 @SparkQA @viirya @cloud-fan @dongjoon-hyun