From d47dd06d6eb2cb9cb287d28c53893c7c9fc8a992 Mon Sep 17 00:00:00 2001 From: Huaxin Gao Date: Sat, 2 Nov 2019 17:10:02 -0700 Subject: [PATCH 1/2] [SPARK-29695][SQL] ALTER TABLE (SerDe properties) should look up catalog/table like v2 commands --- .../spark/sql/catalyst/parser/SqlBase.g4 | 4 +- .../sql/catalyst/parser/AstBuilder.scala | 19 +++++ .../catalyst/plans/logical/statements.scala | 9 +++ .../sql/catalyst/parser/DDLParserSuite.scala | 78 +++++++++++++++++++ .../analysis/ResolveSessionCatalog.scala | 9 +++ .../spark/sql/execution/SparkSqlParser.scala | 18 ----- .../sql/connector/DataSourceV2SQLSuite.scala | 11 +++ .../execution/command/DDLParserSuite.scala | 55 ------------- 8 files changed, 128 insertions(+), 75 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 5b2e623680583..15573c9ce50a2 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 @@ -154,9 +154,9 @@ statement | ALTER TABLE tableIdentifier partitionSpec? CHANGE COLUMN? colName=errorCapturingIdentifier colType colPosition? #changeColumn - | ALTER TABLE tableIdentifier (partitionSpec)? + | ALTER TABLE multipartIdentifier (partitionSpec)? SET SERDE STRING (WITH SERDEPROPERTIES tablePropertyList)? #setTableSerDe - | ALTER TABLE tableIdentifier (partitionSpec)? + | ALTER TABLE multipartIdentifier (partitionSpec)? SET SERDEPROPERTIES tablePropertyList #setTableSerDe | ALTER (TABLE | VIEW) multipartIdentifier ADD (IF NOT EXISTS)? partitionSpecLocation+ #addTablePartition 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 f3e61fe31f265..39cf171829b31 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 @@ -3048,4 +3048,23 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging purge = ctx.PURGE != null, retainData = false) } + + /** + * Create an [[AlterTableSerDePropertiesStatement]] + * + * For example: + * {{{ + * ALTER TABLE multi_part_name [PARTITION spec] SET SERDE serde_name + * [WITH SERDEPROPERTIES props]; + * ALTER TABLE multi_part_name [PARTITION spec] SET SERDEPROPERTIES serde_properties; + * }}} + */ + override def visitSetTableSerDe(ctx: SetTableSerDeContext): LogicalPlan = withOrigin(ctx) { + AlterTableSerDePropertiesStatement( + visitMultipartIdentifier(ctx.multipartIdentifier), + Option(ctx.STRING).map(string), + Option(ctx.tablePropertyList).map(visitPropertyKeyValues), + // TODO a partition spec is allowed to have optional values. This is currently violated. + Option(ctx.partitionSpec).map(visitNonOptionalPartitionSpec)) + } } diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala index 79038c5184a6c..377ca2589fa8e 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala @@ -214,6 +214,15 @@ case class AlterTableDropPartitionStatement( purge: Boolean, retainData: Boolean) extends ParsedStatement +/** + * ALTER TABLE ... SERDEPROPERTIES command, as parsed from SQL + */ +case class AlterTableSerDePropertiesStatement( + tableName: Seq[String], + serdeClassName: Option[String], + serdeProperties: Option[Map[String, String]], + partitionSpec: Option[TablePartitionSpec]) extends ParsedStatement + /** * ALTER VIEW ... SET TBLPROPERTIES 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 ce6c5191fb0bc..5744d30e77e7e 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 @@ -1320,6 +1320,84 @@ class DDLParserSuite extends AnalysisTest { ShowCurrentNamespaceStatement()) } + test("alter table: SerDe properties") { + val sql1 = "ALTER TABLE table_name SET SERDE 'org.apache.class'" + val sql2 = + """ + |ALTER TABLE table_name SET SERDE 'org.apache.class' + |WITH SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val sql3 = + """ + |ALTER TABLE table_name SET SERDEPROPERTIES ('columns'='foo,bar', + |'field.delim' = ',') + """.stripMargin + val sql4 = + """ + |ALTER TABLE table_name PARTITION (test=1, dt='2008-08-08', + |country='us') SET SERDE 'org.apache.class' WITH SERDEPROPERTIES ('columns'='foo,bar', + |'field.delim' = ',') + """.stripMargin + val sql5 = + """ + |ALTER TABLE table_name PARTITION (test=1, dt='2008-08-08', + |country='us') SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val sql6 = + """ + |ALTER TABLE a.b.c SET SERDE 'org.apache.class' + |WITH SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val sql7 = + """ + |ALTER TABLE a.b.c PARTITION (test=1, dt='2008-08-08', + |country='us') SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val parsed1 = parsePlan(sql1) + val parsed2 = parsePlan(sql2) + val parsed3 = parsePlan(sql3) + val parsed4 = parsePlan(sql4) + val parsed5 = parsePlan(sql5) + val parsed6 = parsePlan(sql6) + val parsed7 = parsePlan(sql7) + val expected1 = AlterTableSerDePropertiesStatement( + Seq("table_name"), Some("org.apache.class"), None, None) + val expected2 = AlterTableSerDePropertiesStatement( + Seq("table_name"), + Some("org.apache.class"), + Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), + None) + val expected3 = AlterTableSerDePropertiesStatement( + Seq("table_name"), None, Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), None) + val expected4 = AlterTableSerDePropertiesStatement( + Seq("table_name"), + Some("org.apache.class"), + Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), + Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) + val expected5 = AlterTableSerDePropertiesStatement( + Seq("table_name"), + None, + Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), + Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) + val expected6 = AlterTableSerDePropertiesStatement( + Seq("a", "b", "c"), + Some("org.apache.class"), + Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), + None) + val expected7 = AlterTableSerDePropertiesStatement( + Seq("a", "b", "c"), + None, + Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), + Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) + comparePlans(parsed1, expected1) + comparePlans(parsed2, expected2) + comparePlans(parsed3, expected3) + comparePlans(parsed4, expected4) + comparePlans(parsed5, expected5) + comparePlans(parsed6, expected6) + comparePlans(parsed7, expected7) + } + private case class TableSpec( name: Seq[String], schema: Option[StructType], diff --git a/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala index b5c61abf610f1..9b4c8ef742494 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala @@ -412,6 +412,15 @@ class ResolveSessionCatalog( ifExists, purge, retainData) + + case AlterTableSerDePropertiesStatement( + tableName, serdeClassName, serdeProperties, partitionSpec) => + val v1TableName = parseV1Table(tableName, "ALTER TABLE SerDe Properties") + AlterTableSerDePropertiesCommand( + v1TableName.asTableIdentifier, + serdeClassName, + serdeProperties, + partitionSpec) } private def parseV1Table(tableName: Seq[String], sql: String): Seq[String] = { 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 8bbf549309ad9..6902890c5d173 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 @@ -408,24 +408,6 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) { ctx.VIEW != null) } - /** - * Create an [[AlterTableSerDePropertiesCommand]] command. - * - * For example: - * {{{ - * ALTER TABLE table [PARTITION spec] SET SERDE serde_name [WITH SERDEPROPERTIES props]; - * ALTER TABLE table [PARTITION spec] SET SERDEPROPERTIES serde_properties; - * }}} - */ - override def visitSetTableSerDe(ctx: SetTableSerDeContext): LogicalPlan = withOrigin(ctx) { - AlterTableSerDePropertiesCommand( - visitTableIdentifier(ctx.tableIdentifier), - Option(ctx.STRING).map(string), - Option(ctx.tablePropertyList).map(visitPropertyKeyValues), - // TODO a partition spec is allowed to have optional values. This is currently violated. - Option(ctx.partitionSpec).map(visitNonOptionalPartitionSpec)) - } - /** * Create a [[AlterTableChangeColumnCommand]] command. * diff --git a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala index 00b1eefa4ad93..0b507ebd89a15 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala @@ -1441,6 +1441,17 @@ class DataSourceV2SQLSuite } } + test("ALTER TABLE SerDe properties") { + val t = "testcat.ns1.ns2.tbl" + withTable(t) { + spark.sql(s"CREATE TABLE $t (id bigint, data string) USING foo PARTITIONED BY (id)") + val e = intercept[AnalysisException] { + sql(s"ALTER TABLE $t SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',')") + } + assert(e.message.contains("ALTER TABLE SerDe Properties is only supported with v1 tables")) + } + } + private def testV1Command(sqlCommand: String, sqlParams: String): Unit = { val e = intercept[AnalysisException] { sql(s"$sqlCommand $sqlParams") 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 9ea4a8cc38a6f..df81f46390e2d 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 @@ -458,61 +458,6 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession { containsThesePhrases = Seq("key_with_value")) } - test("alter table: SerDe properties") { - val sql1 = "ALTER TABLE table_name SET SERDE 'org.apache.class'" - val sql2 = - """ - |ALTER TABLE table_name SET SERDE 'org.apache.class' - |WITH SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') - """.stripMargin - val sql3 = - """ - |ALTER TABLE table_name SET SERDEPROPERTIES ('columns'='foo,bar', - |'field.delim' = ',') - """.stripMargin - val sql4 = - """ - |ALTER TABLE table_name PARTITION (test=1, dt='2008-08-08', - |country='us') SET SERDE 'org.apache.class' WITH SERDEPROPERTIES ('columns'='foo,bar', - |'field.delim' = ',') - """.stripMargin - val sql5 = - """ - |ALTER TABLE table_name PARTITION (test=1, dt='2008-08-08', - |country='us') SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') - """.stripMargin - val parsed1 = parser.parsePlan(sql1) - val parsed2 = parser.parsePlan(sql2) - val parsed3 = parser.parsePlan(sql3) - val parsed4 = parser.parsePlan(sql4) - val parsed5 = parser.parsePlan(sql5) - val tableIdent = TableIdentifier("table_name", None) - val expected1 = AlterTableSerDePropertiesCommand( - tableIdent, Some("org.apache.class"), None, None) - val expected2 = AlterTableSerDePropertiesCommand( - tableIdent, - Some("org.apache.class"), - Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), - None) - val expected3 = AlterTableSerDePropertiesCommand( - tableIdent, None, Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), None) - val expected4 = AlterTableSerDePropertiesCommand( - tableIdent, - Some("org.apache.class"), - Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), - Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) - val expected5 = AlterTableSerDePropertiesCommand( - tableIdent, - None, - Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), - Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) - comparePlans(parsed1, expected1) - comparePlans(parsed2, expected2) - comparePlans(parsed3, expected3) - comparePlans(parsed4, expected4) - comparePlans(parsed5, expected5) - } - test("alter table - SerDe property values must be set") { assertUnsupported( sql = "ALTER TABLE my_tab SET SERDE 'serde' " + From 7c2078f442bcda5ee0a22a2273a80282ea13663b Mon Sep 17 00:00:00 2001 From: Huaxin Gao Date: Mon, 4 Nov 2019 18:14:25 -0800 Subject: [PATCH 2/2] address comments --- .../sql/catalyst/parser/DDLParserSuite.scala | 86 ++++++++++--------- 1 file changed, 46 insertions(+), 40 deletions(-) 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 5744d30e77e7e..998067a9a9f39 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 @@ -1322,79 +1322,85 @@ class DDLParserSuite extends AnalysisTest { test("alter table: SerDe properties") { val sql1 = "ALTER TABLE table_name SET SERDE 'org.apache.class'" + val parsed1 = parsePlan(sql1) + val expected1 = AlterTableSerDePropertiesStatement( + Seq("table_name"), Some("org.apache.class"), None, None) + comparePlans(parsed1, expected1) + val sql2 = """ |ALTER TABLE table_name SET SERDE 'org.apache.class' |WITH SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') """.stripMargin - val sql3 = - """ - |ALTER TABLE table_name SET SERDEPROPERTIES ('columns'='foo,bar', - |'field.delim' = ',') - """.stripMargin - val sql4 = - """ - |ALTER TABLE table_name PARTITION (test=1, dt='2008-08-08', - |country='us') SET SERDE 'org.apache.class' WITH SERDEPROPERTIES ('columns'='foo,bar', - |'field.delim' = ',') - """.stripMargin - val sql5 = - """ - |ALTER TABLE table_name PARTITION (test=1, dt='2008-08-08', - |country='us') SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') - """.stripMargin - val sql6 = - """ - |ALTER TABLE a.b.c SET SERDE 'org.apache.class' - |WITH SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') - """.stripMargin - val sql7 = - """ - |ALTER TABLE a.b.c PARTITION (test=1, dt='2008-08-08', - |country='us') SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') - """.stripMargin - val parsed1 = parsePlan(sql1) val parsed2 = parsePlan(sql2) - val parsed3 = parsePlan(sql3) - val parsed4 = parsePlan(sql4) - val parsed5 = parsePlan(sql5) - val parsed6 = parsePlan(sql6) - val parsed7 = parsePlan(sql7) - val expected1 = AlterTableSerDePropertiesStatement( - Seq("table_name"), Some("org.apache.class"), None, None) val expected2 = AlterTableSerDePropertiesStatement( Seq("table_name"), Some("org.apache.class"), Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), None) + comparePlans(parsed2, expected2) + + val sql3 = + """ + |ALTER TABLE table_name + |SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val parsed3 = parsePlan(sql3) val expected3 = AlterTableSerDePropertiesStatement( Seq("table_name"), None, Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), None) + comparePlans(parsed3, expected3) + + val sql4 = + """ + |ALTER TABLE table_name PARTITION (test=1, dt='2008-08-08', country='us') + |SET SERDE 'org.apache.class' + |WITH SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val parsed4 = parsePlan(sql4) val expected4 = AlterTableSerDePropertiesStatement( Seq("table_name"), Some("org.apache.class"), Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) + comparePlans(parsed4, expected4) + + val sql5 = + """ + |ALTER TABLE table_name PARTITION (test=1, dt='2008-08-08', country='us') + |SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val parsed5 = parsePlan(sql5) val expected5 = AlterTableSerDePropertiesStatement( Seq("table_name"), None, Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) + comparePlans(parsed5, expected5) + + val sql6 = + """ + |ALTER TABLE a.b.c SET SERDE 'org.apache.class' + |WITH SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val parsed6 = parsePlan(sql6) val expected6 = AlterTableSerDePropertiesStatement( Seq("a", "b", "c"), Some("org.apache.class"), Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), None) + comparePlans(parsed6, expected6) + + val sql7 = + """ + |ALTER TABLE a.b.c PARTITION (test=1, dt='2008-08-08', country='us') + |SET SERDEPROPERTIES ('columns'='foo,bar', 'field.delim' = ',') + """.stripMargin + val parsed7 = parsePlan(sql7) val expected7 = AlterTableSerDePropertiesStatement( Seq("a", "b", "c"), None, Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) - comparePlans(parsed1, expected1) - comparePlans(parsed2, expected2) - comparePlans(parsed3, expected3) - comparePlans(parsed4, expected4) - comparePlans(parsed5, expected5) - comparePlans(parsed6, expected6) comparePlans(parsed7, expected7) }