remove recursion in optimizer rules #4650

jackwener · 2022-12-15T15:09:00Z

Which issue does this PR close?

Closes #.

Rationale for this change

Remove recursion in optimizer rules. and utilize the optimizer traverse the plan tree.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jackwener · 2022-12-15T15:11:26Z

datafusion/expr/src/logical_plan/plan.rs

+ pub predicate: Expr,
 /// The incoming logical plan
- input: Arc<LogicalPlan>,
+ pub input: Arc<LogicalPlan>,


pub them because I need pattern-match in rules.

Discussion about it in #4464.

cc @alamb @andygrove @tustvold .

I think it is ok -- it would be nice to add some comments explaining a Filter should not be created directly but instead use try_new() and that these fields are only pub to support pattern matching

jackwener · 2022-12-15T15:12:50Z

datafusion/optimizer/src/eliminate_filter.rs

+ LogicalPlan::Filter(Filter {
+ predicate: Expr::Literal(ScalarValue::Boolean(Some(v))),
+ input,
+ }) => {


Nested Pattern-match here, so I make filter public

I wonder if you could use a match guard?

LogicalPlan::Filter(filter) if matches(!filter.expr(), Expr::Literal(ScalarValue::Boolean(Some(v))))) { ...

jackwener · 2022-12-15T15:16:54Z

Rely on #4618

alamb · 2022-12-15T19:52:00Z

Marking as draft as it builds on #4618

alamb

As usual this is looking great @jackwener . Thank you 🙏

From my perspective this PR is basically ready to go except for:

If possible I think it would be worth the time trying to avoid making Filter fields pub as we have discussed earlier. I left a suggestion on how maybe that could be done.
Double check the Top Down vs Bottom Up changes in sql optimizer passes

Really nicely done and thank you again.

alamb · 2022-12-19T19:44:33Z

datafusion/expr/src/logical_plan/plan.rs

+ pub predicate: Expr,
 /// The incoming logical plan
- input: Arc<LogicalPlan>,
+ pub input: Arc<LogicalPlan>,


I think it is ok -- it would be nice to add some comments explaining a Filter should not be created directly but instead use try_new() and that these fields are only pub to support pattern matching

alamb · 2022-12-19T19:46:10Z

datafusion/optimizer/src/decorrelate_where_in.rs

 }
 }

 fn name(&self) -> &str {
 "decorrelate_where_in"
 }
+
+ fn apply_order(&self) -> Option<ApplyOrder> {


this is a really nice pattern 👨‍🍳 👌

alamb · 2022-12-19T19:48:27Z

benchmarks/expected-plans/q20.txt

@@ -1,17 +1,17 @@
 Sort: supplier.s_name ASC NULLS LAST
 Projection: supplier.s_name, supplier.s_address
- LeftSemi Join: supplier.s_suppkey = __sq_2.ps_suppkey
+ LeftSemi Join: supplier.s_suppkey = __sq_1.ps_suppkey


these changes imply the decorrelate passes used to be applied bottom up and after this PR they are applied top-down

Is that intentional? Or maybe I am misreading the diff 🤔

😂 original implementation mix TopDown and BottomUp.
It use TopDown overall, but use BottomUp when match the subPlan.
I think it's a small mistake in original code but don't affect correctness.

alamb · 2022-12-19T19:51:20Z

datafusion/optimizer/src/eliminate_filter.rs

+ LogicalPlan::Filter(Filter {
+ predicate: Expr::Literal(ScalarValue::Boolean(Some(v))),
+ input,
+ }) => {


I wonder if you could use a match guard?

LogicalPlan::Filter(filter) if matches(!filter.expr(), Expr::Literal(ScalarValue::Boolean(Some(v))))) { ...

jackwener · 2022-12-19T23:49:19Z

If possible I think it would be worth the time trying to avoid making Filter fields pub as we have discussed earlier. I left a suggestion on how maybe that could be done.

I find a good way to resolve it by adding #[non_exhaustive]

alamb · 2022-12-20T10:47:06Z

I find a good way to resolve it by adding #[non_exhaustive]

This is reasonable (it will prevent construction outside of the crate, so in datafusion core and planner) 👍 nice

alamb

Thanks @jackwener

ursabot · 2022-12-20T18:32:57Z

Benchmark runs are scheduled for baseline = fe477e4 and contender = 2792113. 2792113 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

jackwener commented Dec 15, 2022

View reviewed changes

jackwener force-pushed the remove_recursion branch from 88cdde8 to 045926e Compare December 15, 2022 15:14

github-actions bot added core Core datafusion crate logical-expr Logical plan and expressions optimizer Optimizer rules labels Dec 15, 2022

alamb marked this pull request as draft December 15, 2022 19:51

jackwener force-pushed the remove_recursion branch from 045926e to 9622d47 Compare December 19, 2022 05:31

jackwener marked this pull request as ready for review December 19, 2022 05:32

jackwener force-pushed the remove_recursion branch 2 times, most recently from b21faa9 to 4e2afb7 Compare December 19, 2022 05:40

alamb reviewed Dec 19, 2022

View reviewed changes

optimizer: remove recursion in optimizer rules

e7a961e

jackwener force-pushed the remove_recursion branch from 4e2afb7 to e7a961e Compare December 19, 2022 23:50

alamb approved these changes Dec 20, 2022

View reviewed changes

alamb merged commit 2792113 into apache:master Dec 20, 2022

jackwener deleted the remove_recursion branch December 21, 2022 01:55

jackwener mentioned this pull request Dec 21, 2022

FOLLOWUP: remove more recursion in optimizer rules. #4687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove recursion in optimizer rules #4650

remove recursion in optimizer rules #4650

jackwener commented Dec 15, 2022 •

edited

Loading

jackwener Dec 15, 2022

alamb Dec 19, 2022

jackwener Dec 15, 2022

alamb Dec 19, 2022

jackwener commented Dec 15, 2022

alamb commented Dec 15, 2022

alamb left a comment

alamb Dec 19, 2022

alamb Dec 19, 2022

alamb Dec 19, 2022

jackwener Dec 19, 2022

alamb Dec 19, 2022

jackwener commented Dec 19, 2022

alamb commented Dec 20, 2022

alamb left a comment

ursabot commented Dec 20, 2022

remove recursion in optimizer rules #4650

remove recursion in optimizer rules #4650

Conversation

jackwener commented Dec 15, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackwener commented Dec 15, 2022

alamb commented Dec 15, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackwener commented Dec 19, 2022

alamb commented Dec 20, 2022

alamb left a comment

Choose a reason for hiding this comment

ursabot commented Dec 20, 2022

jackwener commented Dec 15, 2022 •

edited

Loading