[SPARK-13139][SQL] Follow-ups to #11573

Addressing outstanding comments in #11573.

Jenkins, new test case in `DDLCommandSuite`

Author: Andrew Or <andrew@databricks.com>

Closes #11667 from andrewor14/ddl-parser-followups.

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkQl.scala

@@ -34,8 +34,21 @@ private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends Cataly
   }
 
   /**
-   * For each node, extract properties in the form of a list ['key1', 'key2', 'key3', 'value']
-   * into a pair (key1.key2.key3, value).
+   * For each node, extract properties in the form of a list
+   * ['key_part1', 'key_part2', 'key_part3', 'value']
+   * into a pair (key_part1.key_part2.key_part3, value).
+   *
+   * Example format:
+   *
+   *   TOK_TABLEPROPERTY
+   *   :- 'k1'
+   *   +- 'v1'
+   *   TOK_TABLEPROPERTY
+   *   :- 'k2'
+   *   +- 'v2'
+   *   TOK_TABLEPROPERTY
+   *   :- 'k3'
+   *   +- 'v3'
    */
   private def extractProps(
       props: Seq[ASTNode],
@@ -101,6 +114,16 @@ private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends Cataly
         }
         val props = dbprops.toSeq.flatMap {
           case Token("TOK_DATABASEPROPERTIES", Token("TOK_DBPROPLIST", propList) :: Nil) =>
+            // Example format:
+            //
+            //   TOK_DATABASEPROPERTIES
+            //   +- TOK_DBPROPLIST
+            //      :- TOK_TABLEPROPERTY
+            //      :  :- 'k1'
+            //      :  +- 'v1'
+            //      :- TOK_TABLEPROPERTY
+            //         :- 'k2'
+            //         +- 'v2'
             extractProps(propList, "TOK_TABLEPROPERTY")
           case _ => parseFailed("Invalid CREATE DATABASE command", node)
         }.toMap
@@ -112,16 +135,16 @@ private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends Cataly
         // Example format:
         //
         //   TOK_CREATEFUNCTION
-        //     :- db_name
-        //     :- func_name
-        //     :- alias
-        //     +- TOK_RESOURCE_LIST
-        //        :- TOK_RESOURCE_URI
-        //        :  :- TOK_JAR
-        //        :  +- '/path/to/jar'
-        //        +- TOK_RESOURCE_URI
-        //           :- TOK_FILE
-        //           +- 'path/to/file'
+        //   :- db_name
+        //   :- func_name
+        //   :- alias
+        //   +- TOK_RESOURCE_LIST
+        //      :- TOK_RESOURCE_URI
+        //      :  :- TOK_JAR
+        //      :  +- '/path/to/jar'
+        //      +- TOK_RESOURCE_URI
+        //         :- TOK_FILE
+        //         +- 'path/to/file'
         val (funcNameArgs, otherArgs) = args.partition {
           case Token("TOK_RESOURCE_LIST", _) => false
           case Token("TOK_TEMPORARY", _) => false
@@ -139,9 +162,9 @@ private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends Cataly
         }
         // Extract other keywords, if they exist
         val Seq(rList, temp) = getClauses(Seq("TOK_RESOURCE_LIST", "TOK_TEMPORARY"), otherArgs)
