berkaysynnada commented on PR #8891: URL: https://github.com/apache/arrow-datafusion/pull/8891#issuecomment-1933614494
> 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. What do you think about separation of TreeNodeTransform and TreeNodeRewrite? It can both separate the concerns, and resolve the unused methods, such as that transform functions actually don't need apply_children() implementations. > 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 current `transform_up()` calls will simply change to the new`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 refactor all `transform_up()`s to `TreeNodeRewriter`s, just add an extra top-down closure. But we need to fix the APIs first... I understand the need of f_down, then f_up with pruned tree, but I think this must be the use of Rewriter/Visitor. Why do you think that all transform_up's will be converted to TreeNodeRewriter's? The functionality of your transform() needs to be converted to Rewriter only. As I check the diffs, there is only one usage of transform() with a dummy f_down. -- 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]
