fix: preserve short-circuit evaluation in Boolean CASE simplification - #25143
fix: preserve short-circuit evaluation in Boolean CASE simplification#25143emecii wants to merge 2 commits into
Conversation
Generated-by: OpenAI Codex
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The overall direction makes sense, and the new regression coverage catches the original short-circuit issue well. I found one remaining correctness case around the first WHEN condition that I think needs to be addressed before merging. I also left a small comment suggestion to keep the documented invariant aligned with the implementation.
| when_then_expr.iter().enumerate().all(|(i, (when, then))| { | ||
| is_leaf(then) | ||
| && if i == 0 { | ||
| !when.is_volatile() |
There was a problem hiding this comment.
I think there is still a correctness issue with allowing the first WHEN as long as it is non-volatile. It can still fail, and the Boolean rewrite can eliminate its evaluation entirely.
For example, SELECT CASE WHEN CAST(s AS INT) > 0 THEN false ELSE false END FROM (VALUES ('abc')) t(s) should raise the cast error because CASE evaluates its first WHEN for every row. With this rewrite, both branches can simplify to false, so the condition is never evaluated and the query returns false instead.
Could we require the first WHEN to be a column or literal as well, unless we add a sound fallibility analysis? It would also be good to add this example as a regression test, particularly with identical literal results so we cover the case where simplification removes the condition.
There was a problem hiding this comment.
Fixed in 3d52a60. The guard now requires the first WHEN, every later WHEN, and all branch outputs to be columns or literals. I added the CAST(s AS INT) regression with identical false outputs and a unit case proving the CASE is preserved.
|
|
||
| /// Conservatively checks the inputs whose evaluation can change when lowering | ||
| /// CASE to AND/OR. [`Expr`] has no general fallibility analysis: only columns and | ||
| /// literals are admitted from conditional branches, including later WHEN conditions. |
There was a problem hiding this comment.
Once the first-WHEN guard is tightened, could we update this comment to match the invariant and say that all WHEN conditions and branch outputs must be columns or literals? I think spelling that out here will make it less likely that a future special case accidentally reintroduces this error-preservation gap.
There was a problem hiding this comment.
Updated in 3d52a60. The invariant comment now explicitly states that all WHEN conditions and branch outputs must be columns or literals.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25143 +/- ##
==========================================
- Coverage 81.83% 81.83% -0.01%
==========================================
Files 1130 1130
Lines 418785 418819 +34
Branches 418785 418819 +34
==========================================
+ Hits 342723 342727 +4
- Misses 55857 55883 +26
- Partials 20205 20209 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which issue does this PR close?
Closes #25136.
Rationale for this change
CASE WHEN s ~ '^[0-9]+$' THEN CAST(s AS INT) > 0 ELSE false ENDcurrently becomes anANDpredicate. On input containing'abc', predicate evaluation can reach the cast even though theWHENexcluded that row.What changes are included in this PR?
Restrict the searched Boolean CASE-to-AND/OR rewrite to columns or literals in
THEN,ELSE, and laterWHENexpressions. The firstWHENalready runs on every row, but must be nonvolatile because expansion may repeat it. Apply the same restriction to the companion inversion rule.This is deliberately conservative: it does not attempt general expression fallibility analysis or change predicate ordering. More complex conditional expressions retain their CASE structure. Existing column/literal rewrites remain available, and two optimizer tests record the intentionally reduced simplification.
Three EXPLAIN snapshots in
null_aware_mark_join.sltandsubquery.sltalso change. CASE expressions generated for ANY comparisons now survive until EXISTS decorrelation, allowing the first mark column to be reused instead of duplicating its join. Their query-result expectations are unchanged.What is the testing strategy for this PR?
The SQL regressions in
case.sltcover guarded casts in filters and projections, true/false/NULL/implicit ELSE, a fallible later WHEN with literal outputs, guarded division, and a selected invalid branch that must still error. Five queries failed against current main before the optimizer change. The unit regression also covers a volatile first WHEN.Validation with pinned Rust 1.97.0:
cargo fmt --allandgit diff --checkpassed.cargo clippy --all-targets --all-features -- -D warningspassed../dev/rust_lint.shsuite passed.The nearby
simplify_function_over_casemicrobenchmark was run against current main with separately rebuilt binaries (Rust 1.98.0, dev profile, short Criterion samples). The quieter comparison reported no change or changes within the noise threshold. It exercises string-output CASE, not the retained Boolean CASE workloads, so this is not a general performance claim.Are there any user-facing changes?
Guarded Boolean CASE expressions preserve branch-local evaluation. Some optimized plans retain CASE expressions where Boolean lowering was previously applied; there is no public API change.
Generated with OpenAI Codex assistance, including implementation, tests, and this description.