[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user viirya closed the pull request at: https://github.com/apache/spark/pull/17623 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r117493117
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala
---
@@ -111,6 +111,11 @@ case class GetStructField(child: Expression, ordinal:
Int, name: Option[String]
override def dataType: DataType = childSchema(ordinal).dataType
override def nullable: Boolean = child.nullable ||
childSchema(ordinal).nullable
+ override def verboseString: String = {
--- End diff --
`toString` shows all children nodes under the expression, but
`verboseString` only shows the info of the current node.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r112856025
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala
---
@@ -111,6 +111,11 @@ case class GetStructField(child: Expression, ordinal:
Int, name: Option[String]
override def dataType: DataType = childSchema(ordinal).dataType
override def nullable: Boolean = child.nullable ||
childSchema(ordinal).nullable
+ override def verboseString: String = {
--- End diff --
We rarely call directly `Expression.verboseString`. It is mostly called by
`treeString` to show individual nodes in this tree.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r112854488
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala
---
@@ -111,6 +111,11 @@ case class GetStructField(child: Expression, ordinal:
Int, name: Option[String]
override def dataType: DataType = childSchema(ordinal).dataType
override def nullable: Boolean = child.nullable ||
childSchema(ordinal).nullable
+ override def verboseString: String = {
--- End diff --
I don't think the `verboseString` here provides better string
representation than `toString`, when will we call `verboseString`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r112805879
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala
---
@@ -111,6 +111,11 @@ case class GetStructField(child: Expression, ordinal:
Int, name: Option[String]
override def dataType: DataType = childSchema(ordinal).dataType
override def nullable: Boolean = child.nullable ||
childSchema(ordinal).nullable
+ override def verboseString: String = {
--- End diff --
`verboseString` is often override like this to provide better string
representation for specified expressions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r112805866
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala
---
@@ -111,6 +111,11 @@ case class GetStructField(child: Expression, ordinal:
Int, name: Option[String]
override def dataType: DataType = childSchema(ordinal).dataType
override def nullable: Boolean = child.nullable ||
childSchema(ordinal).nullable
+ override def verboseString: String = {
--- End diff --
Sorry I don't get the meaning of your question.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r112805843
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
---
@@ -149,6 +149,7 @@ case class Cast(child: Expression, dataType: DataType,
timeZoneId: Option[String
def this(child: Expression, dataType: DataType) = this(child, dataType,
None)
+ override def verboseString: String = s"cast to ${dataType.simpleString}"
--- End diff --
`verboseString` doesn't show children information. `toString` does.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r112703773
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala
---
@@ -111,6 +111,11 @@ case class GetStructField(child: Expression, ordinal:
Int, name: Option[String]
override def dataType: DataType = childSchema(ordinal).dataType
override def nullable: Boolean = child.nullable ||
childSchema(ordinal).nullable
+ override def verboseString: String = {
--- End diff --
so `verboseString` must use function-style format like `func(x1, x2)`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r112703193
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
---
@@ -149,6 +149,7 @@ case class Cast(child: Expression, dataType: DataType,
timeZoneId: Option[String
def this(child: Expression, dataType: DataType) = this(child, dataType,
None)
+ override def verboseString: String = s"cast to ${dataType.simpleString}"
--- End diff --
why it's different from `toString`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r112636638
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
---
@@ -422,40 +422,62 @@ abstract class TreeNode[BaseType <:
TreeNode[BaseType]] extends Product {
def nodeName: String = getClass.getSimpleName.replaceAll("Exec$", "")
/**
+ * Returns a user-facing string representation of this node's name. By
default it's `nodeName`.
+ */
+ def prettyName: String = nodeName
+
+ /**
* The arguments that should be included in the arg string. Defaults to
the `productIterator`.
*/
protected def stringArgs: Iterator[Any] = productIterator
private lazy val allChildren: Set[TreeNode[_]] = (children ++
innerChildren).toSet[TreeNode[_]]
+ /** Converts a node to string. Subclasses can override this to use other
string representation. */
+ protected def argToString(arg: Any): String = arg match {
+case tn: TreeNode[_] => tn.verboseString
+case _ => arg.toString
+ }
+
/** Returns a string representing the arguments to this node, minus any
children */
def argString: String = stringArgs.flatMap {
case tn: TreeNode[_] if allChildren.contains(tn) => Nil
case Some(tn: TreeNode[_]) if allChildren.contains(tn) => Nil
-case Some(tn: TreeNode[_]) => tn.simpleString :: Nil
-case tn: TreeNode[_] => tn.simpleString :: Nil
+case Some(tn: TreeNode[_]) => tn :: Nil
+case tn: TreeNode[_] => tn :: Nil
case seq: Seq[Any] if
seq.toSet.subsetOf(allChildren.asInstanceOf[Set[Any]]) => Nil
case iter: Iterable[_] if iter.isEmpty => Nil
-case seq: Seq[_] => Utils.truncatedString(seq, "[", ", ", "]") :: Nil
-case set: Set[_] => Utils.truncatedString(set.toSeq, "{", ", ", "}")
:: Nil
+case seq: Seq[_] => Utils.truncatedString(seq.map(argToString), "[",
", ", "]") :: Nil
+case set: Set[_] => Utils.truncatedString(set.toSeq.map(argToString),
"{", ", ", "}") :: Nil
case array: Array[_] if array.isEmpty => Nil
-case array: Array[_] => Utils.truncatedString(array, "[", ", ", "]")
:: Nil
+case array: Array[_] => Utils.truncatedString(array.map(argToString),
"[", ", ", "]") :: Nil
case null => Nil
case None => Nil
case Some(null) => Nil
case Some(any) => any :: Nil
case other => other :: Nil
- }.mkString(", ")
+ }.map(argToString).mkString(", ")
- /** ONE line description of this node. */
- def simpleString: String = s"$nodeName $argString".trim
+ /** ONE line description of this node, not including any arguments and
children information */
+ def simpleString: String = prettyName
- /** ONE line description of this node with more information */
- def verboseString: String
+ /**
+ * ONE line description of this node with more information, without any
children information.
+ * By default, it includes the arguments to this node, minus any
children.
+ * It is mainly called by `generateTreeString`, when constructing the
string representation
+ * of the nodes in this tree. Subclasses can override it to provide more
user-friendly
+ * representation.
+ */
+ def verboseString: String = if (argString != "") {
+s"$prettyName $argString".trim
+ } else {
+simpleString
+ }
- /** ONE line description of this node with some suffix information */
+ /** ONE line description of this node by adding some suffix information
to `verboseString` */
def verboseStringWithSuffix: String = verboseString
--- End diff --
I've added the related interfaces in the PR description. Please take a look
and let me know if there're others needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r111719916
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
---
@@ -422,40 +422,62 @@ abstract class TreeNode[BaseType <:
TreeNode[BaseType]] extends Product {
def nodeName: String = getClass.getSimpleName.replaceAll("Exec$", "")
/**
+ * Returns a user-facing string representation of this node's name. By
default it's `nodeName`.
+ */
+ def prettyName: String = nodeName
+
+ /**
* The arguments that should be included in the arg string. Defaults to
the `productIterator`.
*/
protected def stringArgs: Iterator[Any] = productIterator
private lazy val allChildren: Set[TreeNode[_]] = (children ++
innerChildren).toSet[TreeNode[_]]
+ /** Converts a node to string. Subclasses can override this to use other
string representation. */
+ protected def argToString(arg: Any): String = arg match {
+case tn: TreeNode[_] => tn.verboseString
+case _ => arg.toString
+ }
+
/** Returns a string representing the arguments to this node, minus any
children */
def argString: String = stringArgs.flatMap {
case tn: TreeNode[_] if allChildren.contains(tn) => Nil
case Some(tn: TreeNode[_]) if allChildren.contains(tn) => Nil
-case Some(tn: TreeNode[_]) => tn.simpleString :: Nil
-case tn: TreeNode[_] => tn.simpleString :: Nil
+case Some(tn: TreeNode[_]) => tn :: Nil
+case tn: TreeNode[_] => tn :: Nil
case seq: Seq[Any] if
seq.toSet.subsetOf(allChildren.asInstanceOf[Set[Any]]) => Nil
case iter: Iterable[_] if iter.isEmpty => Nil
-case seq: Seq[_] => Utils.truncatedString(seq, "[", ", ", "]") :: Nil
-case set: Set[_] => Utils.truncatedString(set.toSeq, "{", ", ", "}")
:: Nil
+case seq: Seq[_] => Utils.truncatedString(seq.map(argToString), "[",
", ", "]") :: Nil
+case set: Set[_] => Utils.truncatedString(set.toSeq.map(argToString),
"{", ", ", "}") :: Nil
case array: Array[_] if array.isEmpty => Nil
-case array: Array[_] => Utils.truncatedString(array, "[", ", ", "]")
:: Nil
+case array: Array[_] => Utils.truncatedString(array.map(argToString),
"[", ", ", "]") :: Nil
case null => Nil
case None => Nil
case Some(null) => Nil
case Some(any) => any :: Nil
case other => other :: Nil
- }.mkString(", ")
+ }.map(argToString).mkString(", ")
- /** ONE line description of this node. */
- def simpleString: String = s"$nodeName $argString".trim
+ /** ONE line description of this node, not including any arguments and
children information */
+ def simpleString: String = prettyName
- /** ONE line description of this node with more information */
- def verboseString: String
+ /**
+ * ONE line description of this node with more information, without any
children information.
+ * By default, it includes the arguments to this node, minus any
children.
+ * It is mainly called by `generateTreeString`, when constructing the
string representation
+ * of the nodes in this tree. Subclasses can override it to provide more
user-friendly
+ * representation.
+ */
+ def verboseString: String = if (argString != "") {
+s"$prettyName $argString".trim
+ } else {
+simpleString
+ }
- /** ONE line description of this node with some suffix information */
+ /** ONE line description of this node by adding some suffix information
to `verboseString` */
def verboseStringWithSuffix: String = verboseString
--- End diff --
Ok. Let me update this description later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17623#discussion_r111717917
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
---
@@ -422,40 +422,62 @@ abstract class TreeNode[BaseType <:
TreeNode[BaseType]] extends Product {
def nodeName: String = getClass.getSimpleName.replaceAll("Exec$", "")
/**
+ * Returns a user-facing string representation of this node's name. By
default it's `nodeName`.
+ */
+ def prettyName: String = nodeName
+
+ /**
* The arguments that should be included in the arg string. Defaults to
the `productIterator`.
*/
protected def stringArgs: Iterator[Any] = productIterator
private lazy val allChildren: Set[TreeNode[_]] = (children ++
innerChildren).toSet[TreeNode[_]]
+ /** Converts a node to string. Subclasses can override this to use other
string representation. */
+ protected def argToString(arg: Any): String = arg match {
+case tn: TreeNode[_] => tn.verboseString
+case _ => arg.toString
+ }
+
/** Returns a string representing the arguments to this node, minus any
children */
def argString: String = stringArgs.flatMap {
case tn: TreeNode[_] if allChildren.contains(tn) => Nil
case Some(tn: TreeNode[_]) if allChildren.contains(tn) => Nil
-case Some(tn: TreeNode[_]) => tn.simpleString :: Nil
-case tn: TreeNode[_] => tn.simpleString :: Nil
+case Some(tn: TreeNode[_]) => tn :: Nil
+case tn: TreeNode[_] => tn :: Nil
case seq: Seq[Any] if
seq.toSet.subsetOf(allChildren.asInstanceOf[Set[Any]]) => Nil
case iter: Iterable[_] if iter.isEmpty => Nil
-case seq: Seq[_] => Utils.truncatedString(seq, "[", ", ", "]") :: Nil
-case set: Set[_] => Utils.truncatedString(set.toSeq, "{", ", ", "}")
:: Nil
+case seq: Seq[_] => Utils.truncatedString(seq.map(argToString), "[",
", ", "]") :: Nil
+case set: Set[_] => Utils.truncatedString(set.toSeq.map(argToString),
"{", ", ", "}") :: Nil
case array: Array[_] if array.isEmpty => Nil
-case array: Array[_] => Utils.truncatedString(array, "[", ", ", "]")
:: Nil
+case array: Array[_] => Utils.truncatedString(array.map(argToString),
"[", ", ", "]") :: Nil
case null => Nil
case None => Nil
case Some(null) => Nil
case Some(any) => any :: Nil
case other => other :: Nil
- }.mkString(", ")
+ }.map(argToString).mkString(", ")
- /** ONE line description of this node. */
- def simpleString: String = s"$nodeName $argString".trim
+ /** ONE line description of this node, not including any arguments and
children information */
+ def simpleString: String = prettyName
- /** ONE line description of this node with more information */
- def verboseString: String
+ /**
+ * ONE line description of this node with more information, without any
children information.
+ * By default, it includes the arguments to this node, minus any
children.
+ * It is mainly called by `generateTreeString`, when constructing the
string representation
+ * of the nodes in this tree. Subclasses can override it to provide more
user-friendly
+ * representation.
+ */
+ def verboseString: String = if (argString != "") {
+s"$prettyName $argString".trim
+ } else {
+simpleString
+ }
- /** ONE line description of this node with some suffix information */
+ /** ONE line description of this node by adding some suffix information
to `verboseString` */
def verboseStringWithSuffix: String = verboseString
--- End diff --
can we list all the string related interfaces defined in `TreeNode` in the
PR description? e.g. `nodeName`, `prettyName`, `stringArgs`, etc. This can be
very helpful to review this PR and a good reference for future contributors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
