Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CHORE] Use treenode for tree traversal in logical optimizer rules #2797

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

kevinzwang
Copy link
Member

@kevinzwang kevinzwang commented Sep 6, 2024

Follow-up from #2791 after offline conversation.

This PR still uses the existing logical plan optimizer, but now uses common_treenode::Transformed instead of a custom implementation. In addition, it removes the apply order logic from the optimizer and replaces it with common_treenode::TreeNode transforms done in the optimizer rules themselves.

This PR should not cause any functional changes to any of the optimizer rules or the way they are applied, except for the fact that rules in a batch are now each applied to the whole tree before the next rule, instead of each being applied to a single plan node if they have the same apply order. Future PRs will separate the rules out and make better use of treenode.

@kevinzwang kevinzwang marked this pull request as ready for review September 6, 2024 00:32
@github-actions github-actions bot added the chore label Sep 6, 2024
Copy link

codspeed-hq bot commented Sep 6, 2024

CodSpeed Performance Report

Merging #2797 will degrade performances by 12.25%

Comparing kevin/logical-optimizer-treenode (c9ea3bb) with main (6fe408c)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 14 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main kevin/logical-optimizer-treenode Change
test_count[1 Small File] 20.9 ms 23.8 ms -12.25%
test_show[100 Small Files] 59.5 ms 51.9 ms +14.56%


fn try_optimize(&self, plan: Arc<LogicalPlan>) -> DaftResult<Transformed<Arc<LogicalPlan>>> {
impl PushDownLimit {
#[allow(clippy::only_used_in_recursion)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this clippy allow? This should be used right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&self is not actually used other than recursive invocations. It's weird because I don't think Clippy complains if you don't use &self at all, only if there's a recursive call

Copy link
Member Author

@kevinzwang kevinzwang Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just like a quick and dirty fix to make Clippy happy and ensure that things stay the same before we start actually ripping into the rules themselves.

@kevinzwang kevinzwang merged commit 91d9fe9 into main Sep 6, 2024
42 of 43 checks passed
@kevinzwang kevinzwang deleted the kevin/logical-optimizer-treenode branch September 6, 2024 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants