sql: Simplify quantified comparisons in JOIN ON clauses#36065
sql: Simplify quantified comparisons in JOIN ON clauses#36065def- merged 1 commit intoMaterializeInc:mainfrom
Conversation
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
There was a problem hiding this comment.
Hmm, thanks, this looks good to me!
But I'd say we should create a feature flag for it (on by default), because we'd like an escape hatch in case it causes a catastrophic performance regression at some user. It's extremely hard to fully anticipate the consequences of optimizer changes. Even though the HIR plan change of this PR is generally considered a positive change, there are tons of surprising non-monotonicities in the optimizer, in the sense that when the plan looks better at a certain stage of the pipeline, it often happens that the better-looking plan is handled worse by a later optimizer pipeline stage, and we end up with a regression overall. (See #35441 for more discussion on the evolvability of the optimizer.)
I've kicked off the random query parts of Nightly as a further precaution: https://buildkite.com/materialize/nightly/builds/15999
| transform_hir::try_simplify_quantified_comparisons(&mut other)?; | ||
| if context.config.enable_simplify_quantified_comparisons { | ||
| transform_hir::try_simplify_quantified_comparisons(&mut other)?; | ||
| } |
There was a problem hiding this comment.
This would completely disable try_simplify_quantified_comparisons when the flag is off, so we'd have changed plans from before the PR with the flag both on and off. What I meant is to guard only the new part of try_simplify_quantified_comparisons with the feature flag, because the important thing is to be able to get back to the old plans by turning off the flag (if needed).
There was a problem hiding this comment.
Oops, thanks a lot for catching!
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
LGTM, just a minor thing left
|
Thanks for the great reviews Gabor! |
2 participants
This comes from #9623