Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ statement
| ALTER TABLE multipartIdentifier

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can distinguish the two multipartIdentifier

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

You can fix it in your followup

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok.

(ALTER | CHANGE) COLUMN? qualifiedName
(TYPE dataType)? (COMMENT comment=STRING)? colPosition? #alterTableColumn
| ALTER TABLE tableIdentifier partitionSpec?
| ALTER TABLE multipartIdentifier partitionSpec?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

// 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}'")
}

@viirya viirya Nov 1, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok. let's remove it.

CHANGE COLUMN?
colName=errorCapturingIdentifier colType colPosition? #changeColumn
| ALTER TABLE tableIdentifier (partitionSpec)?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2951,4 +2951,31 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
ctx: RecoverPartitionsContext): LogicalPlan = withOrigin(ctx) {
AlterTableRecoverPartitionsStatement(visitMultipartIdentifier(ctx.multipartIdentifier))
}

/**
* Create a [[AlterTableChangeColumnStatement]] command.
*
* For example:
* {{{
* ALTER TABLE multi_part_name [PARTITION partition_spec]
* CHANGE [COLUMN] column_old_name column_new_name column_dataType [COMMENT column_comment]
* [FIRST | AFTER column_name];
* }}}
*/
override def visitChangeColumn(ctx: ChangeColumnContext): LogicalPlan = withOrigin(ctx) {
if (ctx.partitionSpec != null) {
operationNotAllowed("ALTER TABLE table PARTITION partition_spec CHANGE COLUMN", ctx)
}

if (ctx.colPosition != null) {
operationNotAllowed(
"ALTER TABLE table [PARTITION partition_spec] CHANGE COLUMN ... FIRST | AFTER otherCol",
ctx)
}

AlterTableChangeColumnStatement(
visitMultipartIdentifier(ctx.multipartIdentifier),
ctx.colName.getText,
visitColType(ctx.colType))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import org.apache.spark.sql.catalyst.catalog.BucketSpec
import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec
import org.apache.spark.sql.catalyst.expressions.{Attribute, Expression}
import org.apache.spark.sql.connector.expressions.Transform
import org.apache.spark.sql.types.{DataType, StructType}
import org.apache.spark.sql.types.{DataType, StructField, StructType}

/**
* A logical plan node that contains exactly what was parsed from SQL.
Expand Down Expand Up @@ -136,7 +136,7 @@ case class AlterTableAddColumnsStatement(
columnsToAdd: Seq[QualifiedColType]) extends ParsedStatement

/**
* ALTER TABLE ... CHANGE COLUMN command, as parsed from SQL.
* ALTER TABLE ... ALTER COLUMN command, as parsed from SQL.
*/
case class AlterTableAlterColumnStatement(
tableName: Seq[String],
Expand Down Expand Up @@ -188,6 +188,14 @@ case class AlterTableSetLocationStatement(
case class AlterTableRecoverPartitionsStatement(
tableName: Seq[String]) extends ParsedStatement

/**
* ALTER TABLE ... CHANGE COLUMN command, as parsed from SQL.
*/
case class AlterTableChangeColumnStatement(
tableName: Seq[String],
columnName: String,
newColumn: StructField) extends ParsedStatement

/**
* ALTER VIEW ... SET TBLPROPERTIES command, as parsed from SQL.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import org.apache.spark.sql.catalyst.catalog.BucketSpec
import org.apache.spark.sql.catalyst.expressions.{EqualTo, Literal}
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.connector.expressions.{ApplyTransform, BucketTransform, DaysTransform, FieldReference, HoursTransform, IdentityTransform, LiteralValue, MonthsTransform, Transform, YearsTransform}
import org.apache.spark.sql.types.{IntegerType, LongType, StringType, StructType, TimestampType}
import org.apache.spark.sql.types.{IntegerType, LongType, StringType, StructField, StructType, TimestampType}
import org.apache.spark.unsafe.types.UTF8String

class DDLParserSuite extends AnalysisTest {
Expand Down Expand Up @@ -601,11 +601,47 @@ class DDLParserSuite extends AnalysisTest {
Some("new comment")))
}

test("alter table: change column position (not supported)") {
test("alter table: alter column position (not supported)") {
assertUnsupported("ALTER TABLE table_name CHANGE COLUMN name COMMENT 'doc' FIRST")
assertUnsupported("ALTER TABLE table_name CHANGE COLUMN name TYPE INT AFTER other_col")
}

test("alter table: change column name/type/comment") {
val sql1 = "ALTER TABLE table_name CHANGE COLUMN col_old_name col_new_name INT"
val sql2 = "ALTER TABLE table_name CHANGE COLUMN col_name col_name INT COMMENT 'new_comment'"
val sql3 = "ALTER TABLE a.b.c CHANGE COLUMN col_name col_name INT COMMENT 'new_comment'"

val parsed1 = parsePlan(sql1)
val parsed2 = parsePlan(sql2)
val parsed3 = parsePlan(sql3)

val expected1 = AlterTableChangeColumnStatement(
Seq("table_name"),
"col_old_name",
StructField("col_new_name", IntegerType))
val expected2 = AlterTableChangeColumnStatement(
Seq("table_name"),
"col_name",
StructField("col_name", IntegerType).withComment("new_comment"))
val expected3 = AlterTableChangeColumnStatement(
Seq("a", "b", "c"),
"col_name",
StructField("col_name", IntegerType).withComment("new_comment"))
comparePlans(parsed1, expected1)
comparePlans(parsed2, expected2)
comparePlans(parsed3, expected3)
}

test("alter table: change column position (not supported)") {
assertUnsupported("ALTER TABLE table_name CHANGE COLUMN col_old_name col_new_name INT FIRST")
assertUnsupported(
"ALTER TABLE table_name CHANGE COLUMN col_old_name col_new_name INT AFTER other_col")
}

test("alter table: change column in partition spec") {
assertUnsupported("ALTER TABLE table_name PARTITION (a='1', a='2') CHANGE COLUMN a new_a INT")
}

test("alter table: drop column") {
comparePlans(
parsePlan("ALTER TABLE table_name DROP COLUMN a.b.c"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin, LookupCatalog, TableChange, V1Table}
import org.apache.spark.sql.connector.expressions.Transform
import org.apache.spark.sql.execution.command.{AlterTableAddColumnsCommand, AlterTableRecoverPartitionsCommand, AlterTableSetLocationCommand, AlterTableSetPropertiesCommand, AlterTableUnsetPropertiesCommand, AnalyzeColumnCommand, AnalyzePartitionCommand, AnalyzeTableCommand, CacheTableCommand, CreateDatabaseCommand, DescribeColumnCommand, DescribeTableCommand, DropDatabaseCommand, DropTableCommand, LoadDataCommand, ShowColumnsCommand, ShowCreateTableCommand, ShowPartitionsCommand, ShowTablesCommand, TruncateTableCommand, UncacheTableCommand}
import org.apache.spark.sql.execution.command.{AlterTableAddColumnsCommand, AlterTableChangeColumnCommand, AlterTableRecoverPartitionsCommand, AlterTableSetLocationCommand, AlterTableSetPropertiesCommand, AlterTableUnsetPropertiesCommand, AnalyzeColumnCommand, AnalyzePartitionCommand, AnalyzeTableCommand, CacheTableCommand, CreateDatabaseCommand, DescribeColumnCommand, DescribeTableCommand, DropDatabaseCommand, DropTableCommand, LoadDataCommand, ShowColumnsCommand, ShowCreateTableCommand, ShowPartitionsCommand, ShowTablesCommand, TruncateTableCommand, UncacheTableCommand}
import org.apache.spark.sql.execution.datasources.{CreateTable, DataSource, RefreshTable}
import org.apache.spark.sql.execution.datasources.v2.FileDataSourceV2
import org.apache.spark.sql.internal.SQLConf
Expand Down Expand Up @@ -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")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

AlterTableChangeColumnCommand(
v1TableName.asTableIdentifier,
columnName,
newColumn)
}

private def parseV1Table(tableName: Seq[String], sql: String): Seq[String] = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,33 +502,6 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
retainData = false)
}

/**
* Create a [[AlterTableChangeColumnCommand]] command.
*
* For example:
* {{{
* ALTER TABLE table [PARTITION partition_spec]
* CHANGE [COLUMN] column_old_name column_new_name column_dataType [COMMENT column_comment]
* [FIRST | AFTER column_name];
* }}}
*/
override def visitChangeColumn(ctx: ChangeColumnContext): LogicalPlan = withOrigin(ctx) {
if (ctx.partitionSpec != null) {
operationNotAllowed("ALTER TABLE table PARTITION partition_spec CHANGE COLUMN", ctx)
}

if (ctx.colPosition != null) {
operationNotAllowed(
"ALTER TABLE table [PARTITION partition_spec] CHANGE COLUMN ... FIRST | AFTER otherCol",
ctx)
}

AlterTableChangeColumnCommand(
tableName = visitTableIdentifier(ctx.tableIdentifier),
columnName = ctx.colName.getText,
newColumn = visitColType(ctx.colType))
}

/**
* Convert a nested constants list into a sequence of string sequences.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1374,16 +1374,32 @@ class DataSourceV2SQLSuite
}
}

test("ALTER TABLE RECOVER PARTITIONS") {
test("ALTER TABLE RECOVER PARTITIONS") {
val t = "testcat.ns1.ns2.tbl"
withTable(t) {
spark.sql(s"CREATE TABLE $t (id bigint, data string) USING foo")
val e = intercept[AnalysisException] {
val partition = sql(s"ALTER TABLE $t RECOVER PARTITIONS")
sql(s"ALTER TABLE $t RECOVER PARTITIONS")
}
assert(e.message.contains("ALTER TABLE RECOVER PARTITIONS is only supported with v1 tables"))
}
}
}

test("ALTER TABLE CHANGE COLUMN") {
val t = "testcat.ns1.ns2.tbl"
withTable(t) {
spark.sql(s"CREATE TABLE $t (id bigint, data string) USING foo")
val e1 = intercept[AnalysisException] {
sql(s"ALTER TABLE $t CHANGE COLUMN id id_new INT")
}
assert(e1.message.contains("ALTER TABLE CHANGE COLUMN is only supported with v1 tables"))

val e2 = intercept[AnalysisException] {
sql(s"ALTER TABLE $t CHANGE COLUMN id id_new INT COMMENT 'new_comment'")
}
assert(e2.message.contains("ALTER TABLE CHANGE COLUMN is only supported with v1 tables"))
}
}

private def testV1Command(sqlCommand: String, sqlParams: String): Unit = {
val e = intercept[AnalysisException] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -635,34 +635,6 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession {
"SET FILEFORMAT PARQUET")
}

test("alter table: change column name/type/comment") {
val sql1 = "ALTER TABLE table_name CHANGE COLUMN col_old_name col_new_name INT"
val sql2 = "ALTER TABLE table_name CHANGE COLUMN col_name col_name INT COMMENT 'new_comment'"
val parsed1 = parser.parsePlan(sql1)
val parsed2 = parser.parsePlan(sql2)
val tableIdent = TableIdentifier("table_name", None)
val expected1 = AlterTableChangeColumnCommand(
tableIdent,
"col_old_name",
StructField("col_new_name", IntegerType))
val expected2 = AlterTableChangeColumnCommand(
tableIdent,
"col_name",
StructField("col_name", IntegerType).withComment("new_comment"))
comparePlans(parsed1, expected1)
comparePlans(parsed2, expected2)
}

test("alter table: change column position (not supported)") {
assertUnsupported("ALTER TABLE table_name CHANGE COLUMN col_old_name col_new_name INT FIRST")
assertUnsupported(
"ALTER TABLE table_name CHANGE COLUMN col_old_name col_new_name INT AFTER other_col")
}

test("alter table: change column in partition spec") {
assertUnsupported("ALTER TABLE table_name PARTITION (a='1', a='2') CHANGE COLUMN a new_a INT")
}

test("alter table: touch (not supported)") {
assertUnsupported("ALTER TABLE table_name TOUCH")
assertUnsupported("ALTER TABLE table_name TOUCH PARTITION (dt='2008-08-08', country='us')")
Expand Down