berkaysynnada commented on PR #8891: URL: https://github.com/apache/arrow-datafusion/pull/8891#issuecomment-1972654893
> @berkaysynnada, I merged your PR: [peter-toth#1](https://github.com/peter-toth/arrow-datafusion/pull/1) and fixed a few issues. Please take a look at the `map_children()` of `DynTreeNode` and `ConcreteTreeNode`. I think those are now correctly propagate both `transformed` and `tnr`. I also fixed the todo from here: [peter-toth#1 (comment)](https://github.com/peter-toth/arrow-datafusion/pull/1#issuecomment-1970825454). > > I will update the PR description and address @alamb's comment tomorrow. Thank you for integrating the changes and addressing the issues. Your last commits enhance the overall structure and make the code smoother. However, I'm slightly worried about the absence of tests for the `map_children()` for "jump" scenarios and transform propagations. They appear to be correct and safe, yet having explicit tests would be preferable. I will provide some tests through https://github.com/apache/arrow-datafusion/issues/9111, and AFAIK you have some plans regarding using "jump" for some rules. I think they will indirectly guard those. > @berkaysynnada, @ozankabak I realy like the ideas in @alamb's [#8891 (comment)](https://github.com/apache/arrow-datafusion/pull/8891#discussion_r1504371192). I feel the same that in most of the cases we just want to run a transformation and get back the transformed tree. Appending `.data` or `.map(|t| t.data)` is inconvenient. So how about having `transform_down` and `transform_down_inner` like APIs? Or having a `.data()` method on `Result<Transformed<T>>`? I would prefer adding methods to `Transformed` instead of expanding the TreeNode API further. -- 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]
