LuciferYang commented on a change in pull request #35890:
URL: https://github.com/apache/spark/pull/35890#discussion_r829663269



##########
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
##########
@@ -610,24 +610,15 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] 
extends Product with Tre
 
   /**
    * Returns a copy of this node where `f` has been applied to all the nodes 
in `children`.
+   * This method force making a copy of the nodes even if no child has been 
changed.
    * @param f The transform function to be applied on applicable `TreeNode` 
elements.
-   * @param forceCopy Whether to force making a copy of the nodes even if no 
child has been changed.
    */
-  private def mapChildren(
-      f: BaseType => BaseType,
-      forceCopy: Boolean): BaseType = {
-    var changed = false
+  private def mapChildrenWithForceCopy(f: BaseType => BaseType): BaseType = {

Review comment:
       Should we move this method into `clone()` method? It is now only used by 
`TreeNode.clone`
   
   

##########
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
##########
@@ -610,24 +610,15 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] 
extends Product with Tre
 
   /**
    * Returns a copy of this node where `f` has been applied to all the nodes 
in `children`.
+   * This method force making a copy of the nodes even if no child has been 
changed.
    * @param f The transform function to be applied on applicable `TreeNode` 
elements.
-   * @param forceCopy Whether to force making a copy of the nodes even if no 
child has been changed.
    */
-  private def mapChildren(
-      f: BaseType => BaseType,
-      forceCopy: Boolean): BaseType = {
-    var changed = false
+  private def mapChildrenWithForceCopy(f: BaseType => BaseType): BaseType = {

Review comment:
       Should we move this method into `clone()` method? Then we can change 
   
   ```scala
   f(arg.asInstanceOf[BaseType]) 
   ```
   to
   
   ```scala
   arg.asInstanceOf[BaseType].clone()
   ```
   
   It is now only used by `TreeNode.clone`
   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to