peter-toth commented on PR #7942: URL: https://github.com/apache/arrow-datafusion/pull/7942#issuecomment-1866908166
I've updated the https://github.com/peter-toth/arrow-datafusion/commits/refactor-treenode-benchmark/ with a `LogicalPlan` benchmark so as the see how the proposed PR affects trees with `Arc`s. The benchmark is very similar to the previous `Expr` based one but uses `Union` and `EmptyRelation` to build up a tree: https://github.com/peter-toth/arrow-datafusion/commit/6d8ad17df8f3090f36824377d09bd6ef316476f7#diff-9619441d9605f143a911319cea75ae5192e6c5b17acfcbc17a3c73a9e32a8e61R83-R153 It is interresting to see that new `transform_down()` is better than the old `transform_down_old()` on `LogicalPlan` trees as well, but the improvement is not that significant though: ``` ---- tree_node::plan::test::transform_test stdout ---- create_union_tree: 8481 union_tree.transform_down_old: 6406 union_tree.clone: 0 union_tree_clone.transform_down: 3861 results: true union_tree.transform_down_old 2: 11855 union_tree_clone.transform_down 2: 9479 results: true results: false ``` I think the key takeaway of these 2 benchmarks is that I had to scale down the `LogicalPlan` based one run on a 23 height binary tree (https://github.com/peter-toth/arrow-datafusion/commit/75cde35dcbd3ef85e39e68d951b7fd26293be06e#diff-9619441d9605f143a911319cea75ae5192e6c5b17acfcbc17a3c73a9e32a8e61R102) vs the `Expr` based one that ran on 25 height tree (https://github.com/peter-toth/arrow-datafusion/commit/75cde35dcbd3ef85e39e68d951b7fd26293be06e#diff-6515fda3c67b9d487ab491fd21c27e32c411a8cbee24725d737290d19c10c199R498) to get the roughly similar transform down numbers. I think this is the cost of using `Arc`s vs. `Box`es and so we can't mutate in place. -- 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]
