Skip to content

Conversation

@imback82
Copy link
Contributor

What changes were proposed in this pull request?

This PR proposes to migrate ALTER TABLE ... SET [SERDE|SERDEPROPERTIES 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 ... SET [SERDE|SERDEPROPERTIES] 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 SET SERDE 'serdename'") // works fine

, but after this PR:

sql("ALTER TABLE t SET SERDE 'serdename'")
org.apache.spark.sql.AnalysisException: t is a temp view. 'ALTER TABLE ... SET [SERDE|SERDEPROPERTIES\' 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, t in the above example is resolved to a temp view 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 17, 2020
@SparkQA
Copy link

SparkQA commented Dec 17, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 17, 2020

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

override def visitSetTableSerDe(ctx: SetTableSerDeContext): LogicalPlan = withOrigin(ctx) {
AlterTableSerDePropertiesStatement(
visitMultipartIdentifier(ctx.multipartIdentifier),
AlterTableSerDeProperties(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

partitionSpec is not using UnresolvedPartitionSpec since v2 command is not supported.

@imback82
Copy link
Contributor Author

cc @cloud-fan

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 0c19497 Dec 17, 2020
@SparkQA
Copy link

SparkQA commented Dec 17, 2020

Test build #132911 has finished for PR 30813 at commit b59a7d7.

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

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants