Add try_optimize method #4208
Add try_optimize method #4208
Conversation
There was a problem hiding this comment.
Nice Job 👍. This is one of the things I prepare to do in the future.
Add a newmethed is a good way to keep backwards compatibility(I‘m blocked by it)
Thanks @andygrove !
| @@ -20,7 +20,7 @@ use crate::utils::{ | |||
| verify_not_disjunction, | |||
| }; | |||
| use crate::{utils, OptimizerConfig, OptimizerRule}; | |||
| use datafusion_common::{context, plan_err, DataFusionError}; | |||
| use datafusion_common::context; | |||
There was a problem hiding this comment.
keep DataFusionError, following code we can use Result instead of datafusion_common::Result
There was a problem hiding this comment.
Or maybe we could just do use datafusion_common::Result
Will we remove the existing |
in my opinion, It depends on can we refactor all rule, when we do all job, we can remove. |
| @@ -20,7 +20,7 @@ use crate::utils::{ | |||
| verify_not_disjunction, | |||
| }; | |||
| use crate::{utils, OptimizerConfig, OptimizerRule}; | |||
| use datafusion_common::{context, plan_err, DataFusionError}; | |||
| use datafusion_common::context; | |||
There was a problem hiding this comment.
Or maybe we could just do use datafusion_common::Result
Yes, exactly. This way we can update one rule at a time, and remove it when we are done. |
|
Benchmark runs are scheduled for baseline = 4653df4 and contender = 28ca3ee. 28ca3ee 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 #4209
Rationale for this change
This PR aims to improve the API for optimizer rules by adding a new
try_optimizemethod that returnsResult<Option<LogicalPlan>>so that rules can now choose to returnOk(None)if the rule is not applicable to the plan.What changes are included in this PR?
try_optimizemethod toOptimizertrait with a default implementation that calls the existingoptimizemethod for backwards compatibilitydecorrelate_where_existsto use this new approachAre these changes tested?
Relevant unit tests updated to expect new behavior.
Are there any user-facing changes?