-        val resourcesMap = rList.toSeq.flatMap {
-          case Token("TOK_RESOURCE_LIST", resources) =>
-            resources.map {
+        val resources: Seq[(String, String)] = rList.toSeq.flatMap {
+          case Token("TOK_RESOURCE_LIST", resList) =>
+            resList.map {
               case Token("TOK_RESOURCE_URI", rType :: Token(rPath, Nil) :: Nil) =>
                 val resourceType = rType match {
                   case Token("TOK_JAR", Nil) => "jar"
@@ -153,8 +176,8 @@ private[sql] class SparkQl(conf: ParserConf = SimpleParserConf()) extends Cataly
               case _ => parseFailed("Invalid CREATE FUNCTION command", node)
             }
           case _ => parseFailed("Invalid CREATE FUNCTION command", node)
-        }.toMap
-        CreateFunction(funcName, alias, resourcesMap, temp.isDefined)(node.source)
+        }
+        CreateFunction(funcName, alias, resources, temp.isDefined)(node.source)
 
       case Token("TOK_ALTERTABLE", alterTableArgs) =>
         AlterTableCommandParser.parse(node)

sql/core/src/main/scala/org/apache/spark/sql/execution/command/AlterTableCommandParser.scala

@@ -55,20 +55,22 @@ object AlterTableCommandParser {
   /**
    * Extract partition spec from the given [[ASTNode]] as a map, assuming it exists.
    *
-   * Expected format:
-   *   +- TOK_PARTSPEC
-   *      :- TOK_PARTVAL
-   *      :  :- dt
-   *      :  +- '2008-08-08'
-   *      +- TOK_PARTVAL
-   *         :- country
-   *         +- 'us'
+   * Example format:
+   *
+   *   TOK_PARTSPEC
+   *   :- TOK_PARTVAL
+   *   :  :- dt
+   *   :  +- '2008-08-08'
+   *   +- TOK_PARTVAL
+   *      :- country
+   *      +- 'us'
    */
   private def parsePartitionSpec(node: ASTNode): Map[String, String] = {
     node match {
       case Token("TOK_PARTSPEC", partitions) =>
         partitions.map {
           // Note: sometimes there's a "=", "<" or ">" between the key and the value
+          // (e.g. when dropping all partitions with value > than a certain constant)
           case Token("TOK_PARTVAL", ident :: conj :: constant :: Nil) =>
             (cleanAndUnquoteString(ident.text), cleanAndUnquoteString(constant.text))
           case Token("TOK_PARTVAL", ident :: constant :: Nil) =>
@@ -86,15 +88,16 @@ object AlterTableCommandParser {
   /**
    * Extract table properties from the given [[ASTNode]] as a map, assuming it exists.
    *
-   * Expected format:
-   *   +- TOK_TABLEPROPERTIES
-   *      +- TOK_TABLEPROPLIST
-   *         :- TOK_TABLEPROPERTY
-   *         :  :- 'test'
-   *         :  +- 'value'
-   *         +- TOK_TABLEPROPERTY
-   *            :- 'comment'
-   *            +- 'new_comment'
+   * Example format:
+   *
+   *   TOK_TABLEPROPERTIES
+   *   +- TOK_TABLEPROPLIST
+   *      :- TOK_TABLEPROPERTY
+   *      :  :- 'test'
+   *      :  +- 'value'
+   *      +- TOK_TABLEPROPERTY
+   *         :- 'comment'
+   *         +- 'new_comment'
    */
   private def extractTableProps(node: ASTNode): Map[String, String] = {
     node match {
@@ -209,21 +212,21 @@ object AlterTableCommandParser {
           Token("TOK_TABCOLNAME", colNames) :: colValues :: rest) :: Nil) :: _ =>
         // Example format:
         //
-        //   +- TOK_ALTERTABLE_SKEWED
-        //      :- TOK_TABLESKEWED
-        //      :  :- TOK_TABCOLNAME
-        //      :  :  :- dt
-        //      :  :  +- country
-        //      :- TOK_TABCOLVALUE_PAIR
-        //      :  :- TOK_TABCOLVALUES
-        //      :  :  :- TOK_TABCOLVALUE
-        //      :  :  :  :- '2008-08-08'
-        //      :  :  :  +- 'us'
-        //      :  :- TOK_TABCOLVALUES
-        //      :  :  :- TOK_TABCOLVALUE
-        //      :  :  :  :- '2009-09-09'
-        //      :  :  :  +- 'uk'
-        //      +- TOK_STOREASDIR
+        //   TOK_ALTERTABLE_SKEWED
+        //   :- TOK_TABLESKEWED
+        //   :  :- TOK_TABCOLNAME
+        //   :  :  :- dt
+        //   :  :  +- country
+        //   :- TOK_TABCOLVALUE_PAIR
+        //   :  :- TOK_TABCOLVALUES
+        //   :  :  :- TOK_TABCOLVALUE
+        //   :  :  :  :- '2008-08-08'
+        //   :  :  :  +- 'us'
+        //   :  :- TOK_TABCOLVALUES
+        //   :  :  :- TOK_TABCOLVALUE
+        //   :  :  :  :- '2009-09-09'
+        //   :  :  :  +- 'uk'
+        //   +- TOK_STOREASDIR
         val names = colNames.map { n => cleanAndUnquoteString(n.text) }
         val values = colValues match {
           case Token("TOK_TABCOLVALUE", vals) =>
@@ -260,20 +263,20 @@ object AlterTableCommandParser {
       case Token("TOK_ALTERTABLE_SKEWED_LOCATION",
         Token("TOK_SKEWED_LOCATIONS",
         Token("TOK_SKEWED_LOCATION_LIST", locationMaps) :: Nil) :: Nil) :: _ =>
-        // Expected format:
+        // Example format:
         //
-        //   +- TOK_ALTERTABLE_SKEWED_LOCATION
-        //      +- TOK_SKEWED_LOCATIONS
-        //         +- TOK_SKEWED_LOCATION_LIST
-        //            :- TOK_SKEWED_LOCATION_MAP
-        //            :  :- 'col1'
-        //            :  +- 'loc1'
-        //            +- TOK_SKEWED_LOCATION_MAP
-        //               :- TOK_TABCOLVALUES
-        //               :  +- TOK_TABCOLVALUE
-        //               :     :- 'col2'
-        //               :     +- 'col3'
-        //               +- 'loc2'
+        //   TOK_ALTERTABLE_SKEWED_LOCATION
+        //   +- TOK_SKEWED_LOCATIONS
+        //      +- TOK_SKEWED_LOCATION_LIST
+        //         :- TOK_SKEWED_LOCATION_MAP
+        //         :  :- 'col1'
+        //         :  +- 'loc1'
+        //         +- TOK_SKEWED_LOCATION_MAP
+        //            :- TOK_TABCOLVALUES
+        //            :  +- TOK_TABCOLVALUE
+        //            :     :- 'col2'
+        //            :     +- 'col3'
+        //            +- 'loc2'
         val skewedMaps = locationMaps.flatMap {
           case Token("TOK_SKEWED_LOCATION_MAP", col :: loc :: Nil) =>
             col match {

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

@@ -55,7 +55,7 @@ case class CreateDatabase(
 case class CreateFunction(
     functionName: String,
     alias: String,
-    resourcesMap: Map[String, String],
+    resources: Seq[(String, String)],
     isTemp: Boolean)(sql: String)
   extends NativeDDLCommand(sql) with Logging
 

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala

@@ -48,26 +48,26 @@ class DDLCommandSuite extends PlanTest {
     val sql1 =
       """
        |CREATE TEMPORARY FUNCTION helloworld as
-       |'com.matthewrathbone.example.SimpleUDFExample' USING JAR '/path/to/jar',
-       |FILE 'path/to/file'
+       |'com.matthewrathbone.example.SimpleUDFExample' USING JAR '/path/to/jar1',
+       |JAR '/path/to/jar2'
      """.stripMargin
     val sql2 =
       """
         |CREATE FUNCTION hello.world as
         |'com.matthewrathbone.example.SimpleUDFExample' USING ARCHIVE '/path/to/archive',
-        |FILE 'path/to/file'
+        |FILE '/path/to/file'
       """.stripMargin
     val parsed1 = parser.parsePlan(sql1)
     val parsed2 = parser.parsePlan(sql2)
     val expected1 = CreateFunction(
       "helloworld",
       "com.matthewrathbone.example.SimpleUDFExample",
-      Map("jar" -> "/path/to/jar", "file" -> "path/to/file"),
+      Seq(("jar", "/path/to/jar1"), ("jar", "/path/to/jar2")),
       isTemp = true)(sql1)
     val expected2 = CreateFunction(
       "hello.world",
       "com.matthewrathbone.example.SimpleUDFExample",
-      Map("archive" -> "/path/to/archive", "file" -> "path/to/file"),
+      Seq(("archive", "/path/to/archive"), ("file", "/path/to/file")),
       isTemp = false)(sql2)
     comparePlans(parsed1, expected1)
     comparePlans(parsed2, expected2)