From 62c161e477650fa1d6ca86d3a7b16820f5babfcc Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Mon, 27 May 2019 14:59:08 -0700 Subject: [PATCH 1/8] Add ALTER TABLE statements to Catalyst. --- .../spark/sql/catalyst/parser/SqlBase.g4 | 40 +++- .../sql/catalyst/parser/AstBuilder.scala | 142 +++++++++++- .../logical/sql/AlterTableStatements.scala | 78 +++++++ .../logical/sql/AlterViewStatements.scala | 33 +++ .../logical/sql/CreateTableStatement.scala | 10 +- .../plans/logical/sql/ParsedStatement.scala | 5 + .../sql/catalyst/parser/DDLParserSuite.scala | 213 +++++++++++++++++- .../spark/sql/execution/SparkSqlParser.scala | 60 +---- .../datasources/DataSourceResolution.scala | 35 ++- .../execution/command/DDLParserSuite.scala | 83 +------ .../command/PlanResolutionSuite.scala | 76 +++++++ 11 files changed, 619 insertions(+), 156 deletions(-) create mode 100644 sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterTableStatements.scala create mode 100644 sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterViewStatements.scala diff --git a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 index 91beb5e639af7..a7e699961e72e 100644 --- a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 +++ b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 @@ -113,14 +113,29 @@ statement LIKE source=tableIdentifier locationSpec? #createTableLike | ANALYZE TABLE tableIdentifier partitionSpec? COMPUTE STATISTICS (identifier | FOR COLUMNS identifierSeq | FOR ALL COLUMNS)? #analyze - | ALTER TABLE tableIdentifier - ADD COLUMNS '(' columns=colTypeList ')' #addTableColumns + | ALTER TABLE multipartIdentifier + ADD (COLUMN | COLUMNS) columns=newColumnList colPosition? #addTableColumns + | ALTER TABLE multipartIdentifier + ADD (COLUMN | COLUMNS) + '(' columns=newColumnList ')' colPosition? #addTableColumns + | ALTER TABLE multipartIdentifier + RENAME COLUMN from=qualifiedName TO to=identifier #renameTableColumn + | ALTER TABLE multipartIdentifier + DROP (COLUMN | COLUMNS) '(' columns=qualifiedNameList ')' #dropTableColumns + | ALTER TABLE multipartIdentifier + DROP (COLUMN | COLUMNS) columns=qualifiedNameList #dropTableColumns | ALTER (TABLE | VIEW) from=tableIdentifier RENAME TO to=tableIdentifier #renameTable - | ALTER (TABLE | VIEW) tableIdentifier + | ALTER (TABLE | VIEW) multipartIdentifier SET TBLPROPERTIES tablePropertyList #setTableProperties - | ALTER (TABLE | VIEW) tableIdentifier + | ALTER (TABLE | VIEW) multipartIdentifier UNSET TBLPROPERTIES (IF EXISTS)? tablePropertyList #unsetTableProperties + | ALTER TABLE multipartIdentifier + (ALTER | CHANGE) COLUMN? qualifiedName + TYPE dataType (COMMENT comment=STRING)? colPosition? #alterTableColumn + | ALTER TABLE multipartIdentifier + (ALTER | CHANGE) COLUMN? qualifiedName + COMMENT comment=STRING colPosition? #alterTableColumn | ALTER TABLE tableIdentifier partitionSpec? CHANGE COLUMN? identifier colType colPosition? #changeColumn | ALTER TABLE tableIdentifier (partitionSpec)? @@ -137,7 +152,8 @@ statement DROP (IF EXISTS)? partitionSpec (',' partitionSpec)* PURGE? #dropTablePartitions | ALTER VIEW tableIdentifier DROP (IF EXISTS)? partitionSpec (',' partitionSpec)* #dropTablePartitions - | ALTER TABLE tableIdentifier partitionSpec? SET locationSpec #setTableLocation + | ALTER TABLE multipartIdentifier SET locationSpec #setTableLocation + | ALTER TABLE tableIdentifier partitionSpec SET locationSpec #setPartitionLocation | ALTER TABLE tableIdentifier RECOVER PARTITIONS #recoverPartitions | DROP TABLE (IF EXISTS)? multipartIdentifier PURGE? #dropTable | DROP VIEW (IF EXISTS)? multipartIdentifier #dropView @@ -730,6 +746,14 @@ dataType | identifier ('(' INTEGER_VALUE (',' INTEGER_VALUE)* ')')? #primitiveDataType ; +newColumnList + : newColumn (',' newColumn)* + ; + +newColumn + : name=qualifiedName dataType (COMMENT comment=STRING)? + ; + colTypeList : colType (',' colType)* ; @@ -782,6 +806,10 @@ frameBound | expression boundType=(PRECEDING | FOLLOWING) ; +qualifiedNameList + : qualifiedName (',' qualifiedName)* + ; + qualifiedName : identifier ('.' identifier)* ; @@ -1244,6 +1272,7 @@ nonReserved | TRANSFORM | TRUE | TRUNCATE + | TYPE | UNARCHIVE | UNBOUNDED | UNCACHE @@ -1499,6 +1528,7 @@ TRANSACTIONS: 'TRANSACTIONS'; TRANSFORM: 'TRANSFORM'; TRUE: 'TRUE'; TRUNCATE: 'TRUNCATE'; +TYPE: 'TYPE'; UNARCHIVE: 'UNARCHIVE'; UNBOUNDED: 'UNBOUNDED'; UNCACHE: 'UNCACHE'; diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index fa05efebf9c6d..fd6c52f8457c8 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -38,7 +38,7 @@ import org.apache.spark.sql.catalyst.expressions.aggregate.{First, Last} import org.apache.spark.sql.catalyst.parser.SqlBaseParser._ import org.apache.spark.sql.catalyst.plans._ import org.apache.spark.sql.catalyst.plans.logical._ -import org.apache.spark.sql.catalyst.plans.logical.sql.{CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement} +import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableAlterColumnStatement, AlterTableDropColumnsStatement, AlterTableRenameColumnStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement, NewColumn} import org.apache.spark.sql.catalyst.util.DateTimeUtils.{getZoneId, stringToDate, stringToTimestamp} import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types._ @@ -2213,4 +2213,144 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging visitMultipartIdentifier(ctx.multipartIdentifier()), ctx.EXISTS != null) } + + /** + * Parse new column info from ADD COLUMN into a NewColumn. + */ + override def visitNewColumn(ctx: NewColumnContext): AnyRef = withOrigin(ctx) { + NewColumn( + ctx.name.identifier.asScala.map(_.getText), + typedVisit[DataType](ctx.dataType), + Option(ctx.comment).map(string)) + } + + /** + * Parse a [[AlterTableAddColumnsStatement]] command. + * + * For example: + * {{{ + * ALTER TABLE table1 + * ADD COLUMNS (col_name data_type [COMMENT col_comment], ...); + * }}} + */ + override def visitAddTableColumns(ctx: AddTableColumnsContext): LogicalPlan = withOrigin(ctx) { + if (ctx.colPosition != null) { + operationNotAllowed("ALTER TABLE table ADD COLUMN ... FIRST | AFTER otherCol", ctx) + } + + AlterTableAddColumnsStatement( + visitMultipartIdentifier(ctx.multipartIdentifier), + ctx.columns.newColumn.asScala.map(typedVisit[NewColumn]) + ) + } + + /** + * Parse a [[AlterTableRenameColumnStatement]] command. + * + * For example: + * {{{ + * ALTER TABLE table1 RENAME COLUMN a.b.c TO x + * }}} + */ + override def visitRenameTableColumn(ctx: RenameTableColumnContext): AnyRef = withOrigin(ctx) { + AlterTableRenameColumnStatement( + visitMultipartIdentifier(ctx.multipartIdentifier), + ctx.from.identifier.asScala.map(_.getText), + ctx.to.getText) + } + + /** + * Parse a [[AlterTableAlterColumnStatement]] command. + * + * For example: + * {{{ + * ALTER TABLE table1 ALTER COLUMN a.b.c TYPE bigint + * ALTER TABLE table1 ALTER COLUMN a.b.c TYPE bigint COMMENT 'new comment' + * ALTER TABLE table1 ALTER COLUMN a.b.c COMMENT 'new comment' + * }}} + */ + override def visitAlterTableColumn( + ctx: AlterTableColumnContext): AnyRef = withOrigin(ctx) { + if (ctx.colPosition != null) { + operationNotAllowed("ALTER TABLE table CHANGE COLUMN ... FIRST | AFTER otherCol", ctx) + } + + AlterTableAlterColumnStatement( + visitMultipartIdentifier(ctx.multipartIdentifier), + ctx.qualifiedName.identifier.asScala.map(_.getText), + Option(ctx.dataType).map(typedVisit[DataType]), + Option(ctx.comment).map(string)) + } + + /** + * Parse a [[AlterTableAlterColumnStatement]] command. + * + * For example: + * {{{ + * ALTER TABLE table1 DROP COLUMN a.b.c + * ALTER TABLE table1 DROP COLUMNS a.b.c, x, y + * }}} + */ + override def visitDropTableColumns( + ctx: DropTableColumnsContext): AnyRef = withOrigin(ctx) { + val columnsToDrop = ctx.columns.qualifiedName.asScala.map(_.identifier.asScala.map(_.getText)) + AlterTableDropColumnsStatement( + visitMultipartIdentifier(ctx.multipartIdentifier), + columnsToDrop) + } + + /** + * Parse [[AlterViewSetPropertiesStatement]] or [[AlterTableSetPropertiesStatement]] commands. + * + * For example: + * {{{ + * ALTER TABLE table SET TBLPROPERTIES ('comment' = new_comment); + * ALTER VIEW view SET TBLPROPERTIES ('comment' = new_comment); + * }}} + */ + override def visitSetTableProperties( + ctx: SetTablePropertiesContext): LogicalPlan = withOrigin(ctx) { + val identifier = visitMultipartIdentifier(ctx.multipartIdentifier) + val properties = visitPropertyKeyValues(ctx.tablePropertyList) + if (ctx.VIEW != null) { + AlterViewSetPropertiesStatement(identifier, properties) + } else { + AlterTableSetPropertiesStatement(identifier, properties) + } + } + + /** + * Parse [[AlterViewUnsetPropertiesStatement]] or [[AlterTableUnsetPropertiesStatement]] commands. + * + * For example: + * {{{ + * ALTER TABLE table UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); + * ALTER VIEW view UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); + * }}} + */ + override def visitUnsetTableProperties( + ctx: UnsetTablePropertiesContext): LogicalPlan = withOrigin(ctx) { + val identifier = visitMultipartIdentifier(ctx.multipartIdentifier) + val properties = visitPropertyKeys(ctx.tablePropertyList) + val ifExists = ctx.EXISTS != null + if (ctx.VIEW != null) { + AlterViewUnsetPropertiesStatement(identifier, properties, ifExists) + } else { + AlterTableUnsetPropertiesStatement(identifier, properties, ifExists) + } + } + + /** + * Create an [[AlterTableSetLocationStatement]] command. + * + * For example: + * {{{ + * ALTER TABLE table SET LOCATION "loc"; + * }}} + */ + override def visitSetTableLocation(ctx: SetTableLocationContext): LogicalPlan = withOrigin(ctx) { + AlterTableSetLocationStatement( + visitMultipartIdentifier(ctx.multipartIdentifier), + visitLocationSpec(ctx.locationSpec)) + } } diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterTableStatements.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterTableStatements.scala new file mode 100644 index 0000000000000..efe4ea2e3f83b --- /dev/null +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterTableStatements.scala @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.catalyst.plans.logical.sql + +import org.apache.spark.sql.types.DataType + +/** + * Column data as parsed by ALTER TABLE ... ADD COLUMNS. + */ +case class NewColumn(name: Seq[String], dataType: DataType, comment: Option[String]) + +/** + * ALTER TABLE ... ADD COLUMNS command, as parsed from SQL. + */ +case class AlterTableAddColumnsStatement( + tableName: Seq[String], + columnsToAdd: Seq[NewColumn]) extends ParsedStatement + +/** + * ALTER TABLE ... CHANGE COLUMN command, as parsed from SQL. + */ +case class AlterTableAlterColumnStatement( + tableName: Seq[String], + column: Seq[String], + dataType: Option[DataType], + comment: Option[String]) extends ParsedStatement + +/** + * ALTER TABLE ... RENAME COLUMN command, as parsed from SQL. + */ +case class AlterTableRenameColumnStatement( + tableName: Seq[String], + column: Seq[String], + newName: String) extends ParsedStatement + +/** + * ALTER TABLE ... DROP COLUMNS command, as parsed from SQL. + */ +case class AlterTableDropColumnsStatement( + tableName: Seq[String], + columnsToDrop: Seq[Seq[String]]) extends ParsedStatement + +/** + * ALTER TABLE ... SET TBLPROPERTIES command, as parsed from SQL. + */ +case class AlterTableSetPropertiesStatement( + tableName: Seq[String], + properties: Map[String, String]) extends ParsedStatement + +/** + * ALTER TABLE ... UNSET TBLPROPERTIES command, as parsed from SQL. + */ +case class AlterTableUnsetPropertiesStatement( + tableName: Seq[String], + propertyKeys: Seq[String], + ifExists: Boolean) extends ParsedStatement + +/** + * ALTER TABLE ... SET LOCATION command, as parsed from SQL. + */ +case class AlterTableSetLocationStatement( + tableName: Seq[String], + location: String) extends ParsedStatement diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterViewStatements.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterViewStatements.scala new file mode 100644 index 0000000000000..bba7f12c94e50 --- /dev/null +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterViewStatements.scala @@ -0,0 +1,33 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.catalyst.plans.logical.sql + +/** + * ALTER VIEW ... SET TBLPROPERTIES command, as parsed from SQL. + */ +case class AlterViewSetPropertiesStatement( + viewName: Seq[String], + properties: Map[String, String]) extends ParsedStatement + +/** + * ALTER VIEW ... UNSET TBLPROPERTIES command, as parsed from SQL. + */ +case class AlterViewUnsetPropertiesStatement( + viewName: Seq[String], + propertyKeys: Seq[String], + ifExists: Boolean) extends ParsedStatement diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/CreateTableStatement.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/CreateTableStatement.scala index 7a26e01cde830..190711303e32d 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/CreateTableStatement.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/CreateTableStatement.scala @@ -19,7 +19,6 @@ package org.apache.spark.sql.catalyst.plans.logical.sql import org.apache.spark.sql.catalog.v2.expressions.Transform import org.apache.spark.sql.catalyst.catalog.BucketSpec -import org.apache.spark.sql.catalyst.expressions.Attribute import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan import org.apache.spark.sql.types.StructType @@ -38,12 +37,7 @@ case class CreateTableStatement( options: Map[String, String], location: Option[String], comment: Option[String], - ifNotExists: Boolean) extends ParsedStatement { - - override def output: Seq[Attribute] = Seq.empty - - override def children: Seq[LogicalPlan] = Seq.empty -} + ifNotExists: Boolean) extends ParsedStatement /** * A CREATE TABLE AS SELECT command, as parsed from SQL. @@ -60,7 +54,5 @@ case class CreateTableAsSelectStatement( comment: Option[String], ifNotExists: Boolean) extends ParsedStatement { - override def output: Seq[Attribute] = Seq.empty - override def children: Seq[LogicalPlan] = Seq(asSelect) } diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/ParsedStatement.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/ParsedStatement.scala index 510f2a1ba1e6d..2942c4b1fcca5 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/ParsedStatement.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/ParsedStatement.scala @@ -17,6 +17,7 @@ package org.apache.spark.sql.catalyst.plans.logical.sql +import org.apache.spark.sql.catalyst.expressions.Attribute import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan /** @@ -40,5 +41,9 @@ private[sql] abstract class ParsedStatement extends LogicalPlan { case other => other } + override def output: Seq[Attribute] = Seq.empty + + override def children: Seq[LogicalPlan] = Seq.empty + final override lazy val resolved = false } diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala index 06cdd067dc288..539db229dbfa0 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala @@ -17,17 +17,29 @@ package org.apache.spark.sql.catalyst.parser +import java.util.Locale + import org.apache.spark.sql.catalog.v2.expressions.{ApplyTransform, BucketTransform, DaysTransform, FieldReference, HoursTransform, IdentityTransform, LiteralValue, MonthsTransform, YearsTransform} import org.apache.spark.sql.catalyst.analysis.AnalysisTest import org.apache.spark.sql.catalyst.catalog.BucketSpec import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan -import org.apache.spark.sql.catalyst.plans.logical.sql.{CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement} -import org.apache.spark.sql.types.{IntegerType, StringType, StructType, TimestampType} +import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableAlterColumnStatement, AlterTableDropColumnsStatement, AlterTableRenameColumnStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement, NewColumn} +import org.apache.spark.sql.types.{IntegerType, LongType, StringType, StructType, TimestampType} import org.apache.spark.unsafe.types.UTF8String class DDLParserSuite extends AnalysisTest { import CatalystSqlParser._ + private def assertUnsupported(sql: String, containsThesePhrases: Seq[String] = Seq()): Unit = { + val e = intercept[ParseException] { + parsePlan(sql) + } + assert(e.getMessage.toLowerCase(Locale.ROOT).contains("operation not allowed")) + containsThesePhrases.foreach { p => + assert(e.getMessage.toLowerCase(Locale.ROOT).contains(p.toLowerCase(Locale.ROOT))) + } + } + private def intercept(sqlCommand: String, messages: String*): Unit = interceptParseException(parsePlan)(sqlCommand, messages: _*) @@ -390,4 +402,201 @@ class DDLParserSuite extends AnalysisTest { parseCompare(s"DROP VIEW view", DropViewStatement(Seq("view"), ifExists = false)) parseCompare(s"DROP VIEW IF EXISTS view", DropViewStatement(Seq("view"), ifExists = true)) } + + // ALTER VIEW view_name SET TBLPROPERTIES ('comment' = new_comment); + // ALTER VIEW view_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); + test("alter view: alter view properties") { + val sql1_view = "ALTER VIEW table_name SET TBLPROPERTIES ('test' = 'test', " + + "'comment' = 'new_comment')" + val sql2_view = "ALTER VIEW table_name UNSET TBLPROPERTIES ('comment', 'test')" + val sql3_view = "ALTER VIEW table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" + + val parsed1_view = parsePlan(sql1_view) + val parsed2_view = parsePlan(sql2_view) + val parsed3_view = parsePlan(sql3_view) + + val expected1_view = AlterViewSetPropertiesStatement( + Seq("table_name"), Map("test" -> "test", "comment" -> "new_comment")) + val expected2_view = AlterViewUnsetPropertiesStatement( + Seq("table_name"), Seq("comment", "test"), ifExists = false) + val expected3_view = AlterViewUnsetPropertiesStatement( + Seq("table_name"), Seq("comment", "test"), ifExists = true) + + comparePlans(parsed1_view, expected1_view) + comparePlans(parsed2_view, expected2_view) + comparePlans(parsed3_view, expected3_view) + } + + // ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment); + // ALTER TABLE table_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); + test("alter table: alter table properties") { + val sql1_table = "ALTER TABLE table_name SET TBLPROPERTIES ('test' = 'test', " + + "'comment' = 'new_comment')" + val sql2_table = "ALTER TABLE table_name UNSET TBLPROPERTIES ('comment', 'test')" + val sql3_table = "ALTER TABLE table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" + + val parsed1_table = parsePlan(sql1_table) + val parsed2_table = parsePlan(sql2_table) + val parsed3_table = parsePlan(sql3_table) + + val expected1_table = AlterTableSetPropertiesStatement( + Seq("table_name"), Map("test" -> "test", "comment" -> "new_comment")) + val expected2_table = AlterTableUnsetPropertiesStatement( + Seq("table_name"), Seq("comment", "test"), ifExists = false) + val expected3_table = AlterTableUnsetPropertiesStatement( + Seq("table_name"), Seq("comment", "test"), ifExists = true) + + comparePlans(parsed1_table, expected1_table) + comparePlans(parsed2_table, expected2_table) + comparePlans(parsed3_table, expected3_table) + } + + test("alter table: add column") { + comparePlans( + parsePlan("ALTER TABLE table_name ADD COLUMN x int"), + AlterTableAddColumnsStatement(Seq("table_name"), Seq( + NewColumn(Seq("x"), IntegerType, None) + ))) + } + + test("alter table: add multiple columns") { + comparePlans( + parsePlan("ALTER TABLE table_name ADD COLUMNS x int, y string"), + AlterTableAddColumnsStatement(Seq("table_name"), Seq( + NewColumn(Seq("x"), IntegerType, None), + NewColumn(Seq("y"), StringType, None) + ))) + } + + test("alter table: add column with COLUMNS") { + comparePlans( + parsePlan("ALTER TABLE table_name ADD COLUMNS x int"), + AlterTableAddColumnsStatement(Seq("table_name"), Seq( + NewColumn(Seq("x"), IntegerType, None) + ))) + } + + test("alter table: add column with COLUMNS (...)") { + comparePlans( + parsePlan("ALTER TABLE table_name ADD COLUMNS (x int)"), + AlterTableAddColumnsStatement(Seq("table_name"), Seq( + NewColumn(Seq("x"), IntegerType, None) + ))) + } + + test("alter table: add column with COLUMNS (...) and COMMENT") { + comparePlans( + parsePlan("ALTER TABLE table_name ADD COLUMNS (x int COMMENT 'doc')"), + AlterTableAddColumnsStatement(Seq("table_name"), Seq( + NewColumn(Seq("x"), IntegerType, Some("doc")) + ))) + } + + test("alter table: add column with COMMENT") { + comparePlans( + parsePlan("ALTER TABLE table_name ADD COLUMN x int COMMENT 'doc'"), + AlterTableAddColumnsStatement(Seq("table_name"), Seq( + NewColumn(Seq("x"), IntegerType, Some("doc")) + ))) + } + + test("alter table: add column with nested column name") { + comparePlans( + parsePlan("ALTER TABLE table_name ADD COLUMN x.y.z int COMMENT 'doc'"), + AlterTableAddColumnsStatement(Seq("table_name"), Seq( + NewColumn(Seq("x", "y", "z"), IntegerType, Some("doc")) + ))) + } + + test("alter table: add multiple columns with nested column name") { + comparePlans( + parsePlan("ALTER TABLE table_name ADD COLUMN x.y.z int COMMENT 'doc', a.b string"), + AlterTableAddColumnsStatement(Seq("table_name"), Seq( + NewColumn(Seq("x", "y", "z"), IntegerType, Some("doc")), + NewColumn(Seq("a", "b"), StringType, None) + ))) + } + + test("alter table: add column at position (not supported)") { + assertUnsupported("ALTER TABLE table_name ADD COLUMN name bigint COMMENT 'doc' FIRST") + assertUnsupported("ALTER TABLE table_name ADD COLUMN name string AFTER other_col") + } + + test("alter table: set location") { + val sql1 = "ALTER TABLE table_name SET LOCATION 'new location'" + val parsed1 = parsePlan(sql1) + val expected1 = AlterTableSetLocationStatement(Seq("table_name"), "new location") + comparePlans(parsed1, expected1) + } + + test("alter table: rename column") { + comparePlans( + parsePlan("ALTER TABLE table_name RENAME COLUMN a.b.c TO d"), + AlterTableRenameColumnStatement( + Seq("table_name"), + Seq("a", "b", "c"), + "d")) + } + + test("alter table: update column type using ALTER") { + comparePlans( + parsePlan("ALTER TABLE table_name ALTER COLUMN a.b.c TYPE bigint"), + AlterTableAlterColumnStatement( + Seq("table_name"), + Seq("a", "b", "c"), + Some(LongType), + None)) + } + + test("alter table: update column type") { + comparePlans( + parsePlan("ALTER TABLE table_name CHANGE COLUMN a.b.c TYPE bigint"), + AlterTableAlterColumnStatement( + Seq("table_name"), + Seq("a", "b", "c"), + Some(LongType), + None)) + } + + test("alter table: update column comment") { + comparePlans( + parsePlan("ALTER TABLE table_name CHANGE COLUMN a.b.c COMMENT 'new comment'"), + AlterTableAlterColumnStatement( + Seq("table_name"), + Seq("a", "b", "c"), + None, + Some("new comment"))) + } + + test("alter table: update column type and comment") { + comparePlans( + parsePlan("ALTER TABLE table_name CHANGE COLUMN a.b.c TYPE bigint COMMENT 'new comment'"), + AlterTableAlterColumnStatement( + Seq("table_name"), + Seq("a", "b", "c"), + Some(LongType), + Some("new comment"))) + } + + test("alter table: change 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: drop column") { + comparePlans( + parsePlan("ALTER TABLE table_name DROP COLUMN a.b.c"), + AlterTableDropColumnsStatement(Seq("table_name"), Seq(Seq("a", "b", "c")))) + } + + test("alter table: drop multiple columns") { + val sql = "ALTER TABLE table_name DROP COLUMN x, y, a.b.c" + Seq(sql, sql.replace("COLUMN", "COLUMNS")).foreach { drop => + comparePlans( + parsePlan(drop), + AlterTableDropColumnsStatement( + Seq("table_name"), + Seq(Seq("x"), Seq("y"), Seq("a", "b", "c")))) + } + } } diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala index 1f1b41b7c4405..2b3a561a23c90 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala @@ -661,57 +661,6 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) { ctx.VIEW != null) } - /** - * Create a [[AlterTableAddColumnsCommand]] command. - * - * For example: - * {{{ - * ALTER TABLE table1 - * ADD COLUMNS (col_name data_type [COMMENT col_comment], ...); - * }}} - */ - override def visitAddTableColumns(ctx: AddTableColumnsContext): LogicalPlan = withOrigin(ctx) { - AlterTableAddColumnsCommand( - visitTableIdentifier(ctx.tableIdentifier), - visitColTypeList(ctx.columns) - ) - } - - /** - * Create an [[AlterTableSetPropertiesCommand]] command. - * - * For example: - * {{{ - * ALTER TABLE table SET TBLPROPERTIES ('comment' = new_comment); - * ALTER VIEW view SET TBLPROPERTIES ('comment' = new_comment); - * }}} - */ - override def visitSetTableProperties( - ctx: SetTablePropertiesContext): LogicalPlan = withOrigin(ctx) { - AlterTableSetPropertiesCommand( - visitTableIdentifier(ctx.tableIdentifier), - visitPropertyKeyValues(ctx.tablePropertyList), - ctx.VIEW != null) - } - - /** - * Create an [[AlterTableUnsetPropertiesCommand]] command. - * - * For example: - * {{{ - * ALTER TABLE table UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); - * ALTER VIEW view UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); - * }}} - */ - override def visitUnsetTableProperties( - ctx: UnsetTablePropertiesContext): LogicalPlan = withOrigin(ctx) { - AlterTableUnsetPropertiesCommand( - visitTableIdentifier(ctx.tableIdentifier), - visitPropertyKeys(ctx.tablePropertyList), - ctx.EXISTS != null, - ctx.VIEW != null) - } - /** * Create an [[AlterTableSerDePropertiesCommand]] command. * @@ -820,17 +769,18 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) { } /** - * Create an [[AlterTableSetLocationCommand]] command + * Create an [[AlterTableSetLocationCommand]] command for a partition. * * For example: * {{{ - * ALTER TABLE table [PARTITION spec] SET LOCATION "loc"; + * ALTER TABLE table PARTITION spec SET LOCATION "loc"; * }}} */ - override def visitSetTableLocation(ctx: SetTableLocationContext): LogicalPlan = withOrigin(ctx) { + override def visitSetPartitionLocation( + ctx: SetPartitionLocationContext): LogicalPlan = withOrigin(ctx) { AlterTableSetLocationCommand( visitTableIdentifier(ctx.tableIdentifier), - Option(ctx.partitionSpec).map(visitNonOptionalPartitionSpec), + Some(visitNonOptionalPartitionSpec(ctx.partitionSpec)), visitLocationSpec(ctx.locationSpec)) } diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala index 19881f69f158c..eefb95704d4c9 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala @@ -28,12 +28,12 @@ import org.apache.spark.sql.catalyst.TableIdentifier import org.apache.spark.sql.catalyst.analysis.CastSupport import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogTable, CatalogTableType, CatalogUtils} import org.apache.spark.sql.catalyst.plans.logical.{CreateTableAsSelect, CreateV2Table, DropTable, LogicalPlan} -import org.apache.spark.sql.catalyst.plans.logical.sql.{CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement} +import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement} import org.apache.spark.sql.catalyst.rules.Rule -import org.apache.spark.sql.execution.command.DropTableCommand +import org.apache.spark.sql.execution.command.{AlterTableAddColumnsCommand, AlterTableSetLocationCommand, AlterTableSetPropertiesCommand, AlterTableUnsetPropertiesCommand, DropTableCommand} import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.sources.v2.TableProvider -import org.apache.spark.sql.types.StructType +import org.apache.spark.sql.types.{Metadata, StructField, StructType} case class DataSourceResolution( conf: SQLConf, @@ -98,6 +98,35 @@ case class DataSourceResolution( case DropViewStatement(AsTableIdentifier(tableName), ifExists) => DropTableCommand(tableName, ifExists, isView = true, purge = false) + + case AlterTableSetPropertiesStatement(AsTableIdentifier(table), properties) => + AlterTableSetPropertiesCommand(table, properties, isView = false) + + case AlterViewSetPropertiesStatement(AsTableIdentifier(table), properties) => + AlterTableSetPropertiesCommand(table, properties, isView = true) + + case AlterTableUnsetPropertiesStatement(AsTableIdentifier(table), propertyKeys, ifExists) => + AlterTableUnsetPropertiesCommand(table, propertyKeys, ifExists, isView = false) + + case AlterViewUnsetPropertiesStatement(AsTableIdentifier(table), propertyKeys, ifExists) => + AlterTableUnsetPropertiesCommand(table, propertyKeys, ifExists, isView = true) + + case AlterTableSetLocationStatement(AsTableIdentifier(table), newLocation) => + AlterTableSetLocationCommand(table, None, newLocation) + + case AlterTableAddColumnsStatement(AsTableIdentifier(table), newColumns) + if newColumns.forall(_.name.size == 1) => + // only top-level adds are supported using AlterTableAddColumnsCommand + val newFields = newColumns.map { newCol => + val struct = StructField( + newCol.name.head, + newCol.dataType, + nullable = true, + Metadata.empty) + newCol.comment.map(struct.withComment).getOrElse(struct) + } + + AlterTableAddColumnsCommand(table, newFields) } object V1WriteProvider { diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala index e46ccac9f5b04..31da6fe423bab 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala @@ -36,7 +36,7 @@ import org.apache.spark.sql.execution.SparkSqlParser import org.apache.spark.sql.execution.datasources.CreateTable import org.apache.spark.sql.internal.{HiveSerDe, SQLConf} import org.apache.spark.sql.test.SharedSQLContext -import org.apache.spark.sql.types.{IntegerType, StructField, StructType} +import org.apache.spark.sql.types.StructType class DDLParserSuite extends AnalysisTest with SharedSQLContext { private lazy val parser = new SparkSqlParser(new SQLConf) @@ -522,45 +522,6 @@ class DDLParserSuite extends AnalysisTest with SharedSQLContext { assert(plan.newName == TableIdentifier("tbl2", Some("db1"))) } - // ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment); - // ALTER TABLE table_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); - // ALTER VIEW view_name SET TBLPROPERTIES ('comment' = new_comment); - // ALTER VIEW view_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); - test("alter table/view: alter table/view properties") { - val sql1_table = "ALTER TABLE table_name SET TBLPROPERTIES ('test' = 'test', " + - "'comment' = 'new_comment')" - val sql2_table = "ALTER TABLE table_name UNSET TBLPROPERTIES ('comment', 'test')" - val sql3_table = "ALTER TABLE table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" - val sql1_view = sql1_table.replace("TABLE", "VIEW") - val sql2_view = sql2_table.replace("TABLE", "VIEW") - val sql3_view = sql3_table.replace("TABLE", "VIEW") - - val parsed1_table = parser.parsePlan(sql1_table) - val parsed2_table = parser.parsePlan(sql2_table) - val parsed3_table = parser.parsePlan(sql3_table) - val parsed1_view = parser.parsePlan(sql1_view) - val parsed2_view = parser.parsePlan(sql2_view) - val parsed3_view = parser.parsePlan(sql3_view) - - val tableIdent = TableIdentifier("table_name", None) - val expected1_table = AlterTableSetPropertiesCommand( - tableIdent, Map("test" -> "test", "comment" -> "new_comment"), isView = false) - val expected2_table = AlterTableUnsetPropertiesCommand( - tableIdent, Seq("comment", "test"), ifExists = false, isView = false) - val expected3_table = AlterTableUnsetPropertiesCommand( - tableIdent, Seq("comment", "test"), ifExists = true, isView = false) - val expected1_view = expected1_table.copy(isView = true) - val expected2_view = expected2_table.copy(isView = true) - val expected3_view = expected3_table.copy(isView = true) - - comparePlans(parsed1_table, expected1_table) - comparePlans(parsed2_table, expected2_table) - comparePlans(parsed3_table, expected3_table) - comparePlans(parsed1_view, expected1_view) - comparePlans(parsed2_view, expected2_view) - comparePlans(parsed3_view, expected3_view) - } - test("alter table - property values must be set") { assertUnsupported( sql = "ALTER TABLE my_tab SET TBLPROPERTIES('key_without_value', 'key_with_value'='x')", @@ -758,40 +719,15 @@ class DDLParserSuite extends AnalysisTest with SharedSQLContext { "SET FILEFORMAT PARQUET") } - test("alter table: set location") { - val sql1 = "ALTER TABLE table_name SET LOCATION 'new location'" + test("alter table: set partition location") { val sql2 = "ALTER TABLE table_name PARTITION (dt='2008-08-08', country='us') " + "SET LOCATION 'new location'" - val parsed1 = parser.parsePlan(sql1) val parsed2 = parser.parsePlan(sql2) val tableIdent = TableIdentifier("table_name", None) - val expected1 = AlterTableSetLocationCommand( - tableIdent, - None, - "new location") val expected2 = AlterTableSetLocationCommand( tableIdent, Some(Map("dt" -> "2008-08-08", "country" -> "us")), "new location") - comparePlans(parsed1, expected1) - comparePlans(parsed2, expected2) - } - - 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) } @@ -957,21 +893,6 @@ class DDLParserSuite extends AnalysisTest with SharedSQLContext { comparePlans(parsed, expected) } - test("support for other types in TBLPROPERTIES") { - val sql = - """ - |ALTER TABLE table_name - |SET TBLPROPERTIES ('a' = 1, 'b' = 0.1, 'c' = TRUE) - """.stripMargin - val parsed = parser.parsePlan(sql) - val expected = AlterTableSetPropertiesCommand( - TableIdentifier("table_name"), - Map("a" -> "1", "b" -> "0.1", "c" -> "true"), - isView = false) - - comparePlans(parsed, expected) - } - test("Test CTAS #1") { val s1 = """ diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala index 06f7332086372..727160dafc5de 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala @@ -501,4 +501,80 @@ class PlanResolutionSuite extends AnalysisTest { }.getMessage.toLowerCase(Locale.ROOT).contains( "view support in catalog has not been implemented") } + + // ALTER VIEW view_name SET TBLPROPERTIES ('comment' = new_comment); + // ALTER VIEW view_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); + test("alter view: alter view properties") { + val sql1_view = "ALTER VIEW table_name SET TBLPROPERTIES ('test' = 'test', " + + "'comment' = 'new_comment')" + val sql2_view = "ALTER VIEW table_name UNSET TBLPROPERTIES ('comment', 'test')" + val sql3_view = "ALTER VIEW table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" + + val parsed1_view = parseAndResolve(sql1_view) + val parsed2_view = parseAndResolve(sql2_view) + val parsed3_view = parseAndResolve(sql3_view) + + val tableIdent = TableIdentifier("table_name", None) + val expected1_view = AlterTableSetPropertiesCommand( + tableIdent, Map("test" -> "test", "comment" -> "new_comment"), isView = true) + val expected2_view = AlterTableUnsetPropertiesCommand( + tableIdent, Seq("comment", "test"), ifExists = false, isView = true) + val expected3_view = AlterTableUnsetPropertiesCommand( + tableIdent, Seq("comment", "test"), ifExists = true, isView = true) + + comparePlans(parsed1_view, expected1_view) + comparePlans(parsed2_view, expected2_view) + comparePlans(parsed3_view, expected3_view) + } + + // ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment); + // ALTER TABLE table_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); + test("alter table: alter table properties") { + val sql1_table = "ALTER TABLE table_name SET TBLPROPERTIES ('test' = 'test', " + + "'comment' = 'new_comment')" + val sql2_table = "ALTER TABLE table_name UNSET TBLPROPERTIES ('comment', 'test')" + val sql3_table = "ALTER TABLE table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" + + val parsed1_table = parseAndResolve(sql1_table) + val parsed2_table = parseAndResolve(sql2_table) + val parsed3_table = parseAndResolve(sql3_table) + + val tableIdent = TableIdentifier("table_name", None) + val expected1_table = AlterTableSetPropertiesCommand( + tableIdent, Map("test" -> "test", "comment" -> "new_comment"), isView = false) + val expected2_table = AlterTableUnsetPropertiesCommand( + tableIdent, Seq("comment", "test"), ifExists = false, isView = false) + val expected3_table = AlterTableUnsetPropertiesCommand( + tableIdent, Seq("comment", "test"), ifExists = true, isView = false) + + comparePlans(parsed1_table, expected1_table) + comparePlans(parsed2_table, expected2_table) + comparePlans(parsed3_table, expected3_table) + } + + test("support for other types in TBLPROPERTIES") { + val sql = + """ + |ALTER TABLE table_name + |SET TBLPROPERTIES ('a' = 1, 'b' = 0.1, 'c' = TRUE) + """.stripMargin + val parsed = parseAndResolve(sql) + val expected = AlterTableSetPropertiesCommand( + TableIdentifier("table_name"), + Map("a" -> "1", "b" -> "0.1", "c" -> "true"), + isView = false) + + comparePlans(parsed, expected) + } + + test("alter table: set location") { + val sql1 = "ALTER TABLE table_name SET LOCATION 'new location'" + val parsed1 = parseAndResolve(sql1) + val tableIdent = TableIdentifier("table_name", None) + val expected1 = AlterTableSetLocationCommand( + tableIdent, + None, + "new location") + comparePlans(parsed1, expected1) + } } From 98bac21602ba78d9148b97c5c94603731ba75057 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Wed, 29 May 2019 16:51:31 -0700 Subject: [PATCH 2/8] Fixup char type when converting to v1 ADD COLUMN plan. --- .../datasources/DataSourceResolution.scala | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala index eefb95704d4c9..8925a464c3528 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala @@ -33,7 +33,7 @@ import org.apache.spark.sql.catalyst.rules.Rule import org.apache.spark.sql.execution.command.{AlterTableAddColumnsCommand, AlterTableSetLocationCommand, AlterTableSetPropertiesCommand, AlterTableUnsetPropertiesCommand, DropTableCommand} import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.sources.v2.TableProvider -import org.apache.spark.sql.types.{Metadata, StructField, StructType} +import org.apache.spark.sql.types.{HIVE_TYPE_STRING, HiveStringType, Metadata, MetadataBuilder, StructField, StructType} case class DataSourceResolution( conf: SQLConf, @@ -118,12 +118,19 @@ case class DataSourceResolution( if newColumns.forall(_.name.size == 1) => // only top-level adds are supported using AlterTableAddColumnsCommand val newFields = newColumns.map { newCol => - val struct = StructField( + val builder = new MetadataBuilder + newCol.comment.foreach(builder.putString("comment", _)) + + val cleanedDataType = HiveStringType.replaceCharType(newCol.dataType) + if (newCol.dataType != cleanedDataType) { + builder.putString(HIVE_TYPE_STRING, newCol.dataType.catalogString) + } + + StructField( newCol.name.head, - newCol.dataType, + cleanedDataType, nullable = true, - Metadata.empty) - newCol.comment.map(struct.withComment).getOrElse(struct) + builder.build()) } AlterTableAddColumnsCommand(table, newFields) From d9694baf8ea3faaefc8e46de6e8b2b68da2d5222 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Fri, 31 May 2019 14:22:39 -0700 Subject: [PATCH 3/8] Update for review comments. --- .../spark/sql/catalyst/parser/SqlBase.g4 | 5 +- .../sql/catalyst/parser/AstBuilder.scala | 32 ++++++++---- .../sql/catalyst/parser/DDLParserSuite.scala | 49 ++++++++----------- 3 files changed, 44 insertions(+), 42 deletions(-) diff --git a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 index a7e699961e72e..fe4b4381de197 100644 --- a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 +++ b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 @@ -132,10 +132,7 @@ statement UNSET TBLPROPERTIES (IF EXISTS)? tablePropertyList #unsetTableProperties | ALTER TABLE multipartIdentifier (ALTER | CHANGE) COLUMN? qualifiedName - TYPE dataType (COMMENT comment=STRING)? colPosition? #alterTableColumn - | ALTER TABLE multipartIdentifier - (ALTER | CHANGE) COLUMN? qualifiedName - COMMENT comment=STRING colPosition? #alterTableColumn + (TYPE dataType)? (COMMENT comment=STRING)? colPosition? #alterTableColumn | ALTER TABLE tableIdentifier partitionSpec? CHANGE COLUMN? identifier colType colPosition? #changeColumn | ALTER TABLE tableIdentifier (partitionSpec)? diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index fd6c52f8457c8..2bb95e7feca30 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -2035,6 +2035,13 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging (multipartIdentifier, temporary, ifNotExists, ctx.EXTERNAL != null) } + /** + * Parse a qualified name to a multipart name. + */ + override def visitQualifiedName(ctx: QualifiedNameContext): Seq[String] = withOrigin(ctx) { + ctx.identifier.asScala.map(_.getText) + } + /** * Parse a list of transforms. */ @@ -2067,8 +2074,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging ctx.transforms.asScala.map { case identityCtx: IdentityTransformContext => - IdentityTransform(FieldReference( - identityCtx.qualifiedName.identifier.asScala.map(_.getText))) + IdentityTransform(FieldReference(typedVisit[Seq[String]](identityCtx.qualifiedName))) case applyCtx: ApplyTransformContext => val arguments = applyCtx.argument.asScala.map(visitTransformArgument) @@ -2115,7 +2121,8 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging override def visitTransformArgument(ctx: TransformArgumentContext): v2.expressions.Expression = { withOrigin(ctx) { val reference = Option(ctx.qualifiedName) - .map(nameCtx => FieldReference(nameCtx.identifier.asScala.map(_.getText))) + .map(typedVisit[Seq[String]]) + .map(FieldReference(_)) val literal = Option(ctx.constant) .map(typedVisit[Literal]) .map(lit => LiteralValue(lit.value, lit.dataType)) @@ -2217,7 +2224,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging /** * Parse new column info from ADD COLUMN into a NewColumn. */ - override def visitNewColumn(ctx: NewColumnContext): AnyRef = withOrigin(ctx) { + override def visitNewColumn(ctx: NewColumnContext): NewColumn = withOrigin(ctx) { NewColumn( ctx.name.identifier.asScala.map(_.getText), typedVisit[DataType](ctx.dataType), @@ -2252,7 +2259,8 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging * ALTER TABLE table1 RENAME COLUMN a.b.c TO x * }}} */ - override def visitRenameTableColumn(ctx: RenameTableColumnContext): AnyRef = withOrigin(ctx) { + override def visitRenameTableColumn( + ctx: RenameTableColumnContext): LogicalPlan = withOrigin(ctx) { AlterTableRenameColumnStatement( visitMultipartIdentifier(ctx.multipartIdentifier), ctx.from.identifier.asScala.map(_.getText), @@ -2270,20 +2278,24 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging * }}} */ override def visitAlterTableColumn( - ctx: AlterTableColumnContext): AnyRef = withOrigin(ctx) { + ctx: AlterTableColumnContext): LogicalPlan = withOrigin(ctx) { if (ctx.colPosition != null) { operationNotAllowed("ALTER TABLE table CHANGE COLUMN ... FIRST | AFTER otherCol", ctx) } + if (ctx.dataType == null && ctx.comment == null) { + operationNotAllowed("ALTER TABLE table CHANGE COLUMN requires a TYPE or a COMMENT", ctx) + } + AlterTableAlterColumnStatement( visitMultipartIdentifier(ctx.multipartIdentifier), - ctx.qualifiedName.identifier.asScala.map(_.getText), + typedVisit[Seq[String]](ctx.qualifiedName), Option(ctx.dataType).map(typedVisit[DataType]), Option(ctx.comment).map(string)) } /** - * Parse a [[AlterTableAlterColumnStatement]] command. + * Parse a [[AlterTableDropColumnsStatement]] command. * * For example: * {{{ @@ -2292,8 +2304,8 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging * }}} */ override def visitDropTableColumns( - ctx: DropTableColumnsContext): AnyRef = withOrigin(ctx) { - val columnsToDrop = ctx.columns.qualifiedName.asScala.map(_.identifier.asScala.map(_.getText)) + ctx: DropTableColumnsContext): LogicalPlan = withOrigin(ctx) { + val columnsToDrop = ctx.columns.qualifiedName.asScala.map(typedVisit[Seq[String]]) AlterTableDropColumnsStatement( visitMultipartIdentifier(ctx.multipartIdentifier), columnsToDrop) diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala index 539db229dbfa0..7b2a0d70fe233 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala @@ -411,20 +411,15 @@ class DDLParserSuite extends AnalysisTest { val sql2_view = "ALTER VIEW table_name UNSET TBLPROPERTIES ('comment', 'test')" val sql3_view = "ALTER VIEW table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" - val parsed1_view = parsePlan(sql1_view) - val parsed2_view = parsePlan(sql2_view) - val parsed3_view = parsePlan(sql3_view) - - val expected1_view = AlterViewSetPropertiesStatement( - Seq("table_name"), Map("test" -> "test", "comment" -> "new_comment")) - val expected2_view = AlterViewUnsetPropertiesStatement( - Seq("table_name"), Seq("comment", "test"), ifExists = false) - val expected3_view = AlterViewUnsetPropertiesStatement( - Seq("table_name"), Seq("comment", "test"), ifExists = true) - - comparePlans(parsed1_view, expected1_view) - comparePlans(parsed2_view, expected2_view) - comparePlans(parsed3_view, expected3_view) + comparePlans(parsePlan(sql1_view), + AlterViewSetPropertiesStatement( + Seq("table_name"), Map("test" -> "test", "comment" -> "new_comment"))) + comparePlans(parsePlan(sql2_view), + AlterViewUnsetPropertiesStatement( + Seq("table_name"), Seq("comment", "test"), ifExists = false)) + comparePlans(parsePlan(sql3_view), + AlterViewUnsetPropertiesStatement( + Seq("table_name"), Seq("comment", "test"), ifExists = true)) } // ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment); @@ -435,20 +430,18 @@ class DDLParserSuite extends AnalysisTest { val sql2_table = "ALTER TABLE table_name UNSET TBLPROPERTIES ('comment', 'test')" val sql3_table = "ALTER TABLE table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" - val parsed1_table = parsePlan(sql1_table) - val parsed2_table = parsePlan(sql2_table) - val parsed3_table = parsePlan(sql3_table) - - val expected1_table = AlterTableSetPropertiesStatement( - Seq("table_name"), Map("test" -> "test", "comment" -> "new_comment")) - val expected2_table = AlterTableUnsetPropertiesStatement( - Seq("table_name"), Seq("comment", "test"), ifExists = false) - val expected3_table = AlterTableUnsetPropertiesStatement( - Seq("table_name"), Seq("comment", "test"), ifExists = true) - - comparePlans(parsed1_table, expected1_table) - comparePlans(parsed2_table, expected2_table) - comparePlans(parsed3_table, expected3_table) + comparePlans( + parsePlan(sql1_table), + AlterTableSetPropertiesStatement( + Seq("table_name"), Map("test" -> "test", "comment" -> "new_comment"))) + comparePlans( + parsePlan(sql2_table), + AlterTableUnsetPropertiesStatement( + Seq("table_name"), Seq("comment", "test"), ifExists = false)) + comparePlans( + parsePlan(sql3_table), + AlterTableUnsetPropertiesStatement( + Seq("table_name"), Seq("comment", "test"), ifExists = true)) } test("alter table: add column") { From 5aa3b60daf054917eaa23a1aa8610b4690258aae Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Fri, 31 May 2019 17:07:09 -0700 Subject: [PATCH 4/8] Update missed qualifiedName. --- .../scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index 2bb95e7feca30..4f7223ba59f96 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -2226,7 +2226,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging */ override def visitNewColumn(ctx: NewColumnContext): NewColumn = withOrigin(ctx) { NewColumn( - ctx.name.identifier.asScala.map(_.getText), + typedVisit[Seq[String]](ctx.name), typedVisit[DataType](ctx.dataType), Option(ctx.comment).map(string)) } From 7e2369ef6f58bc83f502cfc09b9809e14c83b937 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Mon, 3 Jun 2019 09:45:55 -0700 Subject: [PATCH 5/8] Fix ADD COLUMN with column position. --- .../spark/sql/catalyst/parser/SqlBase.g4 | 15 +++++------ .../sql/catalyst/parser/AstBuilder.scala | 17 +++++++------ .../logical/sql/AlterTableStatements.scala | 4 +-- .../sql/catalyst/parser/DDLParserSuite.scala | 25 ++++++++++--------- 4 files changed, 32 insertions(+), 29 deletions(-) diff --git a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 index fe4b4381de197..d32f92291bb7d 100644 --- a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 +++ b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 @@ -114,10 +114,11 @@ statement | ANALYZE TABLE tableIdentifier partitionSpec? COMPUTE STATISTICS (identifier | FOR COLUMNS identifierSeq | FOR ALL COLUMNS)? #analyze | ALTER TABLE multipartIdentifier - ADD (COLUMN | COLUMNS) columns=newColumnList colPosition? #addTableColumns + ADD (COLUMN | COLUMNS) + columns=qualifiedColTypeWithPositionList #addTableColumns | ALTER TABLE multipartIdentifier ADD (COLUMN | COLUMNS) - '(' columns=newColumnList ')' colPosition? #addTableColumns + '(' columns=qualifiedColTypeWithPositionList ')' #addTableColumns | ALTER TABLE multipartIdentifier RENAME COLUMN from=qualifiedName TO to=identifier #renameTableColumn | ALTER TABLE multipartIdentifier @@ -733,7 +734,7 @@ intervalUnit ; colPosition - : FIRST | AFTER identifier + : FIRST | AFTER qualifiedName ; dataType @@ -743,12 +744,12 @@ dataType | identifier ('(' INTEGER_VALUE (',' INTEGER_VALUE)* ')')? #primitiveDataType ; -newColumnList - : newColumn (',' newColumn)* +qualifiedColTypeWithPositionList + : qualifiedColTypeWithPosition (',' qualifiedColTypeWithPosition)* ; -newColumn - : name=qualifiedName dataType (COMMENT comment=STRING)? +qualifiedColTypeWithPosition + : name=qualifiedName dataType (COMMENT comment=STRING)? colPosition? ; colTypeList diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index 4f7223ba59f96..f9f0773769838 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -38,7 +38,7 @@ import org.apache.spark.sql.catalyst.expressions.aggregate.{First, Last} import org.apache.spark.sql.catalyst.parser.SqlBaseParser._ import org.apache.spark.sql.catalyst.plans._ import org.apache.spark.sql.catalyst.plans.logical._ -import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableAlterColumnStatement, AlterTableDropColumnsStatement, AlterTableRenameColumnStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement, NewColumn} +import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableAlterColumnStatement, AlterTableDropColumnsStatement, AlterTableRenameColumnStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement, QualifiedColType} import org.apache.spark.sql.catalyst.util.DateTimeUtils.{getZoneId, stringToDate, stringToTimestamp} import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types._ @@ -2224,8 +2224,13 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging /** * Parse new column info from ADD COLUMN into a NewColumn. */ - override def visitNewColumn(ctx: NewColumnContext): NewColumn = withOrigin(ctx) { - NewColumn( + override def visitQualifiedColTypeWithPosition( + ctx: QualifiedColTypeWithPositionContext): QualifiedColType = withOrigin(ctx) { + if (ctx.colPosition != null) { + operationNotAllowed("ALTER TABLE table ADD COLUMN ... FIRST | AFTER otherCol", ctx) + } + + QualifiedColType( typedVisit[Seq[String]](ctx.name), typedVisit[DataType](ctx.dataType), Option(ctx.comment).map(string)) @@ -2241,13 +2246,9 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging * }}} */ override def visitAddTableColumns(ctx: AddTableColumnsContext): LogicalPlan = withOrigin(ctx) { - if (ctx.colPosition != null) { - operationNotAllowed("ALTER TABLE table ADD COLUMN ... FIRST | AFTER otherCol", ctx) - } - AlterTableAddColumnsStatement( visitMultipartIdentifier(ctx.multipartIdentifier), - ctx.columns.newColumn.asScala.map(typedVisit[NewColumn]) + ctx.columns.qualifiedColTypeWithPosition.asScala.map(typedVisit[QualifiedColType]) ) } diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterTableStatements.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterTableStatements.scala index efe4ea2e3f83b..9d7dec9ae0ce0 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterTableStatements.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/sql/AlterTableStatements.scala @@ -22,14 +22,14 @@ import org.apache.spark.sql.types.DataType /** * Column data as parsed by ALTER TABLE ... ADD COLUMNS. */ -case class NewColumn(name: Seq[String], dataType: DataType, comment: Option[String]) +case class QualifiedColType(name: Seq[String], dataType: DataType, comment: Option[String]) /** * ALTER TABLE ... ADD COLUMNS command, as parsed from SQL. */ case class AlterTableAddColumnsStatement( tableName: Seq[String], - columnsToAdd: Seq[NewColumn]) extends ParsedStatement + columnsToAdd: Seq[QualifiedColType]) extends ParsedStatement /** * ALTER TABLE ... CHANGE COLUMN command, as parsed from SQL. diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala index 7b2a0d70fe233..d008b3c78fac3 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala @@ -23,7 +23,7 @@ import org.apache.spark.sql.catalog.v2.expressions.{ApplyTransform, BucketTransf import org.apache.spark.sql.catalyst.analysis.AnalysisTest import org.apache.spark.sql.catalyst.catalog.BucketSpec import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan -import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableAlterColumnStatement, AlterTableDropColumnsStatement, AlterTableRenameColumnStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement, NewColumn} +import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableAlterColumnStatement, AlterTableDropColumnsStatement, AlterTableRenameColumnStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement, QualifiedColType} import org.apache.spark.sql.types.{IntegerType, LongType, StringType, StructType, TimestampType} import org.apache.spark.unsafe.types.UTF8String @@ -448,7 +448,7 @@ class DDLParserSuite extends AnalysisTest { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMN x int"), AlterTableAddColumnsStatement(Seq("table_name"), Seq( - NewColumn(Seq("x"), IntegerType, None) + QualifiedColType(Seq("x"), IntegerType, None) ))) } @@ -456,8 +456,8 @@ class DDLParserSuite extends AnalysisTest { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMNS x int, y string"), AlterTableAddColumnsStatement(Seq("table_name"), Seq( - NewColumn(Seq("x"), IntegerType, None), - NewColumn(Seq("y"), StringType, None) + QualifiedColType(Seq("x"), IntegerType, None), + QualifiedColType(Seq("y"), StringType, None) ))) } @@ -465,7 +465,7 @@ class DDLParserSuite extends AnalysisTest { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMNS x int"), AlterTableAddColumnsStatement(Seq("table_name"), Seq( - NewColumn(Seq("x"), IntegerType, None) + QualifiedColType(Seq("x"), IntegerType, None) ))) } @@ -473,7 +473,7 @@ class DDLParserSuite extends AnalysisTest { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMNS (x int)"), AlterTableAddColumnsStatement(Seq("table_name"), Seq( - NewColumn(Seq("x"), IntegerType, None) + QualifiedColType(Seq("x"), IntegerType, None) ))) } @@ -481,7 +481,7 @@ class DDLParserSuite extends AnalysisTest { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMNS (x int COMMENT 'doc')"), AlterTableAddColumnsStatement(Seq("table_name"), Seq( - NewColumn(Seq("x"), IntegerType, Some("doc")) + QualifiedColType(Seq("x"), IntegerType, Some("doc")) ))) } @@ -489,7 +489,7 @@ class DDLParserSuite extends AnalysisTest { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMN x int COMMENT 'doc'"), AlterTableAddColumnsStatement(Seq("table_name"), Seq( - NewColumn(Seq("x"), IntegerType, Some("doc")) + QualifiedColType(Seq("x"), IntegerType, Some("doc")) ))) } @@ -497,7 +497,7 @@ class DDLParserSuite extends AnalysisTest { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMN x.y.z int COMMENT 'doc'"), AlterTableAddColumnsStatement(Seq("table_name"), Seq( - NewColumn(Seq("x", "y", "z"), IntegerType, Some("doc")) + QualifiedColType(Seq("x", "y", "z"), IntegerType, Some("doc")) ))) } @@ -505,14 +505,15 @@ class DDLParserSuite extends AnalysisTest { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMN x.y.z int COMMENT 'doc', a.b string"), AlterTableAddColumnsStatement(Seq("table_name"), Seq( - NewColumn(Seq("x", "y", "z"), IntegerType, Some("doc")), - NewColumn(Seq("a", "b"), StringType, None) + QualifiedColType(Seq("x", "y", "z"), IntegerType, Some("doc")), + QualifiedColType(Seq("a", "b"), StringType, None) ))) } test("alter table: add column at position (not supported)") { + assertUnsupported("ALTER TABLE table_name ADD COLUMNS name bigint COMMENT 'doc' FIRST, a.b int") assertUnsupported("ALTER TABLE table_name ADD COLUMN name bigint COMMENT 'doc' FIRST") - assertUnsupported("ALTER TABLE table_name ADD COLUMN name string AFTER other_col") + assertUnsupported("ALTER TABLE table_name ADD COLUMN name string AFTER a.b") } test("alter table: set location") { From 1a3810a8b54242c9ddde0baf3ee4ce72d688e05a Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Mon, 3 Jun 2019 09:55:47 -0700 Subject: [PATCH 6/8] Add convertToStructField for QualifiedColType. --- .../datasources/DataSourceResolution.scala | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala index 8925a464c3528..26f7230c8fe8f 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceResolution.scala @@ -28,12 +28,12 @@ import org.apache.spark.sql.catalyst.TableIdentifier import org.apache.spark.sql.catalyst.analysis.CastSupport import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogTable, CatalogTableType, CatalogUtils} import org.apache.spark.sql.catalyst.plans.logical.{CreateTableAsSelect, CreateV2Table, DropTable, LogicalPlan} -import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement} +import org.apache.spark.sql.catalyst.plans.logical.sql.{AlterTableAddColumnsStatement, AlterTableSetLocationStatement, AlterTableSetPropertiesStatement, AlterTableUnsetPropertiesStatement, AlterViewSetPropertiesStatement, AlterViewUnsetPropertiesStatement, CreateTableAsSelectStatement, CreateTableStatement, DropTableStatement, DropViewStatement, QualifiedColType} import org.apache.spark.sql.catalyst.rules.Rule import org.apache.spark.sql.execution.command.{AlterTableAddColumnsCommand, AlterTableSetLocationCommand, AlterTableSetPropertiesCommand, AlterTableUnsetPropertiesCommand, DropTableCommand} import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.sources.v2.TableProvider -import org.apache.spark.sql.types.{HIVE_TYPE_STRING, HiveStringType, Metadata, MetadataBuilder, StructField, StructType} +import org.apache.spark.sql.types.{HIVE_TYPE_STRING, HiveStringType, MetadataBuilder, StructField, StructType} case class DataSourceResolution( conf: SQLConf, @@ -117,23 +117,7 @@ case class DataSourceResolution( case AlterTableAddColumnsStatement(AsTableIdentifier(table), newColumns) if newColumns.forall(_.name.size == 1) => // only top-level adds are supported using AlterTableAddColumnsCommand - val newFields = newColumns.map { newCol => - val builder = new MetadataBuilder - newCol.comment.foreach(builder.putString("comment", _)) - - val cleanedDataType = HiveStringType.replaceCharType(newCol.dataType) - if (newCol.dataType != cleanedDataType) { - builder.putString(HIVE_TYPE_STRING, newCol.dataType.catalogString) - } - - StructField( - newCol.name.head, - cleanedDataType, - nullable = true, - builder.build()) - } - - AlterTableAddColumnsCommand(table, newFields) + AlterTableAddColumnsCommand(table, newColumns.map(convertToStructField)) } object V1WriteProvider { @@ -269,4 +253,20 @@ case class DataSourceResolution( tableProperties.toMap } + + private def convertToStructField(col: QualifiedColType): StructField = { + val builder = new MetadataBuilder + col.comment.foreach(builder.putString("comment", _)) + + val cleanedDataType = HiveStringType.replaceCharType(col.dataType) + if (col.dataType != cleanedDataType) { + builder.putString(HIVE_TYPE_STRING, col.dataType.catalogString) + } + + StructField( + col.name.head, + cleanedDataType, + nullable = true, + builder.build()) + } } From c55849930c793ce72ee2c47798e2af39c05cdd43 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Mon, 3 Jun 2019 09:57:17 -0700 Subject: [PATCH 7/8] Update name. --- .../scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index f9f0773769838..288761b571844 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -2222,7 +2222,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging } /** - * Parse new column info from ADD COLUMN into a NewColumn. + * Parse new column info from ADD COLUMN into a QualifiedColType. */ override def visitQualifiedColTypeWithPosition( ctx: QualifiedColTypeWithPositionContext): QualifiedColType = withOrigin(ctx) { From bd7d8c8f7c6a2c378deba5927c96a8eb4dc6ef54 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Tue, 4 Jun 2019 12:03:35 -0700 Subject: [PATCH 8/8] Fix issues from reviews. --- .../sql/catalyst/parser/AstBuilder.scala | 5 +++-- .../execution/command/DDLParserSuite.scala | 20 ++++++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index 288761b571844..ca5a56712fce0 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -2280,12 +2280,13 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging */ override def visitAlterTableColumn( ctx: AlterTableColumnContext): LogicalPlan = withOrigin(ctx) { + val verb = if (ctx.CHANGE != null) "CHANGE" else "ALTER" if (ctx.colPosition != null) { - operationNotAllowed("ALTER TABLE table CHANGE COLUMN ... FIRST | AFTER otherCol", ctx) + operationNotAllowed(s"ALTER TABLE table $verb COLUMN ... FIRST | AFTER otherCol", ctx) } if (ctx.dataType == null && ctx.comment == null) { - operationNotAllowed("ALTER TABLE table CHANGE COLUMN requires a TYPE or a COMMENT", ctx) + operationNotAllowed(s"ALTER TABLE table $verb COLUMN requires a TYPE or a COMMENT", ctx) } AlterTableAlterColumnStatement( diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala index 31da6fe423bab..42f993e4ac10d 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala @@ -36,7 +36,7 @@ import org.apache.spark.sql.execution.SparkSqlParser import org.apache.spark.sql.execution.datasources.CreateTable import org.apache.spark.sql.internal.{HiveSerDe, SQLConf} import org.apache.spark.sql.test.SharedSQLContext -import org.apache.spark.sql.types.StructType +import org.apache.spark.sql.types.{IntegerType, StructField, StructType} class DDLParserSuite extends AnalysisTest with SharedSQLContext { private lazy val parser = new SparkSqlParser(new SQLConf) @@ -731,6 +731,24 @@ class DDLParserSuite extends AnalysisTest with SharedSQLContext { comparePlans(parsed2, expected2) } + 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(