LuciferYang opened a new pull request, #58045:
URL: https://github.com/apache/spark/pull/58045
### What changes were proposed in this pull request?
`RewriteWithExpression` inlines a `With` sitting in a conditional branch by
blind substitution, without the reference-count and cheapness checks the main
branch applies. When the common expression is nondeterministic, each reference
then evaluates it separately and gets a different value, which is exactly what
`With` exists to prevent.
This PR pre-evaluates such a common expression in a project instead, but
only when it cannot raise. Pre-evaluating runs the expression for every row,
including the rows whose branch is not taken, so an expression that raises
would trade a wrong result for a spurious error.
That safety condition rules out more than it may seem. `randstr(-1, 0)`
raises on its constant length, and `reflect('java.lang.Integer', 'parseInt',
'abc')` raises on constant arguments, so neither "the nondeterministic node is
the root" nor "all children are foldable" is sufficient. The guard therefore
admits `rand`/`randn`, whose only child is a seed the analyzer already requires
to be foldable, and the nondeterministic leaves (`uuid`,
`monotonically_increasing_id`, `spark_partition_id`, `input_file_name`, the
input-file-block pair), which evaluate no arguments at all.
**This is a partial fix.** A nondeterministic expression wrapped in anything
else, `rand() / col` for example, keeps the existing inlining and its existing
wrong result. Covering it needs a way to evaluate a branch lazily, not a wider
guard here — I measured what a wider guard does and it converts the wrong
result into a `DIVIDE_BY_ZERO` on rows whose branch was never taken, which is
not an improvement.
### Why are the changes needed?
The corrupt evaluation is a silent wrong result reachable from ordinary SQL
with default configuration. `BETWEEN` references its input twice, and the
parser produces `Between` by default (`spark.sql.legacy.duplicateBetweenInput`
is false), so:
```sql
SELECT CASE WHEN id < 0 THEN false
ELSE monotonically_increasing_id() BETWEEN 3 AND 5 END AS r
FROM range(0, 10, 1, 1)
```
returns 6 `true` rows instead of 3. Each reference got its own counter, and
the second one advanced only on the rows where the first predicate passed. The
optimized plan shows the duplication:
```
Project [CASE WHEN (id < 0) THEN false
ELSE ((monotonically_increasing_id() >= 3) AND
(monotonically_increasing_id() <= 5)) END]
```
`rand()` is wrong the same way. Over 200k rows, `CASE WHEN id < 0 THEN false
ELSE rand(42) BETWEEN 0.4 AND 0.6 END` selects 36.0% of rows where the correct
rate is 20.0%; written outside a conditional branch the same predicate gives
20.1%.
`NullIf` builds a `With` of the same shape, so it is affected wherever it
appears in a conditional branch.
### Does this PR introduce _any_ user-facing change?
Yes, it fixes the wrong results above. A query whose conditional branch
contains `rand`, `randn`, `uuid`, `monotonically_increasing_id`,
`spark_partition_id` or `input_file_name` under a `With` now evaluates it once
per row rather than once per reference.
One consequence is worth naming: those expressions are now evaluated for
every row, including rows whose branch is not taken. For a generator this only
advances the RNG or the counter, but it does mean
`monotonically_increasing_id()` values and `rand()` draws shift relative to the
previous behavior. That behavior was already wrong, so there is nothing to
preserve, and the same expression outside a conditional branch has always been
pre-evaluated this way.
### How was this patch tested?
New tests, plan-level and end-to-end, both written against the unfixed tree
first and confirmed to fail there:
- `RewriteWithExpressionSuite`: one `comparePlans` test with five cases —
`rand()` is pre-evaluated; a deterministic common expression in the same
position is still inlined; `randstr(-1, 0)` stays inlined because it can raise;
`rand() / a` stays inlined because it reads row data; a single reference stays
inlined because it evaluates once either way.
- `ColumnExpressionSuite`: a `checkAnswer` test on the
`monotonically_increasing_id()` shape above, with the non-branch form as a
control.
On the unfixed tree the catalyst test reports the two `rand(1)` copies, and
the end-to-end test reports 3 wrong rows out of 10.
Mutation-tested each half of the guard: flipping the fallback to admit
everything, dropping the `rand`/`randn` arm, and dropping the reference-count
check each fail at least one of the five cases.
Also verified with throwaway probes (deleted) that a `randstr(-1, 0)` and a
`reflect(...)` in a never-taken branch behave the same before and after, that
the fix produces the right shape under `Filter`, a join condition and
`Aggregate`, and that a nested nondeterministic `With` terminates and is
idempotent.
Regressions: `catalyst/testOnly org.apache.spark.sql.catalyst.optimizer.*
NullExpressionsSuite ConditionalExpressionSuite RandomSuite` (1437 passed),
`sql/testOnly org.apache.spark.sql.ColumnExpressionSuite
org.apache.spark.sql.DataFrameFunctionsSuite` (315 passed).
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5
--
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]
