add try_optimize() for all rules.
#4599
Conversation
There was a problem hiding this comment.
Very nice -- thank you @jackwener 🏅
I wonder if after this change we should perhaps simply remove OptimizerRule::optimize to simply the code 🤔
| @@ -107,7 +111,7 @@ impl OptimizerRule for DecorrelateWhereExists { | |||
| } | |||
|
|
|||
| // iterate through all exists clauses in predicate, turning each into a join | |||
| let mut cur_input = (**filter_input).clone(); | |||
| let mut cur_input = filter_input.clone(); | |||
| @@ -40,27 +40,45 @@ impl OptimizerRule for EliminateLimit { | |||
| fn optimize( | |||
| &self, | |||
| plan: &LogicalPlan, | |||
| optimizer_config: &mut OptimizerConfig, | |||
| _optimizer_config: &mut OptimizerConfig, | |||
There was a problem hiding this comment.
I find it strange to use a variable named _something -- I thought the Rust convention was to name variables starting with _ when they were not used 🤔
There was a problem hiding this comment.
resolved.
BTW, in the followup PR, I will unify them to make all of them become optimizer_config if possible.
|
cc @andygrove |
jackwener
force-pushed
the
try_optimize
branch
from
44b2580
to
f897127
Compare
I prepare to do it in followup-PR |
|
Followup PR
|
| optimize_internal(&DFSchema::empty(), plan) | ||
| Ok(self | ||
| .try_optimize(plan, optimizer_config)? | ||
| .unwrap_or_else(|| plan.clone())) |
There was a problem hiding this comment.
I've stumbled upon this while working on #4614. I think this is wrong: the type coercion MUST NOT be skipped in case of an error, otherwise we may end up with non-executable plans.
There was a problem hiding this comment.
This PR don't change any process logic.
I am not sure about skipped you said.
Current try_optimize alway is some, or maybe I can replace .unwrap_or_else(|| plan.clone())) with unwarp()
And it will be deleted in next PR, so I think it doesn't matter
There was a problem hiding this comment.
I think @crepererum is saying that if the type coercion rules fail internally (return an Error) it is likely a serious bug in DataFusion and the plan will not work as expected.
By effectively ignoring the error here, the error will appear at some later stage (e.g. can't run the plan), making it harder to debug the source of the issue
🤔
There was a problem hiding this comment.
I get it, there is some misunderstand here.
I felt like this comment isn't related with this PR. So I was a little confused about it.
Here I don't ignore error.
self
.try_optimize(plan, optimizer_config)?
.unwrap_or_else(|| plan.clone())error will be return by ?
unwrap_or_else is used for option<>
There was a problem hiding this comment.
OK, let's move the discussion / fix to a follow-up: #4615.
|
Thank you @jackwener 😎 |
|
Benchmark runs are scheduled for baseline = 0baf5ef and contender = 508ba80. 508ba80 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 #4598.
Followup #4208
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?