spark git commit: [SPARK-20412] Throw ParseException from visitNonOptionalPartitionSpec instead of returning null values.

[wenchen]

Repository: spark
Updated Branches:
  refs/heads/branch-2.2 eb4d097c3 -> aaeca8bdd


[SPARK-20412] Throw ParseException from visitNonOptionalPartitionSpec instead 
of returning null values.

## What changes were proposed in this pull request?

If a partitionSpec is supposed to not contain optional values, a ParseException 
should be thrown, and not nulls returned.
The nulls can later cause NullPointerExceptions in places not expecting them.

## How was this patch tested?

A query like "SHOW PARTITIONS tbl PARTITION(col1='val1', col2)" used to throw a 
NullPointerException.
Now it throws a ParseException.

Author: Juliusz Sompolski 

Closes #17707 from juliuszsompolski/SPARK-20412.

(cherry picked from commit c9e6035e1fb825d280eaec3bdfc1e4d362897ffd)
Signed-off-by: Wenchen Fan 


Project: http://git-wip-us.apache.org/repos/asf/spark/repo
Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/aaeca8bd
Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/aaeca8bd
Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/aaeca8bd

Branch: refs/heads/branch-2.2
Commit: aaeca8bdd4bbbad5a14e1030e1d7ecf4836e8a5d
Parents: eb4d097
Author: Juliusz Sompolski 
Authored: Fri Apr 21 22:11:24 2017 +0800
Committer: Wenchen Fan 
Committed: Fri Apr 21 22:20:55 2017 +0800

--
 .../spark/sql/catalyst/parser/AstBuilder.scala  |  5 -
 .../sql/execution/command/DDLCommandSuite.scala | 16 
 2 files changed, 16 insertions(+), 5 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/spark/blob/aaeca8bd/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
--
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 e1db1ef..2cf06d1 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,7 +215,10 @@ 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 {
+  case (key, None) => throw new ParseException(s"Found an empty partition 
key '$key'.", ctx)
+  case (key, Some(value)) => key -> value
+}
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/spark/blob/aaeca8bd/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala
--
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 97c61dc..8a6bc62 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)
@@ -832,6 +832,14 @@ class DDLCommandSuite extends PlanTest {
 assert(e.contains("Found duplicate keys 'a'"))
   }
 
+  test("empty 

spark git commit: [SPARK-20412] Throw ParseException from visitNonOptionalPartitionSpec instead of returning null values.

[wenchen]

Repository: spark
Updated Branches:
  refs/heads/master 34767997e -> c9e6035e1


[SPARK-20412] Throw ParseException from visitNonOptionalPartitionSpec instead 
of returning null values.

## What changes were proposed in this pull request?

If a partitionSpec is supposed to not contain optional values, a ParseException 
should be thrown, and not nulls returned.
The nulls can later cause NullPointerExceptions in places not expecting them.

## How was this patch tested?

A query like "SHOW PARTITIONS tbl PARTITION(col1='val1', col2)" used to throw a 
NullPointerException.
Now it throws a ParseException.

Author: Juliusz Sompolski 

Closes #17707 from juliuszsompolski/SPARK-20412.


Project: http://git-wip-us.apache.org/repos/asf/spark/repo
Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/c9e6035e
Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/c9e6035e
Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/c9e6035e

Branch: refs/heads/master
Commit: c9e6035e1fb825d280eaec3bdfc1e4d362897ffd
Parents: 3476799
Author: Juliusz Sompolski 
Authored: Fri Apr 21 22:11:24 2017 +0800
Committer: Wenchen Fan 
Committed: Fri Apr 21 22:11:24 2017 +0800

--
 .../spark/sql/catalyst/parser/AstBuilder.scala  |  5 -
 .../sql/execution/command/DDLCommandSuite.scala | 16 
 2 files changed, 16 insertions(+), 5 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/spark/blob/c9e6035e/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
--
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 e1db1ef..2cf06d1 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,7 +215,10 @@ 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 {
+  case (key, None) => throw new ParseException(s"Found an empty partition 
key '$key'.", ctx)
+  case (key, Some(value)) => key -> value
+}
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/spark/blob/c9e6035e/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala
--
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 97c61dc..8a6bc62 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)
@@ -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