[GitHub] [spark] rednaxelafx commented on pull request #29771: [SPARK-32635][SQL] Fix foldable propagation

[GitBox <gi...@apache.org>]

rednaxelafx commented on pull request #29771:
URL: https://github.com/apache/spark/pull/29771#issuecomment-694112139


   LGTM (not a Reviewer). Eliminating `var` FTW!
   
   I'd like to ask about a few things though:
   
   1. The fix involved a bit of refactoring. Is the bug caused by "leaking" the `foldableMap` from a sibling node, such that it's not properly just propagating along the child->parent flow? And that the refactoring eliminated the `var` the threaded the propagation via returns in a recursive depth-first traversal to make sure that it's always only propagating along the child->parent flow, and otherwise the existing logic is unchanged? If so, this looks like a nice refactoring.
   2. `TreeNode`'s `transformXXX` functions take care of the case where a new node is actually equal to the old one, so that after transformation the logically unchanged nodes will stay with their original identity. It looks like with the explicit recursion after this PR, that it might introduce more, unnecessary copies of the logically unchanged nodes that end up staying in the transformed tree. Is that the case? If so, can we make it better?
   
   Thanks!


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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