berkaysynnada commented on issue #8663: URL: https://github.com/apache/arrow-datafusion/issues/8663#issuecomment-1871803326
Thanks @peter-toth for your detailed explanation ☺️ > Where I see some differences is the return type of our closures. Your code snipett shows `Result<()>` while I was suggesting `Result<TreeNodeRecursion>`. This is because I wanted to deprecate the current `rewrite()` / `TreeNodeRewriter` and replace/combine that with a new `transform()` method. If we want to do that then the new closures need to be able to prune the tree traversal (skip a subtree) and I don't think we can do it with a simple `Result<()>`. (Actaully as the same pruning capability is needed for the `TreeNode.apply()` / `TreeNode.visit()` as well so I poposed to have a common `TreeNodeRecursion` enum for visit and transform functions. This is 2. in #7942) I hadn't thought deeply about the return type, but your suggestion makes a lot of sense. > But my question about fn children<'a>(&mut self) -> &'a mut [Self]; is that can we return children as &mut[Self] without collecting them into a temporary collection like Vec? Currently Expr.map_children() doesn't collect the children into a temporary collection so if we start doing it then we might introduce some performance regression. I wasn't aware of `Expr` specifically, but I considered this approach because collection is already performed in all rules on the physical plan side. I will review #8664 and write my new thoughts there. I believe we will find the most appropriate solution 🚀 -- 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]
