peter-toth commented on PR #8891: URL: https://github.com/apache/arrow-datafusion/pull/8891#issuecomment-1932689758
Some more thoughts on https://synnada.notion.site/synnada/TreeNode-Design-Proposal-bceac27d18504a2085145550e267c4c1: > Previous Skip variant is somehow confusing, and have different meanings in different context. In the new suggested desing, it is replaced with Jump. I think we are on the same page. But `TreeNodeRecursion::Skip` in this PR behaves as `VisitRecursion::Skip` did (and not as `RewriteRecursion::Skip` did): - When `Skip` is returned in top-down closures it simply prevents recursing into children and prevents calling the bottom-up closure. Calling the bottom-up closure doesn't make sense as the same transformation can be done by the top-down one that returned `Skip`. I think this is the 1st picture in your document. - When it is returned in bottom-up closures it works exactly as `TreeNodeRecursion::Continue` as we already visited children. > During top-down traversals, it means that the current subtree will be skipped, and if there exists another child subtree, the traversal is continued from there. If there does not, it means the same with the Stop, halts the traversal and collect all transformation till that node. Ok, it looks like top-down wise we are on the same page. > During bottom-up traversals, Jump makes the walk on the current subtree skipped till the first multi-child node. So `Jump` seems to be different to my `Skip` in bottom-up traversal. I think this is 2nd image in your document. Do you have any usecase where it make sense to skip `post_visit(D)`? > Rather than using Skip in that way, the rules can have their own handlings to skip that node with Transformed::No and Recursion::Continue. This works exactly the same way in this PR. -- 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]
