[SPARK-58211][SQL] Peel all leading AND/OR operands in subexpression elimination shortcut

[ganeshashree]

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 raising a spurious error for a row where that operand should never have run.

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 subexpression. In a deliberately pathological microbenchmark, an N-way OR chain where every operand shares one from_json over a wide row and none short-circuits, I measured a ~3.5x slowdown at 50 operands and ~10x at 500. That worst case is narrow (it requires an expensive subexpression repeated across many operands that rarely short-circuit), and the config remains available as an opt-out, but the trade-off (hard failure vs. a potential CSE regression) seemed worth surfacing before changing a default. Happy to flip it here or send a follow-up, per reviewer preference.

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)

[cloud-fan]

0 blocking, 0 non-blocking, 0 nits.
The implementation and regression coverage are consistent with Spark's short-circuit evaluation semantics; no actionable findings.

Verification

Traced childrenToRecurse through the old one-level behavior and the new recursive behavior. Because .children is taken immediately after skipForShortcut, the old path exposed the returned inner node's conditional right operand, while the new path reaches the single leftmost non-And/Or expression before selecting children. The unit expectations and SQL queries exercise that exact difference.

[cloud-fan]

Merge Summary:

• merged into master 89d2d91
• merged into branch-4.x 726d8a4

Posted by merge_spark_pr.py