This is an automated email from the ASF dual-hosted git repository.
cloud-fan pushed a commit to branch branch-4.x
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-4.x by this push:
new 726d8a4e82f4 [SPARK-58211][SQL] Peel all leading AND/OR operands in
subexpression elimination shortcut
726d8a4e82f4 is described below
commit 726d8a4e82f4277dc2257b710c82f586d81f456b
Author: Ganesha Shreedhara <[email protected]>
AuthorDate: Wed Jul 22 10:50:17 2026 +0800
[SPARK-58211][SQL] Peel all leading AND/OR operands in subexpression
elimination shortcut
### What changes were proposed in this pull request?
`EquivalentExpressions.skipForShortcut` (used when
`spark.sql.subexpressionElimination.skipForShortcutExpr` is enabled) stripped
only a single And/Or operand. This PR makes it peel leading And/Or operands
recursively, down to the single always-evaluated leaf of the chain:
```
expr match {
case and: And => skipForShortcut(and.left)
case or: Or => skipForShortcut(or.left)
case other => other
}
```
The change is scoped to the existing skipForShortcutExpr code path and does
not change its default (false).
### Why are the changes needed?
And/Or short-circuit, so only the leftmost operand of a chain is guaranteed
to be evaluated. A chained predicate a AND b AND c is parsed left-deep as
And(And(a, b), c), so peeling only one level leaves the analysis recursing into
b (and, at the outer level, c) — treating conditionally-evaluated operands as
always-evaluated. A subexpression shared with such an operand can then be
hoisted into the common-subexpression set and evaluated eagerly, defeating
short-circuit semantics and rais [...]
For example, with subexpression elimination on:
```
SELECT id != 0 AND 1 / id > 0 AND 1 / id < 1 FROM range(0, 1, 1, 1);
```
For id = 0, id != 0 is false, so short-circuit should stop before 1 / id is
ever computed and the query should return false. Instead, the shared 1 / id
subexpression was hoisted and evaluated eagerly, raising [DIVIDE_BY_ZERO]. The
same mechanism produces a NullPointerException when the eagerly-evaluated
operand dereferences a null (the originally reported case). The pre-existing
one-level peel handled the two-operand case but still failed once there were
three or more operands.
If/CaseWhen already model this correctly via the ConditionalExpression
trait (recursing only alwaysEvaluatedInputs); this brings And/Or shortcut
handling in line for chains of arbitrary length.
**Note to reviewers:**
This PR deliberately keeps
`spark.sql.subexpressionElimination.skipForShortcutExpr` set to `false`, so the
fix only takes effect for users who opt in. Could reviewers weigh in on whether
the default should be flipped to `true`? With this fix, `true` is the
correct-by-default behavior (it never produces incorrect results and can only
eliminate spurious short-circuit errors), but it can miss
subexpression-elimination opportunities on wide AND/OR chains that repeat an
expensive subexpres [...]
### Does this PR introduce _any_ user-facing change?
No. The behavior only changes when
`spark.sql.subexpressionElimination.skipForShortcutExpr` is enabled (default
false), where it fixes incorrect eager evaluation for chains of three or more
AND/OR operands.
### How was this patch tested?
- SubexpressionEliminationSuite — a unit test asserting that a
subexpression in an operand past the first of a chained/deeply-nested
(4-way)/mixed AND/OR tree is not collected as a common subexpression, while the
always-evaluated leftmost leaf still is (guarding against over-conservatism).
- SQLQuerySuite — an end-to-end regression test running SELECT id != 0 AND
1 / id > 0 AND 1 / id < 1 FROM range(0, 1, 1, 1) with skipForShortcutExpr=true,
asserting it returns false with no error.
Also ran the surrounding suites to check for regressions:
SubexpressionEliminationSuite (21/21), WholeStageCodegenSuite (55/55), and
DataFrameFunctionsSuite (161/161) all pass.
New tests, both failing before the change and passing after:
- SubexpressionEliminationSuite — a unit test asserting that a
subexpression in an operand past the first of a chained/deeply-nested
(4-way)/mixed AND/OR tree is not collected as a common subexpression, while the
always-evaluated leftmost leaf still is (guarding against over-conservatism).
- SQLQuerySuite — an end-to-end regression test running SELECT id != 0 AND
1 / id > 0 AND 1 / id < 1 FROM range(0, 1, 1, 1) with skipForShortcutExpr=true,
asserting it returns false with no error.
Also ran the surrounding suites to check for regressions:
SubexpressionEliminationSuite (21/21), WholeStageCodegenSuite (55/55), and
DataFrameFunctionsSuite (161/161) all pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)
Closes #57368 from ganeshashree/SPARK-58211.
Lead-authored-by: Ganesha Shreedhara <[email protected]>
Co-authored-by: Ganesha S <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit 89d2d916e64318326e1416e1c96b060b1f6ad6d8)
Signed-off-by: Wenchen Fan <[email protected]>
---
.../expressions/EquivalentExpressions.scala | 10 ++++--
.../SubexpressionEliminationSuite.scala | 38 ++++++++++++++++++++++
.../scala/org/apache/spark/sql/SQLQuerySuite.scala | 21 ++++++++++++
3 files changed, 67 insertions(+), 2 deletions(-)
diff --git
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/EquivalentExpressions.scala
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/EquivalentExpressions.scala
index 8cc4ed7a9f9c..7722d5611670 100644
---
a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/EquivalentExpressions.scala
+++
b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/EquivalentExpressions.scala
@@ -133,9 +133,15 @@ class EquivalentExpressions(
if (skipForShortcutEnable) {
// The subexpression may not need to eval even if it appears more than
once.
// e.g., `if(or(a, and(b, b)))`, the expression `b` would be skipped if
`a` is true.
+ // `And`/`Or` short-circuit, so only the left operand is guaranteed to
be evaluated.
+ // A chained predicate `a AND b AND c` is left-deep, i.e. `And(And(a,
b), c)`, so we must
+ // keep peeling left operands until we reach the single always-evaluated
leaf. Peeling only
+ // one level would leave operands past the first (which are
conditionally evaluated) to be
+ // treated as always-evaluated, so a subexpression they share could be
hoisted and eagerly
+ // evaluated, breaking short-circuit semantics (e.g. a spurious NPE or
divide-by-zero).
expr match {
- case and: And => and.left
- case or: Or => or.left
+ case and: And => skipForShortcut(and.left)
+ case or: Or => skipForShortcut(or.left)
case other => other
}
} else {
diff --git
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubexpressionEliminationSuite.scala
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubexpressionEliminationSuite.scala
index e9faeba2411c..7971be98f0cd 100644
---
a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubexpressionEliminationSuite.scala
+++
b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/SubexpressionEliminationSuite.scala
@@ -495,6 +495,44 @@ class SubexpressionEliminationSuite extends SparkFunSuite
with ExpressionEvalHel
checkShortcut(Not(And(equal, Literal(false))), 1)
}
+ test("SPARK-58211: shortcut eliminate operand past the first in a chained
AND/OR") {
+ val add = Add(Literal(1), Literal(0))
+ val equal = EqualTo(add, add)
+
+ def checkShortcut(expr: Expression, numCommonExpr: Int): Unit = {
+ val e1 = If(expr, Literal(1), Literal(2))
+ val ee1 = new EquivalentExpressions(true)
+ ee1.addExprTree(e1)
+ assert(ee1.getCommonSubexpressions.size == numCommonExpr)
+
+ val e2 = expr
+ val ee2 = new EquivalentExpressions(true)
+ ee2.addExprTree(e2)
+ assert(ee2.getCommonSubexpressions.size == numCommonExpr)
+ }
+
+ // A left-deep chain `a AND b AND c` is `And(And(a, b), c)`. Only `a` is
always evaluated;
+ // `b` and `c` are short-circuited, so their subexpressions must not be
eliminated. Peeling
+ // a single operand would wrongly recurse into `b` and eliminate its inner
subexpression.
+ checkShortcut(And(And(Literal(false), equal), Literal(true)), 0)
+ checkShortcut(Or(Or(Literal(true), equal), Literal(false)), 0)
+ checkShortcut(And(And(Literal(false), Literal(true)), equal), 0)
+ checkShortcut(Or(Or(Literal(true), Literal(false)), equal), 0)
+
+ // The always-evaluated leftmost operand is still eligible for elimination.
+ checkShortcut(And(And(equal, Literal(false)), Literal(true)), 1)
+ checkShortcut(Or(Or(equal, Literal(true)), Literal(false)), 1)
+
+ // Deeper chain `a AND b AND c AND d` = `And(And(And(a, b), c), d)`. The
subexpression lives
+ // in a conditional operand (`c`), so it must not be eliminated no matter
the nesting depth.
+ checkShortcut(And(And(And(Literal(false), Literal(true)), equal),
Literal(true)), 0)
+
+ // Mixed nesting: the always-evaluated leaf `equal` is reached through a
left spine of both
+ // `And` and `Or`. Its internal subexpression (`add`, which appears twice
inside `equal`) is
+ // still eliminated, so the recursive peel is not over-conservative on the
leaf.
+ checkShortcut(And(Or(equal, Literal(true)), equal), 1)
+ }
+
test("Equivalent ternary expressions have different children") {
val add1 = Add(Add(Literal(1), Literal(2)), Literal(3))
val add2 = Add(Add(Literal(3), Literal(1)), Literal(2))
diff --git a/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
b/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
index 52cc8a3e92af..5786199cb376 100644
--- a/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
+++ b/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
@@ -3333,6 +3333,27 @@ class SQLQuerySuite extends SharedSparkSession with
AdaptiveSparkPlanHelper
}
}
+ test("SPARK-58211: subexpression elimination respects AND/OR short-circuit
with chained ANDs") {
+ // A subexpression (1 / id) repeated inside a short-circuited operand of a
chained AND/OR
+ // must not be hoisted and eagerly evaluated. For id = 0, `id != 0` is
false, so the second
+ // operand should never be evaluated and no divide-by-zero should be
raised. The subexpression
+ // is duplicated *inside* operand 2 (not split across operands 2 and 3) so
the failure shape
+ // does not depend on how many operands the one-level peel happened to
drop, and operand 3 is
+ // non-foldable to keep it in the plan. skipForShortcutExpr must peel
every leading AND/OR
+ // operand, not just one, to make this hold for three or more operands.
+ withSQLConf(
+ SQLConf.SUBEXPRESSION_ELIMINATION_ENABLED.key -> "true",
+ SQLConf.SUBEXPRESSION_ELIMINATION_SKIP_FOR_SHORTCUT_EXPR.key -> "true") {
+ val andQuery =
+ "select id != 0 and (1 / id + 1 / id) > 0 and id >= 0 from range(0, 1,
1, 1)"
+ checkAnswer(sql(andQuery), Row(false))
+
+ val orQuery =
+ "select id == 0 or (1 / id + 1 / id) > 0 or id < 0 from range(0, 1, 1,
1)"
+ checkAnswer(sql(orQuery), Row(true))
+ }
+ }
+
test("SPARK-29213: FilterExec should not throw NPE") {
// Under ANSI mode, casting string '' as numeric will cause runtime error
if (!conf.ansiEnabled) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
