Address comments and fix PySpark test

Eric5553 · Eric5553 · commit 1290cd523d6f · 2020-02-21T11:03:59.000+08:00
diff --git a/python/pyspark/sql/dataframe.py b/python/pyspark/sql/dataframe.py
@@ -284,7 +284,8 @@ def explain(self, extended=None, mode=None):
         == Physical Plan ==
         * Scan ExistingRDD (1)
         (1) Scan ExistingRDD [codegen id : 1]
-        Output: [age#0, name#1]
+        Output [2]: [age#0, name#1]
+        ...
 
         .. versionchanged:: 3.0.0
            Added optional argument `mode` to specify the expected output format of plans.
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala
@@ -20,7 +20,6 @@ package org.apache.spark.sql.catalyst.plans
 import org.apache.spark.sql.AnalysisException
 import org.apache.spark.sql.catalyst.expressions._
 import org.apache.spark.sql.catalyst.trees.{CurrentOrigin, TreeNode, TreeNodeTag}
-import org.apache.spark.sql.catalyst.util.StringUtils.PlanStringConcat
 import org.apache.spark.sql.internal.SQLConf
 import org.apache.spark.sql.types.{DataType, StructType}
 
@@ -189,15 +188,18 @@ abstract class QueryPlan[PlanType <: QueryPlan[PlanType]] extends TreeNode[PlanT
     val codegenIdStr =
       getTagValue(QueryPlan.CODEGEN_ID_TAG).map(id => s"[codegen id : $id]").getOrElse("")
     val operatorId = getTagValue(QueryPlan.OP_ID_TAG).map(id => s"$id").getOrElse("unknown")
+    val baseStr = s"($operatorId) $nodeName $codegenIdStr"
     val argumentString = argString(SQLConf.get.maxToStringFields)
 
-    val result = s"""
-       |($operatorId) $nodeName $codegenIdStr
-     """.stripMargin
-    if (argumentString != null && !argumentString.isEmpty) {
-      s"""${result} |Arguments: $argumentString\n""".stripMargin
+    if (argumentString.nonEmpty) {
+      s"""
+         |$baseStr
+         |Arguments: $argumentString
+      """.stripMargin
     } else {
-      result
+      s"""
+         |$baseStr
+      """.stripMargin
     }
   }
 
diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/ExplainUtils.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/ExplainUtils.scala
@@ -189,7 +189,7 @@ object ExplainUtils {
     case iter: Iterable[_] => s"${fieldName} [${iter.size}]: ${iter.mkString("[", ", ", "]")}"
     case str: String if (str == null || str.isEmpty) => s"${fieldName}: None"
     case str: String => s"${fieldName}: ${str}"
-    case _ => s"${fieldName}: Unknown"
+    case _ => throw new IllegalArgumentException(s"Unsupported type for argument values: $values")
   }
 
   /**
diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala
@@ -514,14 +514,20 @@ trait LeafExecNode extends SparkPlan {
   override def producedAttributes: AttributeSet = outputSet
   override def verboseStringWithOperatorId(): String = {
     val argumentString = argString(SQLConf.get.maxToStringFields)
-    val result = s"""
-         |(${ExplainUtils.getOpId(this)}) $nodeName ${ExplainUtils.getCodegenId(this)}
-         |${ExplainUtils.generateFieldString("Output", producedAttributes)}
-       """.stripMargin
-    if (argumentString != null && !argumentString.isEmpty) {
-      s"""${result} |Arguments: $argumentString\n""".stripMargin
+    val baseStr = s"(${ExplainUtils.getOpId(this)}) $nodeName ${ExplainUtils.getCodegenId(this)}"
+    val outputStr = s"${ExplainUtils.generateFieldString("Output", producedAttributes)}"
+
+    if (argumentString.nonEmpty) {
+      s"""
+         |$baseStr
+         |$outputStr
+         |Arguments: $argumentString
+      """.stripMargin
     } else {
-      s"${result}"
+      s"""
+         |$baseStr
+         |$outputStr
+      """.stripMargin
     }
   }
 }
@@ -539,14 +545,20 @@ trait UnaryExecNode extends SparkPlan {
   override final def children: Seq[SparkPlan] = child :: Nil
   override def verboseStringWithOperatorId(): String = {
     val argumentString = argString(SQLConf.get.maxToStringFields)
-    val result = s"""
-         |(${ExplainUtils.getOpId(this)}) $nodeName ${ExplainUtils.getCodegenId(this)}
-         |${ExplainUtils.generateFieldString("Input", child.output)}
-       """.stripMargin
-    if (argumentString != null && !argumentString.isEmpty) {
-      s"""${result} |Arguments: $argumentString\n""".stripMargin
+    val baseStr = s"(${ExplainUtils.getOpId(this)}) $nodeName ${ExplainUtils.getCodegenId(this)}"
+    val inputStr = s"${ExplainUtils.generateFieldString("Input", child.output)}"
+
+    if (argumentString.nonEmpty) {
+      s"""
+         |$baseStr
+         |$inputStr
+         |Arguments: $argumentString
+      """.stripMargin
     } else {
-      s"${result}"
+      s"""
+         |$baseStr
+         |$inputStr
+      """.stripMargin
     }
   }
 }
@@ -558,15 +570,23 @@ trait BinaryExecNode extends SparkPlan {
   override final def children: Seq[SparkPlan] = Seq(left, right)
   override def verboseStringWithOperatorId(): String = {
     val argumentString = argString(SQLConf.get.maxToStringFields)
-    val result = s"""
-         |(${ExplainUtils.getOpId(this)}) $nodeName ${ExplainUtils.getCodegenId(this)}
-         |${ExplainUtils.generateFieldString("Left output", left.output)}
-         |${ExplainUtils.generateFieldString("Right output", right.output)}
-       """.stripMargin
-    if (argumentString != null && !argumentString.isEmpty) {
-      s"""${result} |Arguments: $argumentString\n""".stripMargin
+    val baseStr = s"(${ExplainUtils.getOpId(this)}) $nodeName ${ExplainUtils.getCodegenId(this)}"
+    val leftOutputStr = s"${ExplainUtils.generateFieldString("Left output", left.output)}"
+    val rightOutputStr = s"${ExplainUtils.generateFieldString("Right output", right.output)}"
+
+    if (argumentString.nonEmpty) {
+      s"""
+         |$baseStr
+         |$leftOutputStr
+         |$rightOutputStr
+         |Arguments: $argumentString
+      """.stripMargin
     } else {
-      s"${result}"
+      s"""
+         |$baseStr
+         |$leftOutputStr
+         |$rightOutputStr
+      """.stripMargin
     }
   }
 }
diff --git a/sql/core/src/test/resources/sql-tests/results/explain.sql.out b/sql/core/src/test/resources/sql-tests/results/explain.sql.out

Original file line number	Diff line number	Diff line change
`@@ -189,7 +189,7 @@ object ExplainUtils {`
`189`	`189`	`case iter: Iterable[_] => s"${fieldName} [${iter.size}]: ${iter.mkString("[", ", ", "]")}"`
`190`	`190`	`case str: String if (str == null \|\| str.isEmpty) => s"${fieldName}: None"`
`191`	`191`	`case str: String => s"${fieldName}: ${str}"`
`192`		`- case _ => s"${fieldName}: Unknown"`
	`192`	`+ case _ => throw new IllegalArgumentException(s"Unsupported type for argument values: $values")`
`193`	`193`	`}`
`194`	`194`
`195`	`195`	`/**`