DeviousCardi opened a new pull request, #25561: URL: https://github.com/apache/datafusion/pull/25561
## Which issue does this PR close? - Closes #25477. ## 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: ```sql -- 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 e5dcc8c04 (#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: ```sql -- 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
