MaxGekk opened a new pull request, #59985:
URL: https://github.com/apache/spark/pull/59985
### What changes were proposed in this pull request?
With whole-stage codegen on, `FilterExec` and the `FILTER` clause of an
aggregate function (`GeneratePredicateHelper.generatePredicateCode`) generate
an `IsNotNull` check lazily: just before the first conjunct that references its
column, or after all conjuncts. With codegen off, `And.eval` evaluates the
condition in its order. So a FALSE check cannot drop a row before a later
conjunct that raises on it under codegen.
This PR extends `precedingNotNullChecks` (SPARK-60315) with a rule that
applies with or without the NULL-conjunct flag: before a conjunct that
`mayRaise`, the pending checks written in the query that precede it in the
condition and cannot raise themselves (`ExprUtils.canEvaluateUnconditionally`),
such as a check on a column or on a struct field, are generated before it, in
both `generatePredicateCode` and the CSE branch of `FilterExec.doConsume`. Such
a check only drops a row before an error, as `And.eval` does.
Two kinds of checks keep the lazy placement:
- A check that `InferFiltersFromConstraints` inferred. It is implied by the
conjunct it was inferred from, so the lazy placement, just before that
conjunct, does not load its column earlier. Generating it at its position would
decode the column on every row before a conjunct that can raise, which
`mayRaise` reports for any function, cast or arithmetic: a filter `WHERE
upper(name) = 'X' AND d > 5` over 20M Parquet rows with a `DECIMAL(38, 10)`
column `d` went from 314 ms to 520 ms with that placement, and is back to 316
ms without it. The rule marks the predicates it adds with a `TreeNodeTag`,
`InferFiltersFromConstraints.INFERRED`, which the codegen reads; the tag
survives the optimizer's copies of the predicate.
- A check on an expression that can raise itself, such as `cast(s AS INT) IS
NOT NULL`, past a conjunct that may reject the row (SPARK-56032).
Both are generated at their position after a conjunct that can be NULL, as
before (SPARK-60315). A check written after the raising conjunct stays after it.
The synthetic `IsNotNull` check on a column, for a column covered only by a
check on an expression, is now generated only when no direct check on the
column exists (`idx == -1`): a direct check that is generated always precedes
the conjunct in the generated code, so the synthetic one was dead code there.
This drops a redundant check from the generated code of a condition that
references a column in two conjuncts, such as `BETWEEN`.
`spark.sql.legacy.filterShortCircuitOnNullConjunct` keeps controlling only
the NULL conjunct; the placement applies in both settings, since the issue is
independent of a NULL conjunct.
### Why are the changes needed?
The query should not fail only with whole-stage codegen on (SPARK-60329).
ANSI mode, Parquet filter pushdown off:
```sql
CREATE TABLE t USING parquet AS
SELECT * FROM VALUES (NULL, 1, 2147483647, NULL), (0, 1, 0, 1) AS v(a, b,
y, x);
SELECT a FROM t WHERE x IS NOT NULL AND y + 1 > 0 AND x > 0;
-- codegen on, before: [ARITHMETIC_OVERFLOW] overflow. SQLSTATE: 22003
-- codegen on, after: 0
-- codegen off: 0
```
With codegen on, `isnotnull(x)` was generated just before `x > 0`, after the
overflowing conjunct. The inferred check keeps that placement, so `SELECT a
FROM t WHERE y + 1 > 0 AND x > 0` still raises with codegen on; the migration
guide lists it.
### Does this PR introduce _any_ user-facing change?
Yes. With whole-stage codegen on, an `IS NOT NULL` check written in the
query that cannot raise is evaluated before a later conjunct that can raise, so
a row the check rejects no longer raises the later conjunct's error; the row
was dropped with codegen off already. An inferred check, and a check on an
expression that can raise, keep the previous placement. Results for rows that
pass the checks do not change. The migration guide has an entry.
### How was this patch tested?
A new test in `WholeStageCodegenSuite`, with codegen on and off and the
legacy config on and off: a check written in the query with and without a later
conjunct on the column, the same through the CSE branch of `FilterExec` (a
common `b * 2`, checked from the generated code), a check on a struct field, a
check that can raise (`cast(s AS INT) IS NOT NULL`) that keeps the lazy
placement, a check written after the raising conjunct that still raises, the
inferred check that still raises with codegen on and drops the row with codegen
off, and the aggregate `FILTER` clause. Before the fix, the codegen-on cases
with a check written in the query raised `ARITHMETIC_OVERFLOW`. The
SPARK-60315, SPARK-51356 and SPARK-56032 tests pass unchanged, as do
`InferFiltersFromConstraintsSuite`, `FilterPushdownSuite`,
`DataFrameAggregateSuite` and `SQLQuerySuite`.
```
build/sbt 'catalyst/testOnly *InferFiltersFromConstraintsSuite
*FilterPushdownSuite'
build/sbt 'sql/testOnly *WholeStageCodegenSuite *DataFrameAggregateSuite
*SQLQuerySuite'
```
The benchmark above ran a filter over 20M Parquet rows, codegen on, Parquet
pushdown and AQE off, median of 7 runs after 3 warm-ups, against the parent
commit: `upper(name) = 'X' AND d > 5` 314 ms vs 316 ms, `y > 98 AND d > 5` 192
ms vs 195 ms, `upper(name) = 'X' AND y > 5` 204 ms vs 221 ms.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Fable 5.1)
🤖 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]
