MaxGekk opened a new pull request, #59984:
URL: https://github.com/apache/spark/pull/59984
### What changes were proposed in this pull request?
With whole-stage codegen disabled, `FilterExec` evaluates its condition
through `Predicate.create`, which under the default factory mode builds a
`GeneratePredicate` with subexpression elimination. `GeneratePredicate`
evaluated every common subexpression of the condition before the condition
itself (`subexprFunctionsCode`), so a subexpression shared only by conjuncts
after a FALSE conjunct was evaluated on a row that `And.eval` drops at that
conjunct, and an error it raised failed the query only with whole-stage codegen
off.
This PR makes `GeneratePredicate` evaluate a common subexpression just
before the first top-level conjunct that references it, as the CSE branch of
`FilterExec.doConsume` does with whole-stage codegen on (SPARK-56032):
- `CodegenContext.subexpressionElimination` now returns each common
subexpression with the call of the function that evaluates it, in dependency
order. `generateExpressions` still adds the calls to `subexprFunctionsCode`, so
every other caller is unchanged.
- New `CodegenContext.generateExpressionsWithSubexprCodes` generates a
sequence of expressions with subexpression elimination and returns, per
expression, the calls of the subexpressions it is the first to reference, to be
put right before its code. A subexpression is still evaluated before the
expressions that reference it.
- `GeneratePredicate.create` splits the predicate into its top-level
conjuncts. When subexpression elimination is on, there are several conjuncts,
and they have a subexpression in common, it generates each conjunct with the
subexpressions it is the first to reference before it, and evaluates them like
`And.eval`: a FALSE conjunct returns false, a NULL one sets a flag and the
evaluation goes on, and the predicate is true only if all conjuncts are.
Otherwise the generated code is unchanged.
The subexpression elimination itself does not change: a subexpression common
to several conjuncts, or repeated within one, is still evaluated once. Only its
place in the generated code moves, from before the condition to before the
first conjunct that references it.
The alternative, the internal config
`spark.sql.subexpressionElimination.skipForShortcutExpr` (SPARK-42815), avoids
the error by excluding everything after the first operand of an `AND`/`OR`
chain from subexpression elimination, so a filter condition gets no elimination
past its first conjunct. Emitting the subexpression at the first use site
inside the generated `And` code was rejected too: a subexpression common to all
branches of a conditional expression is used in several places, and only the
first would carry the code.
### Why are the changes needed?
The same query must not fail with whole-stage codegen off and return rows
with it on (SPARK-60332). Reproducer (ANSI mode):
```sql
CREATE TABLE t USING parquet AS
SELECT * FROM VALUES (NULL, 1, 2147483647, NULL), (0, 1, 0, 1) AS v(a, b,
y, x);
SET spark.sql.parquet.filterPushdown=false;
SET spark.sql.codegen.wholeStage=false;
SELECT a FROM t WHERE x IS NOT NULL AND NOT (a > 0 AND b > 0) AND y + 1 > 0
AND y + 1 > -1;
-- before: [ARITHMETIC_OVERFLOW] overflow. ... SQLSTATE: 22003
-- after: 0
SET spark.sql.codegen.wholeStage=true;
SELECT a FROM t WHERE x IS NOT NULL AND NOT (a > 0 AND b > 0) AND y + 1 > 0
AND y + 1 > -1;
-- before and after: 0
```
On the first row `isnotnull(x)` is FALSE, so `And.eval` never reaches `y +
1`, but the common `y + 1` was evaluated up front and overflowed. The same
happens without a NULL conjunct, e.g. with `b < 0` as the first conjunct.
This is independent of SPARK-60315 (a NULL conjunct stopping the evaluation
with codegen on) and of SPARK-60329 (an `IS NOT NULL` check generated later
than its position with codegen on): the inconsistency here is in the opposite
direction, codegen off raised where codegen on drops the row. The common
subexpressions of the update expressions of an aggregate function with a
`FILTER` clause are a separate problem, SPARK-60393.
### Does this PR introduce _any_ user-facing change?
Yes, a bug fix. A predicate evaluated through `Predicate.create` outside of
whole-stage codegen, that is a filter condition with whole-stage codegen
disabled, or a join condition on the non-codegen paths of the hash, sort-merge,
broadcast nested loop and cartesian product joins, no longer raises an error
from a subexpression shared by conjuncts after a FALSE one on a row that the
FALSE conjunct drops, as `And.eval` does not. For example, `SELECT a FROM t
WHERE b < 0 AND y + 1 > 0 AND y + 1 > -1` no longer fails with
`ARITHMETIC_OVERFLOW` on a row where `b` is `1` and `y` is `2147483647`.
Results for rows that pass the earlier conjuncts do not change. A subexpression
whose first occurrence is in a conditional branch of a conjunct, such as a
`CASE WHEN` branch, is still evaluated before that conjunct, as the CSE branch
of `FilterExec` does with whole-stage codegen enabled. The migration guide has
an entry for this, and the SPARK-60315 entry no longer says that codegen off
evaluates
a shared subexpression before the condition.
### How was this patch tested?
- A new test in `CodegenSubexpressionEliminationSuite`: a
`GeneratePredicate` over `And(And(first, Not(leaf)), Not(leaf))`, with and
without subexpression elimination, checks that `leaf` is not evaluated when
`first` is FALSE, and is evaluated, with the result of `And.eval`, when `first`
is TRUE or NULL. `ExprWithEvaluatedState` takes its value and nullability as
parameters for that.
- A new test in `WholeStageCodegenSuite`, next to the SPARK-60315 tests: the
reproducer above, the `b < 0` variant and a NULL-conjunct variant, with
whole-stage codegen on and off and the legacy config on and off. The reproducer
and the `b < 0` variant drop the row in every combination; the NULL-conjunct
variant raises, except with the legacy config and codegen on, which skips the
row.
```
build/sbt 'catalyst/testOnly *SubexpressionEliminationSuite
*CodeGenerationSuite *PredicateSuite *CodegenSubexpressionEliminationSuite
*CodegenExpressionCachingSuite *CodeCompilerSuite'
build/sbt 'sql/testOnly *WholeStageCodegenSuite *SQLQuerySuite'
```
### 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]
