[GitHub] spark pull request #17623: [SPARK-20292][SQL] Clean up string representation...

[cloud-fan <gi...@git.apache.org>]

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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org