peter-toth commented on PR #13177: URL: https://github.com/apache/datafusion/pull/13177#issuecomment-2463044895
> I am very worried about using `stacker` or some other similar crate -- while it sounds like it solves all the problems easily I have the following concerns: > > 1. It is not widely used (only 53 dependencies: https://crates.io/crates/stacker/reverse_dependencies) > 2. It was dormant for over 2 years and only recently has started getting updates: https://crates.io/crates/stacker/versions > 3. I worry it will cause some sort of hard to diagnose issue that appears only under load / memory pressure / etc The fact that Rust compiler uses `stacker` doesn't alleviate your concerns? I mean, it might have been dormant for some time but as long as the compiler needs it, it seems safe to use it. > My feeling is that after reading this PR is that there is a very large amount of complexity that arises from ExecutionPlans that don't have ownership of their children (aka they are `Arc<dyn ExecutionPlan`> rather than `Box<dyn ExecutionPlan`> > > If we could actually modify the `ExecutionPlan`s (take children, add new children) I think this code would likely be much simpler as well as more performant. We could also potentially offer in-place mutation APIs that would be even faster. > > What do people think about trying to push through a change to avoid `Arc<dyn ExecutionPlan`> ? I think that's a bit different topic, both the current recursive and proposed iterative approaches could benefit if taking a node's children and reattaching them would be possible. I don't think this is the main differentiator between the 2. I think what we need to decide is if we can rely on `stacker`: - If we can, then we can keep the IMHO cleaner recursive implementation with minimal changes. - If we cannot, then we need to switch to the iterative approach. But fixing all possible stack overflows will be a long journey with iterative implementations. The difficulties will arrise with the last 3 points of my [comment](https://github.com/apache/datafusion/pull/13177#issuecomment-2452940289): Changing the custom recursive rules to avoid using `map_children`, implementing `take_children` / `with_new_children` for `Expr` and finally mixed tree traversal (e.g. `LogicalPlan`'s `..._with_subqquery()` methods). -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
