Skip to content

fix: do not evaluate a volatile coalesce or nvl argument twice - #25561

Open
DeviousCardi wants to merge 3 commits into
apache:mainfrom
DeviousCardi:fix/25477-coalesce-volatile-double-eval
Open

DeviousCardi wants to merge 3 commits into
apache:mainfrom
DeviousCardi:fix/25477-coalesce-volatile-double-eval

Conversation

@DeviousCardi

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

coalesce is rewritten by simplify into CASE 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:

-- both draws are independent, so COALESCE can return NULL,
-- which a single evaluation can never produce
SELECT count(*), count(c) FROM (SELECT coalesce(nullif(floor(random() * 2), 0), neg) AS c FROM w);
+--------+----------+
| 100000 | 75117    |
+--------+----------+

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:

Arrow error: Invalid argument error: Column 'c' is declared as non-nullable but contains null values

nvl delegates to CoalesceFunc, so it has the same bug.

What changes are included in this PR?

simplify now returns the expression unchanged when any of args[..n-1] is volatile, and invoke_with_args gets 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 the ELSE and is named once.

This is deliberately smaller than the BetweenExpr approach in #25476: no new physical expression, no protobuf, no public API change. coalesce already has the right place to evaluate once — invoke_with_args — it was just stubbed out with an internal_err!.

nvl2 is unaffected and needs no guard: its simplify names test, if_non_null and if_null exactly once each.

What is the testing strategy for this PR?

11 unit tests in coalesce.rs covering the kernel and the guard, plus volatile cases in coalesce.slt for both coalesce and nvl — a count(c) = count(*) assertion that is exact rather than probabilistic, and EXPLAIN assertions pinning that the volatile shape stays coalesce(...) instead of expanding to CASE.

Reverting only the source changes and keeping the tests fails 8 of 11 unit tests and 5 coalesce.slt assertions, including the plan diff showing random() 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/nvl is now eager, so a later argument that would previously have been skipped is evaluated:

-- previously returned rows; now raises `Divide by zero error`
SELECT coalesce(random(), y / x) FROM t;

Only calls with a volatile argument before the last are affected; a volatile last argument keeps the lazy rewrite, and non-volatile coalesce/nvl is 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 and scalar_functions.md regenerated via dev/update_function_docs.sh. A 56.0.0 upgrade-guide entry is included, modelled on 54.0.0's evaluation-order section, pointing at CASE as the workaround.

@github-actions github-actions Bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Sep 21, 2026
DeviousCardi and others added 3 commits September 21, 2026 08:58
`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>
@DeviousCardi
DeviousCardi force-pushed the fix/25477-coalesce-volatile-double-eval branch from 2ab95fc to da30a95 Compare September 21, 2026 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong results: COALESCE on a volatile operand evaluates it two times, and fails when the output is declared non-nullable

1 participant