Optimizer: avoid every rule must recursive children in optimizer #4618
Conversation
| /// If a rule use default None, its should traverse recursively plan inside itself | ||
| fn apply_order(&self) -> Option<ApplyOrder> { | ||
| None | ||
| } |
There was a problem hiding this comment.
It' compatible with origin
|
PTAL @alamb @Dandandan @liukun4515 |
|
In the future, I also want to utilize the trait of |
|
I plan to look at this PR carefully tomorrow |
|
We definitely shouldn't hold this up on a theoretical future change, but just FYI - #4627 |
|
has resolved conflict, now there are clear |
|
@jackwener I think you can leverage the
|
|
BTW, some complex rules might be a mixed up of bottom-up and top-down processes. |
|
Good advice for me, thanks @mingmwang |
There was a problem hiding this comment.
Looks really great to me @jackwener -- thank you!
I have some suggestions related to adding some more docstrings but I think this PR is ready to go.
Note it will likely conflict with #4645
| @@ -59,6 +59,11 @@ pub trait OptimizerRule { | |||
|
|
|||
| /// A human readable name for this optimizer rule | |||
| fn name(&self) -> &str; | |||
|
|
|||
| /// If a rule use default None, its should traverse recursively plan inside itself | |||
There was a problem hiding this comment.
| /// If a rule use default None, its should traverse recursively plan inside itself | |
| /// How should the rule be applied by the optimizer? See comments on [`ApplyOrder`] for details. | |
| /// | |
| /// If a rule returns `None`, the default, its should traverse the plan recursively inside itself |
| @@ -274,6 +284,77 @@ impl Optimizer { | |||
| debug!("Optimizer took {} ms", start_time.elapsed().as_millis()); | |||
| Ok(new_plan) | |||
| } | |||
|
|
|||
| fn optimize_node( | |||
There was a problem hiding this comment.
What value does this function add? In other words, why not call rule.try_optimize directly at the callsite 🤔
There was a problem hiding this comment.
I forgot to add future TODO:
we can do batch optimize
for rule in rules:
rule.try_optimize(rule)| Ok(Some(plan.with_new_inputs(new_inputs.as_slice())?)) | ||
| } | ||
|
|
||
| pub fn optimize_recursively( |
There was a problem hiding this comment.
Can you please add docstrings about what this API does?
Also, I wonder if it needs to be pub or if it could be private 🤔
There was a problem hiding this comment.
Sometime we need use it in UT😂
| Some(order) => match order { | ||
| ApplyOrder::TopDown => { | ||
| let optimize_self_opt = | ||
| self.optimize_node(rule, plan, optimizer_config)?; |
There was a problem hiding this comment.
I took a shot at rewriting this stuff to use Result::and_then, etc and I think how you have written it here is clearer 👍
jackwener
force-pushed
the
optimizer
branch
3 times, most recently
from
628cb5f to
e0101ce
Compare
|
Thanks @alamb review!❤️ Advices is used for me. |
|
CI fail due to clippy, fix in #4652 |
has fixed it, and rebased. |
| /// | ||
| /// Notice: **sometime** result after optimize still can be optimized, we need apply again. | ||
| /// | ||
| /// Usage Example: Merge Limit (subtree pattern is: Limit-Limit) |
|
This PR had conflicts, so I took the liberty of resolving them and pushing a fix as @jackwener had already done so 👍 Once CI passes I plan to merge this in again. Thanks again @jackwener |
|
Benchmark runs are scheduled for baseline = 920f11a and contender = 414487c. 414487c 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 #4613 .
Rationale for this change
Currently, we must do optimize recursively in each rules, which is troubled.
It make it's a little hard for putting some rule into optimizer.
What changes are included in this PR?
avoid every rule must recursive children in optimizer
And I modify the
eliminate_limitas an example.When
apply_orderis None, it's completely compatible with original behavior.Are these changes tested?
Are there any user-facing changes?