draft: avoid every rule must recursive children in optimizer #3972
Conversation
github-actions
bot
added
logical-expr
|
I also implemented similar things to PhysicalExprs. I think we can have a unified Trait. |
Yes, Essentially it's the same thing. The main difference is that one is a plan tree and the other is an expression tree |
The Trait |
This job is great. It will improve optimizer well and is overall compatible. The trouble may lie in how to deal with the rules that currently exist. And I think we can create a new optimizer struct and gradually migrate the rules. I find these code don't exist in master, Looking forward to your PR, @mingmwang thanks! |
|
@alamb @andygrove @Dandandan how do you think? |
| @@ -342,6 +342,53 @@ impl LogicalPlan { | |||
| self.accept(&mut visitor)?; | |||
| Ok(visitor.using_columns) | |||
| } | |||
|
|
|||
| pub fn clone_with_inputs(&self, inputs: Vec<LogicalPlan>) -> Result<LogicalPlan, DataFusionError> { | |||
There was a problem hiding this comment.
This has a non trivial amount in common with from_plan:
https://github.com/apache/arrow-datafusion/blob/4cb8ac094ee88dc0023b4cde1b39840a0a362ee0/datafusion/expr/src/utils.rs#L363
I wonder if we could make from_plan easier to find / use / more general 🤔
| fn optimize_inputs(&self, rule: &Arc<dyn OptimizerRule + Send + Sync>, plan: &LogicalPlan, optimizer_config: &mut OptimizerConfig) -> Result<LogicalPlan> { | ||
| let result: Result<Vec<LogicalPlan>> = plan | ||
| .inputs() | ||
| .into_iter() | ||
| .map(|sub_plan| self.optimize_recursively(rule, sub_plan, optimizer_config)) | ||
| .collect(); | ||
| let inputs = result?; | ||
| plan.clone_with_inputs(inputs) | ||
| } | ||
|
|
There was a problem hiding this comment.
I like where this is headed @jackwener 👍
jackwener
changed the title
jackwener
changed the title
|
in new PR #4618 |
Which issue does this PR close?
This PR relate #2620.
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.
If use new optimizer, we just need to consider the subtree which we need to consider.
However, it's really challenging is how to deal with the rules that currently exist
Rationale for this change
What changes are included in this PR?
This PR is draft, which present how to use optimizer instead of rule to traverse plantree.
And PR include an example rule, which present the new rule.
Are there any user-facing changes?