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.

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

revert [SPARK-29680][SQL] Remove ALTER TABLE CHANGE COLUMN syntax #27076

Closed
wants to merge 1 commit into from

Conversation

cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Jan 2, 2020

What changes were proposed in this pull request?

Revert #26338 , as the syntax is actually the hive style ALTER COLUMN.

This PR brings it back, and make it support multi-catalog:

  1. renaming is not allowed as AlterTableAlterColumnStatement can't do renaming.
  2. column name should be multi-part

Why are the changes needed?

to not break hive compatibility.

Does this PR introduce any user-facing change?

no, as the removal was merged in 3.0.

How was this patch tested?

new parser tests

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Jan 2, 2020

cc @viirya @imback82

@SparkQA
Copy link

SparkQA commented Jan 2, 2020

Test build #116036 has finished for PR 27076 at commit e8cd533.

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

@@ -161,6 +161,9 @@ statement
| ALTER TABLE table=multipartIdentifier
(ALTER | CHANGE) COLUMN? column=multipartIdentifier
(TYPE dataType)? (COMMENT comment=STRING)? colPosition? #alterTableColumn
| ALTER TABLE table=multipartIdentifier partitionSpec?
CHANGE COLUMN?
colName=multipartIdentifier colType colPosition? #hiveChangeColumn
Copy link
Member

Choose a reason for hiding this comment

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

colName was errorCapturingIdentifier before. We want it to be multipartIdentifier?

Copy link
Member

Choose a reason for hiding this comment

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

Though it is fine because ResolveSessionCatalog will detect and throw AnalysisException("ALTER COLUMN with qualified column is only supported with v2 tables.").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to be consistent with the other ALTER COLUMN syntax.

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

I see. So this is for hive compatibility, looks good to me.

@cloud-fan
Copy link
Contributor Author

thanks for review, merging to master!

@cloud-fan cloud-fan closed this in 68260f5 Jan 2, 2020
Copy link
Contributor

@imback82 imback82 left a comment

Choose a reason for hiding this comment

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

late +1

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.

5 participants