Uh oh!
There was an error while loading. Please reload this page.
remove recursion in optimizer rules - #4650
Conversation
| pub predicate: Expr, | ||
| /// The incoming logical plan | ||
| input: Arc<LogicalPlan>, | ||
| pub input: Arc<LogicalPlan>, |
There was a problem hiding this comment.
pub them because I need pattern-match in rules.
Discussion about it in #4464.
There was a problem hiding this comment.
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
| LogicalPlan::Filter(Filter { | ||
| predicate: Expr::Literal(ScalarValue::Boolean(Some(v))), | ||
| input, | ||
| }) => { |
There was a problem hiding this comment.
Nested Pattern-match here, so I make filter public
There was a problem hiding this comment.
I wonder if you could use a match guard?
LogicalPlan::Filter(filter)ifmatches(!filter.expr(),Expr::Literal(ScalarValue::Boolean(Some(v))))){ ...jackwener
commented
Dec 15, 2022
Rely on #4618 |
alamb
commented
Dec 15, 2022
Marking as draft as it builds on #4618 |
alamb
left a comment
There was a problem hiding this comment.
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
Filterfieldspubas 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.
| pub predicate: Expr, | ||
| /// The incoming logical plan | ||
| input: Arc<LogicalPlan>, | ||
| pub input: Arc<LogicalPlan>, |
There was a problem hiding this comment.
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
| "decorrelate_where_in" | ||
| } | ||
| fn apply_order(&self) -> Option<ApplyOrder> { |
There was a problem hiding this comment.
this is a really nice pattern 👨🍳 👌
| 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 |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
😂 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.
| LogicalPlan::Filter(Filter { | ||
| predicate: Expr::Literal(ScalarValue::Boolean(Some(v))), | ||
| input, | ||
| }) => { |
There was a problem hiding this comment.
I wonder if you could use a match guard?
LogicalPlan::Filter(filter)ifmatches(!filter.expr(),Expr::Literal(ScalarValue::Boolean(Some(v))))){ ...jackwener
commented
Dec 19, 2022
I find a good way to resolve it by adding |
alamb
commented
Dec 20, 2022
This is reasonable (it will prevent construction outside of the crate, so in datafusion core and planner) 👍 nice |
ursabot
commented
Dec 20, 2022
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. |
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?