peter-toth commented on PR #8891: URL: https://github.com/apache/arrow-datafusion/pull/8891#issuecomment-1932360660
> You are right, very likely they will be like those. I would like to question do we need separate TreeNodeRewriter trait, or encapsulate it within TreeNode. I feel both the `TreeNodeRewriter` (`TreeNode:rewrite()`) and the `TreeNode::transform()` (top-down and bottom-up transform closures directly provided) have their own prefered usecase. `TreeNodeRewriter` is useful if tou have an external data structure that is used by the transformations during rewrite. In that case you can include both the data structure and the transformation into a rewriter. But in many cases we don't have such a data structure so just passing in the 2 closures is a simpler alternative. Sidenote: Originally I wanted to speed up most of the current transforms (up and down) with pruning based on the tree structure. It is widely used technique to store a bitmap in treenodes that contains information about the subtree and use the top-down closure to prune (`Skip`) a subtree during transformations. One of the reasons why I propose adding `transform` (as a 2 closure transformation) is because after that change most of the `transform_up()` calls will simply change to `transform` by simply adding a new `f_down` closure that prunes when there is no interresing node to the `f_up` closure (`f_up` is the original closure provided to `transform_up`). In this case I don't want to use `TreeNodeRewriter`s. -- 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]
