From 71aac755011660c3f48006e83bb1baab69ca5c48 Mon Sep 17 00:00:00 2001 From: Juliusz Sompolski Date: Thu, 20 Apr 2017 17:21:08 +0200 Subject: [PATCH 1/4] Throw ParseException from visitNonOptionalPartitionSpec --- .../org/apache/spark/sql/catalyst/parser/AstBuilder.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 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 e1db1ef5b869..2b92f4a23778 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 @@ -215,9 +215,11 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with Logging { */ protected def visitNonOptionalPartitionSpec( ctx: PartitionSpecContext): Map[String, String] = withOrigin(ctx) { - visitPartitionSpec(ctx).mapValues(_.orNull).map(identity) + visitPartitionSpec(ctx).map(_ match { + case (key, value) => + (key, value.getOrElse(throw new ParseException(s"Found empty key '$key'.", ctx))) + }) } - /** * Convert a constant of any type into a string. This is typically used in DDL commands, and its * main purpose is to prevent slight differences due to back to back conversions i.e.: From 59ba235120ba526505f736afa41559885b6474f8 Mon Sep 17 00:00:00 2001 From: Juliusz Sompolski Date: Thu, 20 Apr 2017 17:25:06 +0200 Subject: [PATCH 2/4] lost a line --- .../scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala | 1 + 1 file changed, 1 insertion(+) 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 2b92f4a23778..f42ba5431c65 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 @@ -220,6 +220,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with Logging { (key, value.getOrElse(throw new ParseException(s"Found empty key '$key'.", ctx))) }) } + /** * Convert a constant of any type into a string. This is typically used in DDL commands, and its * main purpose is to prevent slight differences due to back to back conversions i.e.: From ca8ac7e30613bf5395a96b5b31c99d2b4de3e792 Mon Sep 17 00:00:00 2001 From: Juliusz Sompolski Date: Thu, 20 Apr 2017 19:25:53 +0200 Subject: [PATCH 3/4] add test --- .../apache/spark/sql/catalyst/parser/AstBuilder.scala | 10 +++++----- .../spark/sql/execution/command/DDLCommandSuite.scala | 8 ++++++++ 2 files changed, 13 insertions(+), 5 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 f42ba5431c65..38f2a8467a2c 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 @@ -214,11 +214,11 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with Logging { * Create a partition specification map without optional values. */ protected def visitNonOptionalPartitionSpec( - ctx: PartitionSpecContext): Map[String, String] = withOrigin(ctx) { - visitPartitionSpec(ctx).map(_ match { - case (key, value) => - (key, value.getOrElse(throw new ParseException(s"Found empty key '$key'.", ctx))) - }) + ctx: PartitionSpecContext): Map[String, String] = withOrigin(ctx) { + visitPartitionSpec(ctx).map { + case (key, None) => throw new ParseException(s"Found empty key '$key'.", ctx) + case (key, Some(value)) => key -> value + } } /** diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala index 97c61dc8694b..28add45376f2 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala @@ -832,6 +832,14 @@ class DDLCommandSuite extends PlanTest { assert(e.contains("Found duplicate keys 'a'")) } + test("empty values in non-optional partition specs") { + val e = intercept[ParseException] { + parser.parsePlan( + "SHOW PARTITIONS dbx.tab1 PARTITION (a='1', b)") + }.getMessage + assert(e.contains("Found empty key 'b'")) + } + test("drop table") { val tableName1 = "db.tab" val tableName2 = "tab" From 4abfa2ab7bafbb41cc99c7a98358f9bf39b7fba1 Mon Sep 17 00:00:00 2001 From: Juliusz Sompolski Date: Fri, 21 Apr 2017 11:35:49 +0200 Subject: [PATCH 4/4] review remarks + fix test. --- .../apache/spark/sql/catalyst/parser/AstBuilder.scala | 4 ++-- .../spark/sql/execution/command/DDLCommandSuite.scala | 10 +++++----- 2 files changed, 7 insertions(+), 7 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 38f2a8467a2c..2cf06d15664d 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 @@ -214,9 +214,9 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with Logging { * Create a partition specification map without optional values. */ protected def visitNonOptionalPartitionSpec( - ctx: PartitionSpecContext): Map[String, String] = withOrigin(ctx) { + ctx: PartitionSpecContext): Map[String, String] = withOrigin(ctx) { visitPartitionSpec(ctx).map { - case (key, None) => throw new ParseException(s"Found empty key '$key'.", ctx) + case (key, None) => throw new ParseException(s"Found an empty partition key '$key'.", ctx) case (key, Some(value)) => key -> value } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala index 28add45376f2..8a6bc62fec96 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala @@ -530,13 +530,13 @@ class DDLCommandSuite extends PlanTest { """.stripMargin val sql4 = """ - |ALTER TABLE table_name PARTITION (test, dt='2008-08-08', + |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, dt='2008-08-08', + |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) @@ -558,12 +558,12 @@ class DDLCommandSuite extends PlanTest { tableIdent, Some("org.apache.class"), Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), - Some(Map("test" -> null, "dt" -> "2008-08-08", "country" -> "us"))) + 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" -> null, "dt" -> "2008-08-08", "country" -> "us"))) + Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us"))) comparePlans(parsed1, expected1) comparePlans(parsed2, expected2) comparePlans(parsed3, expected3) @@ -837,7 +837,7 @@ class DDLCommandSuite extends PlanTest { parser.parsePlan( "SHOW PARTITIONS dbx.tab1 PARTITION (a='1', b)") }.getMessage - assert(e.contains("Found empty key 'b'")) + assert(e.contains("Found an empty partition key 'b'")) } test("drop table") {