refactor push_down_filter to fix dead-loop and use optimizer_recurse.
#5337
Conversation
jackwener
force-pushed
the
push_down_filter
branch
from
44c2f83 to
b990e88
Compare
jackwener
force-pushed
the
push_down_filter
branch
from
b990e88 to
454b251
Compare
It could be better to file an issue to explain when the |
| fn try_optimize( | ||
| &self, | ||
| plan: &LogicalPlan, | ||
| config: &dyn OptimizerConfig, | ||
| _config: &dyn OptimizerConfig, |
There was a problem hiding this comment.
Will it lose information if we disable config here?
There was a problem hiding this comment.
Will it lose information if we disable
confighere?
I didn't disable it, just because it's only used recursively, so it needs to be prefixed with _.
It's hint by cargo clippy
There was a problem hiding this comment.
just because it's only used recursively
In the original code, we pass the config to the optimize_children, but now, it is just a placeholder for self recursion, which means we never use the config anymore.
This is strange thing🥹, I also don't understand why. |
We'd better figure out what happens, so that the fix can make sense to every one. |
After investigation, it's due to by original code in |
| LogicalPlan::Join(join) => { | ||
| let optimized_plan = push_down_join(plan, join, None)?; | ||
| return match optimized_plan { | ||
| Some(optimized_plan) => Ok(Some( | ||
| optimize_children(self, &optimized_plan, config)? | ||
| .unwrap_or(optimized_plan), | ||
| )), | ||
| None => optimize_children(self, plan, config), | ||
| }; | ||
| } | ||
| _ => return optimize_children(self, plan, config), |
There was a problem hiding this comment.
@HaoYang670 Here remove a risk recursion.
jackwener
force-pushed
the
push_down_filter
branch
from
454b251 to
8f2ec6d
Compare
There was a problem hiding this comment.
Looks like a nice improvement to me -- thank you @jackwener and @HaoYang670
I agree it is strange that you were seeing a dead loop (is that the same as "infinite recursion") sometimes and not others, but I still think this code represents an improvement
|
Benchmark runs are scheduled for baseline = 8b92b9b and contender = 0b77ec2. 0b77ec2 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 #.
Rationale for this change
When I do #4465, I find
push_down_filterexist dead-loop.So I fix this problem, and refactor it.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?