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

viirya · 2019-10-31T08:13:14Z

What changes were proposed in this pull request?

This patch removes v1 ALTER TABLE CHANGE COLUMN syntax.

Why are the changes needed?

Since in v2 we have ALTER TABLE CHANGE COLUMN and ALTER TABLE RENAME COLUMN, this old syntax is not necessary now and can be confusing.

The v2 ALTER TABLE CHANGE COLUMN should fallback to v1 AlterTableChangeColumnCommand (#26354).

Does this PR introduce any user-facing change?

Yes, the old v1 ALTER TABLE CHANGE COLUMN syntax is removed.

How was this patch tested?

Unit tests.

viirya · 2019-10-31T08:15:01Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -369,6 +369,13 @@ class ResolveSessionCatalog(
      AlterTableRecoverPartitionsCommand(
        v1TableName.asTableIdentifier,
        "ALTER TABLE RECOVER PARTITIONS")
+
+    case AlterTableChangeColumnStatement(tableName, columnName, newColumn) =>
+      val v1TableName = parseV1Table(tableName, "ALTER TABLE CHANGE COLUMN")


Seems this command can also work on V2, if old column name and new column name is the same. cc @cloud-fan

cloud-fan · 2019-10-31T08:18:13Z

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

@@ -151,7 +151,7 @@ statement
    | ALTER TABLE multipartIdentifier
        (ALTER | CHANGE) COLUMN? qualifiedName
        (TYPE dataType)? (COMMENT comment=STRING)? colPosition?        #alterTableColumn
-    | ALTER TABLE tableIdentifier partitionSpec?
+    | ALTER TABLE multipartIdentifier partitionSpec?


hmm, the parser rule above seems to include this one?

the above one is for v2 command. two rules have a bit difference. v1 rule can specify new column name with data type (i.e., colType), but v2 rule can only specify data type.

I did not combine two rules and two statements. A possible combine might have few Option fields and a logic to interpret it to v1/v2 cases, a bit mess it sounds like.

The v2 rule

(ALTER | CHANGE) COLUMN? qualifiedName (TYPE dataType)? (COMMENT comment=STRING)?

The colType

colName=errorCapturingIdentifier dataType (COMMENT STRING)?

Seems like the v1 rule is more powerful and can rename a column and change data type together. If other DBs support it as well, maybe Spark should also support it. Otherwise, maybe 3.0 is a good time to drop this syntax.

Seems we do not really support it, though we allow such syntax:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

Lines 343 to 351 in 7aca0dd

// Find the origin column from dataSchema by column name.

val originColumn = findColumnByName(table.dataSchema, columnName, resolver)

// Throw an AnalysisException if the column name/dataType is changed.

if (!columnEqual(originColumn, newColumn, resolver)) {

throw new AnalysisException(

"ALTER TABLE CHANGE COLUMN is not supported for changing column " +

s"'${originColumn.name}' with type '${originColumn.dataType}' to " +

s"'${newColumn.name}' with type '${newColumn.dataType}'")

}

Not sure what if the original plan was to support that in the future.

Let's simply remove this syntax. We have ALTER TABLE CHANGE COLUMN and ALTER TABLE RENAME COLUMN, which is good enough to me.

ok. let's remove it.

SparkQA · 2019-10-31T12:06:09Z

Test build #113006 has finished for PR 26338 at commit 91c8e58.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterTableChangeColumnStatement(

SparkQA · 2019-11-02T06:15:15Z

Test build #113122 has finished for PR 26338 at commit a432da7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterTableRenamePartitionStatement(
case class AlterTableDropPartitionStatement(
case class OptimizeLocalShuffleReader(conf: SQLConf) extends Rule[SparkPlan]

SparkQA · 2019-11-02T06:23:28Z

Test build #113123 has finished for PR 26338 at commit 05f0584.

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

cloud-fan · 2019-11-04T07:05:21Z

sql/core/src/test/resources/sql-tests/inputs/change-column.sql

@@ -2,54 +2,38 @@
 CREATE TABLE test_change(a INT, b STRING, c INT) using parquet;
 DESC test_change;

-- Change column name (not supported yet)


do we have a test file for RENAME COLUMN? If not can we add some tests here to keep the test coverage?

SparkQA · 2019-11-04T07:41:17Z

Test build #113192 has finished for PR 26338 at commit 17a34c2.

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

SparkQA · 2019-11-05T06:21:02Z

Test build #113238 has finished for PR 26338 at commit 9e7a678.

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

SparkQA · 2019-11-05T06:21:07Z

Test build #113237 has finished for PR 26338 at commit fb7564a.

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

SparkQA · 2019-11-05T07:33:52Z

Test build #113240 has finished for PR 26338 at commit ac76822.

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

SparkQA · 2019-11-05T08:05:01Z

Test build #113245 has finished for PR 26338 at commit 64a68fc.

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

viirya · 2019-11-05T08:09:41Z

retest this please.

SparkQA · 2019-11-05T10:02:11Z

Test build #113249 has finished for PR 26338 at commit 64a68fc.

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

viirya · 2019-11-05T16:14:00Z

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

@@ -149,11 +149,8 @@ statement
    | ALTER (TABLE | VIEW) multipartIdentifier
        UNSET TBLPROPERTIES (IF EXISTS)? tablePropertyList             #unsetTableProperties
    | ALTER TABLE multipartIdentifier
-        (ALTER | CHANGE) COLUMN? qualifiedName
+        (ALTER | CHANGE) COLUMN? multipartIdentifier


Previously we use qualifiedName: identifier ('.' identifier)* to capture column name.

This conflicts a test in ErrorParserSuite that test-col is not allowed in ALTER TABLE t CHANGE COLUMN test-col TYPE BIGINT.

The column name should be multiple errorCapturingIdentifier. So I changed it to multipartIdentifier:

multipartIdentifier : parts+=errorCapturingIdentifier ('.' parts+=errorCapturingIdentifier)* ;

@cloud-fan

it looks like we should replace qualifiedName with multipartIdentifier in all other places. We can do it in a followup.

ok. will make a followup later.

SparkQA · 2019-11-05T19:57:28Z

Test build #113275 has finished for PR 26338 at commit b2acddb.

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

cloud-fan · 2019-11-06T02:26:05Z

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

@@ -149,11 +149,8 @@ statement
    | ALTER (TABLE | VIEW) multipartIdentifier
        UNSET TBLPROPERTIES (IF EXISTS)? tablePropertyList             #unsetTableProperties
    | ALTER TABLE multipartIdentifier


we can distinguish the two multipartIdentifier

ALTER TABLE table=multipartIdentifier ... COLUMN? column=multipartIdentifier

You can fix it in your followup

cloud-fan · 2019-11-06T02:37:31Z

thanks, merging to master!

### What changes were proposed in this pull request? Revert #26338 , as the syntax is actually the [hive style ALTER COLUMN](https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-ChangeColumnName/Type/Position/Comment). 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 Closes #27076 from cloud-fan/alter. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Revert apache/spark#26338 , as the syntax is actually the [hive style ALTER COLUMN](https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-ChangeColumnName/Type/Position/Comment). 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 to not break hive compatibility. no, as the removal was merged in 3.0. new parser tests Closes #27076 from cloud-fan/alter. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

ALTER TABLE CHANGE COLUMN should do multi-catalog resolution.

91c8e58

viirya commented Oct 31, 2019

View reviewed changes

cloud-fan reviewed Oct 31, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into SPARK-29680

a432da7

viirya changed the title ~~[SPARK-29680][SQL] ALTER TABLE CHANGE COLUMN should look up catalog/table like v2 commands~~ [SPARK-29680][SQL] Remove ALTER TABLE CHANGE COLUMN syntax Nov 2, 2019

Remove ALTER TABLE CHANGE COLUMN syntax.

05f0584

viirya force-pushed the SPARK-29680 branch from 61ce02a to 05f0584 Compare November 2, 2019 05:58

dongjoon-hyun added the SQL label Nov 3, 2019

Fix test case.

17a34c2

cloud-fan reviewed Nov 4, 2019

View reviewed changes

viirya added 3 commits November 4, 2019 20:10

Merge remote-tracking branch 'upstream/master' into SPARK-29680

dade338

Generate new test result.

fb7564a

Remove unnecessary sql.

9e7a678

cloud-fan approved these changes Nov 5, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into SPARK-29680

ac76822

Fix missing test.

64a68fc

qualified column name should be multipartIdentifier.

b2acddb

viirya commented Nov 5, 2019

View reviewed changes

cloud-fan reviewed Nov 6, 2019

View reviewed changes

cloud-fan closed this in 6233958 Nov 6, 2019

cloud-fan mentioned this pull request Jan 2, 2020

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

Closed

viirya deleted the SPARK-29680 branch December 27, 2023 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

viirya commented Oct 31, 2019 •

edited

Loading

viirya Oct 31, 2019

cloud-fan Oct 31, 2019

viirya Oct 31, 2019

viirya Oct 31, 2019

cloud-fan Nov 1, 2019

viirya Nov 1, 2019

viirya Nov 1, 2019 •

edited

Loading

cloud-fan Nov 1, 2019

viirya Nov 2, 2019

SparkQA commented Oct 31, 2019

SparkQA commented Nov 2, 2019

SparkQA commented Nov 2, 2019

cloud-fan Nov 4, 2019

viirya Nov 4, 2019

SparkQA commented Nov 4, 2019

SparkQA commented Nov 5, 2019

SparkQA commented Nov 5, 2019

SparkQA commented Nov 5, 2019

SparkQA commented Nov 5, 2019

viirya commented Nov 5, 2019

SparkQA commented Nov 5, 2019

viirya Nov 5, 2019

cloud-fan Nov 5, 2019

viirya Nov 5, 2019

SparkQA commented Nov 5, 2019

cloud-fan Nov 6, 2019

viirya Nov 6, 2019

cloud-fan commented Nov 6, 2019

	// Find the origin column from dataSchema by column name.
	val originColumn = findColumnByName(table.dataSchema, columnName, resolver)
	// Throw an AnalysisException if the column name/dataType is changed.
	if (!columnEqual(originColumn, newColumn, resolver)) {
	throw new AnalysisException(
	"ALTER TABLE CHANGE COLUMN is not supported for changing column " +
	s"'${originColumn.name}' with type '${originColumn.dataType}' to " +
	s"'${newColumn.name}' with type '${newColumn.dataType}'")
	}

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

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

Conversation

viirya commented Oct 31, 2019 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Nov 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 31, 2019

SparkQA commented Nov 2, 2019

SparkQA commented Nov 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 4, 2019

SparkQA commented Nov 5, 2019

SparkQA commented Nov 5, 2019

SparkQA commented Nov 5, 2019

SparkQA commented Nov 5, 2019

viirya commented Nov 5, 2019

SparkQA commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Nov 6, 2019

viirya commented Oct 31, 2019 •

edited

Loading

viirya Nov 1, 2019 •

edited

Loading