Repository navigation
fix: do not evaluate a volatile coalesce or nvl argument twice - #25561
DeviousCardi wants to merge 5 commits into
Conversation
github-actions
Bot
added
documentation
2ab95fc to
da30a95
Compare
|
@DeviousCardi |
|
@kosiew Thanks! I merged main and resolved the conflict. It was only in the 56.0.0 upgrade guide, where my note and new upstream sections landed in the same spot, so I kept both. Ready for review whenever you have time. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25561 +/- ##
========================================
Coverage 82.84% 82.84%
========================================
Files 1151 1151
Lines 455510 455681 +171
Branches 455510 455681 +171
========================================
+ Hits 377362 377527 +165
- Misses 55236 55237 +1
- Partials 22912 22917 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The fix addresses the volatile-expression evaluation bug while preserving the existing CASE rewrite when it is safe. The runtime implementation and regression coverage look good.
I've left one optional suggestion to strengthen coverage for a volatile final argument. I did not review upgrading/56.0.0.md as it contains conflict
|
|
||
| # Non-volatile `coalesce` keeps the lazy CASE rewrite (see also | ||
| # select.slt, which asserts `coalesce(1, y/x)` never divides by zero). | ||
| query TT |
There was a problem hiding this comment.
Could we add a SQL regression test for a volatile final argument, such as coalesce(a, y / x + random()) with non-null a and x = 0? It should return a without a divide-by-zero error, confirming that the CASE rewrite preserves lazy evaluation at runtime. This is optional since the existing tests already cover the reported bug.
There was a problem hiding this comment.
Added in b924da1. The SQL regression uses non-null a with x = 0 and checks that coalesce(a, y / x + random()) returns a without dividing by zero. An EXPLAIN assertion also checks that the volatile final argument keeps the CASE rewrite.
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for addressing the follow-up comments. The new SQL regression confirms that a volatile final argument preserves lazy CASE evaluation, including when the unused branch contains division by zero.
I also checked the upgrade-guide merge resolution and formatting. The compatibility note is accurate, the upstream sections are preserved, and the previously reported formatting issue is resolved.
All my review comments have been addressed, and I found no new issues. Approved, subject to the remaining CI and repository merge requirements.
|
@DeviousCardi |
alamb
added
the
Prolific Author
`CoalesceFunc::simplify` rewrites `coalesce(a, b)` into `CASE WHEN a IS NOT NULL THEN a ELSE b END`. That rewrite names every non-final argument *twice* -- once in the `WHEN` predicate and once in the `THEN` result -- so for a volatile argument the two mentions are two independent draws. Since apache#17357 removed the runtime kernel (`invoke_with_args` was left as `internal_err!("coalesce should have been simplified to case")`), there was no other path and no volatility guard, so every volatile `coalesce` went through the duplicating rewrite. `nvl` delegates `simplify`/`invoke_with_args`/`short_circuits`/ `conditional_arguments` to `CoalesceFunc`, so it had the identical bug. Two observable symptoms, both reproduced in the new sqllogictests against the unpatched code: 1. Wrong results. Over 100000 rows, `count(coalesce(nullif(floor(random()*2), 0), -1))` returned 75112 instead of 100000 -- 24888 impossible NULLs, exactly the 0.5*0.5 two-draw rate, from rows where the `WHEN` draw was non-null but the independent `THEN` draw was null. 2. A hard failure. `return_field_from_args` marks the result non-nullable when any argument is non-nullable (here the `-1` fallback), so those impossible NULLs also trip `Arrow error: Invalid argument error: Column 'c' is declared as non-nullable but contains null values`. Fix (two parts, one file): * Guard `simplify`: if any argument `is_volatile()`, return `ExprSimplifyResult::Original` and leave the `coalesce` call intact. Single-argument `coalesce` is still unwrapped, volatile or not, because it is named only once. * Restore the runtime kernel in `invoke_with_args`, recovered from e5dcc8c (`new_null_array` + `is_not_null`/`is_null` + `zip`). It walks the already-evaluated `ColumnarValue`s and therefore evaluates each argument exactly once. Why this rather than a new physical expression (apache#25476): this is ~60 lines in a single file with no new public API, no new physical expression and no protobuf change, which answers the complexity objection raised on that PR directly. Nothing outside `coalesce.rs` moves. Trade-off -- the restored kernel is eager, which is exactly what apache#17357 removed, so this is deliberately confined to the volatile case: * Non-volatile `coalesce` is untouched and keeps the lazy `CASE` rewrite. `select.slt:1686` (`select coalesce(1, y/x)` with `x = 0`) still plans to `Projection: Int64(1)` and still never divides by zero. * Volatile `coalesce` is now eager, so `coalesce(random(), y/x)` will evaluate `y/x` and can raise divide-by-zero where it previously did not. That is accepted: today the same query silently returns wrong answers (or aborts with the Arrow nullability error above), and a surfaced error beats a silently wrong result. It also only restores the pre-apache#17357 behaviour, and only for the narrow slice of calls that actually contain a volatile argument. There is no way to get both per-row laziness and single-evaluation out of the `CASE` rewrite, because the rewrite is what duplicates the argument. `short_circuits()` stays `true` and `conditional_arguments` is unchanged. Its contract is "some subexpressions *may* not be evaluated", so an eager kernel does not violate it -- common-subexpression elimination simply declines to hoist out of the lazy arguments, which costs an optimization and never correctness. Tests: unit tests for the restored kernel (arrays, array + scalar fallback, all scalars, all-null scalars, all-null arrays, empty args) and for the simplify guard (non-volatile still expands to `CASE`; direct, nested and single-argument volatile cases); sqllogictests asserting the deterministic `count(c) = count(*)` row count, that no value comes from outside the two operands, and `EXPLAIN` output pinning that the volatile shape stays `coalesce(...)`/`nvl(...)` and does not expand to `CASE`, while the non-volatile shape still does. Closes apache#25477. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The volatility guard added in the previous commit tested every argument, but the `CASE` rewrite duplicates only the non-final ones: `args[..n-1]` go into both `whens` and `cases`, while `args[n-1]` appears once as the `ELSE`. A volatile last argument therefore has no double evaluation to avoid, and disabling the rewrite for it made the whole call eager for no benefit. That was a regression on queries that worked before. With `a` never NULL, `select coalesce(a, y/x + random()) from nt` returned rows on the parent of the previous commit and raised `Divide by zero error` after it, because `y/x` stopped being skipped. Restricting the guard to `args[..n-1]` keeps those queries lazy and still fixes the reported bug, whose volatile argument is not the last one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`nvl`'s user documentation promised that the second argument "is not evaluated". That stops being true once the volatility guard skips the `CASE` rewrite, because `ScalarFunctionExpr::evaluate` evaluates every child before calling `invoke_with_args`, and nothing in the physical layer defers an argument. `coalesce` promised nothing either way, but the new behaviour is worth stating there too. `scalar_functions.md` is generated from these `#[user_doc]` attributes by `dev/update_function_docs.sh`, and CI fails on drift, so the regenerated file is included here. Also adds a 56.0.0 upgrade-guide entry, as `api-health.md` asks for user-visible SQL changes, modelled on 54.0.0's evaluation-order section and pointing at the same `CASE` workaround. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`coalesce(a, y / x + random())` with a non-null `a` and `x = 0` must still take the lazy CASE rewrite and return `a` without evaluating the division. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y1b55sEL1gA1c3FCMcQCbm
b924da1 to
146a3eb
Compare
|
Rebased onto current main and resolved the 56.0.0 upgrade-guide conflict, preserving the upstream sections and the coalesce/nvl note. The updated branch is at 146a3eb. Local checks passed: cargo fmt --all -- --check, cargo clippy --all-targets --all-features -- -D warnings, and the documentation Prettier check. CI is running on the new head. |
Which issue does this PR close?
Rationale for this change
coalesceis rewritten bysimplifyintoCASE WHEN a IS NOT NULL THEN a ELSE b END, which names every argument but the last one twice. For a volatile argument those two mentions are two independent draws, so the null test and the returned value disagree:24,883 NULLs, which is exactly
P(draw1 != 0) * P(draw2 == 0) = 0.5 * 0.5. With a non-null literal as the last argument the planner marks the output non-nullable and it fails outright:nvldelegates toCoalesceFunc, so it has the same bug.What changes are included in this PR?
simplifynow returns the expression unchanged when any ofargs[..n-1]is volatile, andinvoke_with_argsgets its runtime kernel back — the one removed in e5dcc8c (#17357), which evaluates each argument exactly once. Only the non-final arguments are guarded, since the last one becomes theELSEand is named once.This is deliberately smaller than the
BetweenExprapproach in #25476: no new physical expression, no protobuf, no public API change.coalescealready has the right place to evaluate once —invoke_with_args— it was just stubbed out with aninternal_err!.nvl2is unaffected and needs no guard: itssimplifynamestest,if_non_nullandif_nullexactly once each.What is the testing strategy for this PR?
11 unit tests in
coalesce.rscovering the kernel and the guard, plus volatile cases incoalesce.sltfor bothcoalesceandnvl— acount(c) = count(*)assertion that is exact rather than probabilistic, andEXPLAINassertions pinning that the volatile shape stayscoalesce(...)instead of expanding toCASE.Reverting only the source changes and keeping the tests fails 8 of 11 unit tests and 5
coalesce.sltassertions, including the plan diff showingrandom()named twice.Are there any user-facing changes?
Yes, and there is a behaviour change beyond the bug fix worth calling out.
A volatile
coalesce/nvlis now eager, so a later argument that would previously have been skipped is evaluated:Only calls with a volatile argument before the last are affected; a volatile last argument keeps the lazy rewrite, and non-volatile
coalesce/nvlis unchanged. Note that for a non-nullable volatile argument the old answer was already correct, so there the eagerness is a regression without a correctness gain — it is the price of evaluating the argument once.nvl's documentation previously said the second argument "is not evaluated", which this makes false; that description is corrected andscalar_functions.mdregenerated viadev/update_function_docs.sh. A 56.0.0 upgrade-guide entry is included, modelled on 54.0.0's evaluation-order section, pointing atCASEas the workaround.Written by Codex.