Skip to content
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

[SPARK-33856][SQL] Migrate ALTER TABLE ... RENAME TO PARTITION to use UnresolvedTable to resolve the identifier #30862

Closed
wants to merge 2 commits into from
Closed

[SPARK-33856][SQL] Migrate ALTER TABLE ... RENAME TO PARTITION to use UnresolvedTable to resolve the identifier #30862

wants to merge 2 commits into from

Conversation

imback82
Copy link
Contributor

What changes were proposed in this pull request?

This PR proposes to migrate ALTER TABLE ... RENAME TO PARTITION to use UnresolvedTable to resolve the table identifier. This allows consistent resolution rules (temp view first, etc.) to be applied for both v1/v2 commands. More info about the consistent resolution rule proposal can be found in JIRA or proposal doc.

Note that ALTER TABLE ... RENAME TO PARTITION is not supported for v2 tables.

Why are the changes needed?

The PR makes the resolution consistent behavior consistent. For example,

sql("CREATE DATABASE test")
sql("CREATE TABLE spark_catalog.test.t (id bigint, val string) USING csv PARTITIONED BY (id)")
sql("CREATE TEMPORARY VIEW t AS SELECT 2")
sql("USE spark_catalog.test")
sql("ALTER TABLE t PARTITION (id=1) RENAME TO PARTITION (id=2)") // works fine assuming id=1 exists.

, but after this PR:

sql("ALTER TABLE t PARTITION (id=1) RENAME TO PARTITION (id=2)")
org.apache.spark.sql.AnalysisException: t is a temp view. 'ALTER TABLE ... RENAME TO PARTITION' expects a table; line 1 pos 0

, which is the consistent behavior with other commands.

Does this PR introduce any user-facing change?

After this PR, ALTER TABLE in the above example is resolved to a temp view t first instead of spark_catalog.test.t.

How was this patch tested?

Updated existing tests.

@github-actions github-actions bot added the SQL label Dec 20, 2020
@SparkQA
Copy link

SparkQA commented Dec 20, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37698/

@SparkQA
Copy link

SparkQA commented Dec 20, 2020

Test build #133098 has finished for PR 30862 at commit e2488bb.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class AlterTableRenamePartition(

@SparkQA
Copy link

SparkQA commented Dec 20, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37698/

MaxGekk
MaxGekk reviewed Dec 20, 2020
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala Outdated
@@ -445,10 +445,9 @@ class ResolveSessionCatalog(
partSpecsAndLocs.asUnresolvedPartitionSpecs.map(spec => (spec.spec, spec.location)),
ifNotExists)

case AlterTableRenamePartitionStatement(tbl, from, to) =>
val v1TableName = parseV1Table(tbl, "ALTER TABLE RENAME PARTITION")
case AlterTableRenamePartition(ResolvedV1TableIdentifier(ident), from, to) =>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder will your changes help to resolve the issue: #30863 (comment)

MaxGekk
MaxGekk reviewed Dec 20, 2020
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala Outdated
Comment on lines 681 to 682
from: TablePartitionSpec,
to: TablePartitionSpec) extends Command {
Copy link
Member

Choose a reason for hiding this comment

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

Please, use PartitionSpec

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala

Line 74 in b112e2b

sealed trait PartitionSpec
as in

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

Line 667 in 0c19497

parts: Seq[PartitionSpec],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. (I was going to introduce when implementing v2 version, but I guess it doesn't hurt to have it now).

MaxGekk
MaxGekk reviewed Dec 20, 2020
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala
@@ -1642,9 +1642,10 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils {
Set(Map("a" -> "10", "b" -> "p"), Map("a" -> "20", "b" -> "c"), Map("a" -> "3", "b" -> "p")))

// table to alter does not exist
intercept[NoSuchTableException] {
val e = intercept[AnalysisException] {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you replace NoSuchTableException ?

Copy link
Contributor Author

@imback82 imback82 Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Unresolved table is now handled here:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Lines 104 to 105 in 8e26339

case u: UnresolvedTable =>
u.failAnalysis(s"Table not found: ${u.multipartIdentifier.quoted}")

Copy link
Member

@MaxGekk MaxGekk Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

in that case, I believe we should throw NoSuchTableException here.

Copy link
Member

Choose a reason for hiding this comment

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

otherwise with such approach, you will lift all specific exception to the general one - AnalysisException.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's fix it separately.

Copy link
Contributor Author

@imback82 imback82 Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good. I will do a follow-up PR.

@SparkQA
Copy link

SparkQA commented Dec 21, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37721/

@SparkQA
Copy link

SparkQA commented Dec 21, 2020

Test build #133122 has finished for PR 30862 at commit 9894f9e.

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

@SparkQA
Copy link

SparkQA commented Dec 21, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37721/

@imback82
Copy link
Contributor Author

cc @cloud-fan

@cloud-fan
Copy link
Contributor

GA passed, merging to master!

@cloud-fan cloud-fan closed this in 1c7b79c Dec 21, 2020
@imback82 imback82 mentioned this pull request Dec 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxGekk MaxGekk MaxGekk left review comments

@cloud-fan cloud-fan cloud-fan approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@imback82 @SparkQA @cloud-fan @MaxGekk