Update optimize_children to return Result<Option<LogicalPlan>>
#4888
Conversation
Signed-off-by: remzi <13716567376yh@gmail.com>
There was a problem hiding this comment.
LGTM -- thank you. @HaoYang670 for the contribution. cc @jackwener
| match optimized_plan { | ||
| Some(optimized_plan) if optimized_plan.schema() != &original_schema => { | ||
| // add an additional projection if the output schema changed. | ||
| Ok(Some(build_recover_project_plan( | ||
| &original_schema, | ||
| optimized_plan, | ||
| ))) | ||
| } | ||
| plan => Ok(plan), |
There was a problem hiding this comment.
You might be able to do this something like
| match optimized_plan { | |
| Some(optimized_plan) if optimized_plan.schema() != &original_schema => { | |
| // add an additional projection if the output schema changed. | |
| Ok(Some(build_recover_project_plan( | |
| &original_schema, | |
| optimized_plan, | |
| ))) | |
| } | |
| plan => Ok(plan), | |
| optimized_plan.map(|optimized_plan| { | |
| if optimized_plan.schema() != &original_schema => { | |
| // add an additional projection if the output schema changed. | |
| build_recover_project_plan( | |
| &original_schema, | |
| optimized_plan, | |
| ) | |
| }); |
Not sure if that is all that much better
There was a problem hiding this comment.
Hmm, what is the rationale of this suggestion 🤔? It can't pass the type checker, I'm afraid.
There was a problem hiding this comment.
The rationale was to make it slightly less verbose and more "functional" by using map rather than match. i don't think it is required
| pub fn optimize_children( | ||
| optimizer: &impl OptimizerRule, | ||
| plan: &LogicalPlan, | ||
| config: &dyn OptimizerConfig, | ||
| ) -> Result<LogicalPlan> { | ||
| ) -> Result<Option<LogicalPlan>> { | ||
| let new_exprs = plan.expressions(); | ||
| let mut new_inputs = Vec::with_capacity(plan.inputs().len()); | ||
| let mut plan_is_changed = false; | ||
| for input in plan.inputs() { | ||
| let new_input = optimizer.try_optimize(input, config)?; | ||
| plan_is_changed = plan_is_changed || new_input.is_some(); | ||
| new_inputs.push(new_input.unwrap_or_else(|| input.clone())) | ||
| } | ||
| from_plan(plan, &new_exprs, &new_inputs) | ||
| if plan_is_changed { | ||
| Ok(Some(from_plan(plan, &new_exprs, &new_inputs)?)) | ||
| } else { | ||
| Ok(None) | ||
| } |
| @@ -749,7 +749,9 @@ impl OptimizerRule for PushDownFilter { | |||
| _ => plan.clone(), | |||
| }; | |||
|
|
|||
| Ok(Some(utils::optimize_children(self, &new_plan, config)?)) | |||
| Ok(Some( | |||
| utils::optimize_children(self, &new_plan, config)?.unwrap_or(new_plan), | |||
There was a problem hiding this comment.
| utils::optimize_children(self, &new_plan, config)?.unwrap_or(new_plan), | |
| optimize_children(self, &new_plan, config)?.unwrap_or(new_plan), |
|
Thanks for your review @alamb @jackwener 😁 |
👍 thank you @HaoYang670 -- I'll mark this PR as a draft so we don't accidentally merge it while working through the backlog |
…ldren_return_option
Signed-off-by: remzi <13716567376yh@gmail.com>
|
LGTM -- thanks again @HaoYang670 |
|
Benchmark runs are scheduled for baseline = 42c81be and contender = d37dccf. d37dccf 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 #4882.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?