MaxGekk opened a new pull request, #59573:
URL: https://github.com/apache/spark/pull/59573
### What changes were proposed in this pull request?
Whole-stage codegen of a filter condition (`FilterExec`) and of the `FILTER`
clause of an aggregate function (`AggregateCodegenSupport`) generates each
top-level conjunct separately and drops the row as soon as one of them is FALSE
**or NULL** (`if (isNull || !value) continue;`). The interpreted path evaluates
the whole condition with `And.eval`, which stops only at FALSE. So when an
earlier conjunct is NULL and a later one raises on that row, the query fails
with codegen off and returns rows with it on.
This PR changes the generated code so that only a FALSE conjunct stops the
evaluation:
- `GeneratePredicateHelper` gets three helpers: `nullConjunctFlag` (a fresh
flag name, or `None`), `conjunctCheck` (the per-conjunct check) and
`withNullConjunctFlag` (declares the flag before the checks and drops the row
after them if it is set).
- A NULL conjunct sets the flag, and the remaining conjuncts are still
evaluated: `if (isNull) { flag = true; } else if (!value) continue;`. The row
is dropped after all of them: `if (flag) continue;`.
- The flag is generated only when some conjunct can be NULL after binding.
The `IsNotNull` checks are never NULL and keep their early `continue`, so the
tightened-nullability binding of the later conjuncts, and of the CSE
precomputes, stays safe.
- Both code paths are changed: `generatePredicateCode`, which the aggregate
`FILTER` clause also uses, and the CSE branch of `FilterExec.doConsume`, which
the code comments require to stay in step with it.
- New legacy config `spark.sql.legacy.filterShortCircuitOnNullConjunct`
(internal, default `false`, version 4.4.0). Set to `true`, it restores the
previous codegen behavior. The interpreted path is unchanged in both modes.
Join conditions and `MergeRowsExec` generate the whole condition through
`And.doGenCode`, which already evaluates the right side after a NULL left side,
so they are not changed.
#### SQL standard
- 7.11 `<where clause>`, GR 2 (and 10.9 `<aggregate function>`, GR 4a, for
`<filter clause>`): the search condition is effectively evaluated for each row,
and the result keeps the rows for which it is True.
- 6.35 `<boolean value expression>`, Table 11: Unknown AND True is Unknown,
Unknown AND False is False. After an Unknown conjunct the result of the search
condition still depends on the remaining ones.
- 9075-1 "Rule evaluation order" (SQL-92 3.3.4.4): only the *inessential*
parts, those not needed to determine the result of the expression or search
condition, may be skipped, and whether their exceptions are raised is
implementation-dependent. After a NULL conjunct the rest is essential. The
order of evaluation is implementation-dependent too, but in every order the
raising conjunct is either evaluated first or is still essential, so the
exception is raised. Only a FALSE conjunct makes the rest inessential.
#### Other DBMSes
Table `t(a INT, b INT, y)` with rows `(NULL, 1, <raises>)` and `(0, 1,
<ok>)`. `<E>` raises on the first row:
- `y + 1 > 0` with `y` at the maximum of its type: `INT`, or `BIGINT` on
MySQL and MariaDB, which widen `INT + INT`;
- `10 / y > 0` with `y = 0` on Oracle, whose `NUMBER` does not overflow at
32 bits;
- `abs(y) > 0` with `y` at the minimum `BIGINT` on SQLite, where `+`
switches to floating point.
The images and credentials are those of `connector/docker-integration-tests`.
| Query | PostgreSQL 18.2 | MySQL 9.6.0 | MariaDB 12.2.2 | SQL Server 2022 |
Oracle Free 23.26 | DB2 11.5.9 | H2 2.3.232 | Derby 10.16 | SQLite 3.46 |
|---|---|---|---|---|---|---|---|---|---|
| `WHERE NOT (a > 0 AND b > 0) AND <E>` | skip | skip | skip | **raise** |
skip | **raise** | **raise** | skip | skip |
| `WHERE <E> AND NOT (a > 0 AND b > 0)` | raise | raise | raise | raise |
**skip** | raise | raise | **skip** | raise |
| `WHERE a > 0 AND <E>` | skip | skip | skip | skip | skip | **raise** |
**raise** | skip | skip |
| `WHERE b < 0 AND <E>` (FALSE first) | - | - | - | - | - | raise | - | - |
- |
| `count(a) FILTER (WHERE NOT (a > 0 AND b > 0) AND <E>)` | **raise** | n/a
| n/a | n/a | **raise** | n/a | **raise** | n/a | skip |
"-" means no error and an empty result; "n/a" means the `FILTER` clause is
not supported.
- In WHERE, most engines drop the row at the NULL conjunct. Oracle and Derby
do it even when the raising conjunct is written first, so they reorder.
- DB2 and H2 raise. SQL Server raises for the JIRA's shape but not for a
plain `a > 0` first.
- For the aggregate `FILTER` clause, PostgreSQL, Oracle and H2 evaluate the
whole `AND` and raise, as this PR does.
We considered the opposite fix: make the interpreted path stop at the first
NULL conjunct too. It matches most engines' WHERE behavior, but it departs from
the standard's text and from what Spark already does with codegen off. That
path is kept available through the legacy config.
### Why are the changes needed?
The same query must not raise with whole-stage codegen off and return rows
with it on (SPARK-60315). Reproducer (ANSI mode):
```sql
CREATE TABLE t USING parquet AS
SELECT * FROM VALUES (NULL, 1, 2147483647), (0, 1, 0) AS v(a, b, y);
SET spark.sql.codegen.wholeStage=true;
SELECT a FROM t WHERE NOT (a > 0 AND b > 0) AND y + 1 > 0;
-- before: 0
-- after: [ARITHMETIC_OVERFLOW] overflow. ... SQLSTATE: 22003
SET spark.sql.codegen.wholeStage=false;
SELECT a FROM t WHERE NOT (a > 0 AND b > 0) AND y + 1 > 0;
-- before and after: [ARITHMETIC_OVERFLOW] overflow. ... SQLSTATE: 22003
```
The same happens in non-ANSI mode with year-month interval arithmetic, and
with the aggregate `FILTER` clause.
### Does this PR introduce _any_ user-facing change?
Yes. With whole-stage codegen on, an error raised by a later conjunct on a
row where an earlier conjunct is NULL now fails the query, as it already did
with codegen off. A row can still be skipped before the filter, for example by
Parquet row-group statistics, and then no error is raised.
The later conjuncts also run on more rows than before:
- a nondeterministic conjunct such as `rand(seed)` can return different
values on the remaining rows;
- a side-effecting UDF is called on rows that are then dropped;
- an expensive later conjunct costs more on rows where an earlier, nullable
conjunct is NULL.
Rows whose earlier conjunct is FALSE are not affected. Setting
`spark.sql.legacy.filterShortCircuitOnNullConjunct=true` restores the previous
behavior. The migration guide has an entry for this.
### How was this patch tested?
Four new tests in `WholeStageCodegenSuite`. Each runs with whole-stage
codegen on and off, and with the legacy config on and off; it expects the row
to be skipped only with codegen on and the legacy config set, and the error in
every other combination:
- the ANSI reproducer, and a variant with a common `y + 1` that takes the
CSE path of `FilterExec` (checked from the generated code, with
`subexpressionElimination.filterExec.enabled` on and off);
- the non-ANSI year-month interval reproducer
(`INTERVAL_ARITHMETIC_OVERFLOW`);
- a NULL conjunct followed by a FALSE one, which must not raise; it also
checks that no flag is generated when no conjunct can be NULL;
- the aggregate `FILTER` clause.
The tests use a Parquet table with filter pushdown off, so the filter is
neither evaluated on a local relation by the optimizer nor skipped by row-group
statistics.
Also ran `WholeStageCodegenSuite`, `SQLQuerySuite`, `DataFrameSuite`,
`DataFrameAggregateSuite` and `SQLQueryTestSuite` locally. The only failure is
`udtf/udtf.sql - Python UDTFs`, whose golden output differs from the local
Python's `ValueError` message (`'input' is not in list` vs `list.index(x): x
not in list`), which is unrelated to this change.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
