blaginin commented on PR #13177: URL: https://github.com/apache/datafusion/pull/13177#issuecomment-2452695373
Thanks for the comments! 🙏 As for the performance, I removed a few extra Vec collections and pushes. I have also added the implementation for `ConcreteNode` as @berkaysynnada asked. Benchmarks for me [haven't really changed](https://gist.github.com/blaginin/865a989fa6a57161e70deb706296d547). --- As for the tests, I'm not sure I got the idea. This PR reimplements some methods in the interfaces, while the existing tests only call the main implementation. If I remove the test changes, then the current PR won't be tested, which I am not sure is desirable? --- > Also, I don't see yet how this will work on tree nodes that own their children (e.g., Expr) I think this is a very valid point, @peter-toth! `Expr` and also `LogicalPlan` are both hard to switch to `ConcreteNode`. The solution will definitely be clumsier than just using a library. But I think the reason we might still want to try is because of: - Previous discussions: https://github.com/apache/datafusion-sqlparser-rs/pull/1468#issuecomment-2414747574, https://github.com/apache/datafusion/issues/9373#issuecomment-2414855369 - You've suggested a library, `recursive` where it says: > If your recursive algorithm is very performance-sensitive I would suggest rewriting it to an iterative version regardless. Apart from the obvious implementation solutions, I was thinking about [capturing unpacked objects in closures](https://github.com/apache/datafusion/issues/9373#issuecomment-2424113550), but maybe we can think of something better? This is definitely a very big piece of work, with a lot of discussion needed and potentially quite a few breaking changes... However, I am not sure we should do this discussion as part of this PR. My idea here was to fix the cases that can easily be fixed. So now, even without breaking changes, we can fix quite a few of the existing structs and future ones that will implement the supported interface. And even if we decide to introduce tools like `recursive` later, I’d argue that we’d still aim to use them only when an iterative approach isn’t feasible, as the docs suggest. -- 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]
