LuciferYang opened a new pull request, #58935: URL: https://github.com/apache/spark/pull/58935
### What changes were proposed in this pull request? This backports #58868 to `branch-4.3`. One thing differs from the master change: `childrenToRecurse` here has no `With` case, because SPARK-58818 is not on this branch, so there is no after-peel re-check to remove and the `With` half of the master change does not apply. The rest is the same. `EquivalentExpressions.childrenToRecurse` refuses to descend into children that must not be evaluated ahead of time: a `CodegenFallback` generates no code for them, a `ConditionalExpression` offers only its `alwaysEvaluatedInputs`, and a `HigherOrderFunction` only its always-evaluated arguments. With `spark.sql.subexpressionElimination.skipForShortcutExpr` on, `skipForShortcut` peels the leading `And`/`Or` operands to reach the one operand of the chain that is always evaluated. `And`/`Or` are not `ConditionalExpression`s, so that peel walked straight past every case above and the result was used as `peeled.children`, recursing into all of the peeled expression's children, conditional branches included. The always-evaluated operand is now computed once in `updateExprTree`, and both `childrenToRecurse` and `commonChildrenToRecurse` are asked about that operand. Asking `commonChildrenToRecurse` recovers what the peel used to drop: the branch groups of a conditional the peel lands on, whose shared subexpression is safe to evaluate once because the conditional itself always runs. `childrenToRecurse` still peels the always-evaluated inputs of a `ConditionalExpression`. That peel no longer keeps the descent safe: `updateExprTree` peels before it descends, so `childrenToRecurse` is asked about the peeled node either way. It stays because dropping it would pass `updateExprTree` the input itself rather than the operand its peel lands on. One consequence beyond stopping the leak: this branch reached a peeled-to conditional's branches by recursing into all of its children, so a subexpression shared by every branch picked up a use count of 2 and was eliminated. It now goes through the branch-group intersection, which contributes 1, and is eliminated only if it also occurs in an always-evaluated position, which is how a conditional met directly has always behaved. Where each branch holds the subexpression once, that elimination saved nothing, since only one branch runs. It is a real loss only when one branch body holds the subexpression more than once and every other branch holds it too. Nothing changes while the config is off, where `skipForShortcut` returns its argument unchanged and the recursion meets the conditional directly, guards and all. ### Why are the changes needed? The config exists to stop a short-circuited operand from being evaluated eagerly, and it made a conditional branch behind that operand eligible instead. With ANSI mode and both subexpression elimination configs on, `select (case when id = 0 then false else (1 / id + 1 / id) > 0 end) and id >= 0 from range(0, 1, 1, 1)` raised `[DIVIDE_BY_ZERO]`. `1 / id` is repeated inside a single branch body, is shared with no other branch, and should run only when that branch runs; for id = 0 the other branch does. The bypass reaches the `HigherOrderFunction` case too, where the cost is a failure rather than an early evaluation: a subexpression repeated inside a lambda body becomes a candidate, `supportedExpression` does not stop it because `NamedLambdaVariable` carries no `LAMBDA_VARIABLE` pattern, and generating that candidate at the top of the projection leaves `CodegenContext.getLambdaVar` with no variable to bind. No query was found where that changes an answer, so that half closes a hole rather than fixing an observed failure. ### Does this PR introduce _any_ user-facing change? Yes, for a query run with `spark.sql.subexpressionElimination.skipForShortcutExpr` enabled, which is off by default: the query above returns `false` where it used to raise `[DIVIDE_BY_ZERO]`. No change with the config off. ### How was this patch tested? `SubexpressionEliminationSuite` pins that no subexpression from a conditional branch, a `CodegenFallback`'s children or a lambda body is eliminated, with each shape met directly and behind an `And`, an `Or` chain and a mixed chain. Two positive controls sit beside it: the operand the peel lands on still contributes its own duplicates, and a duplicate shared by every branch of an `If` or `CaseWhen` group is still eliminated once, with the use count pinning that it came from the group rather than from recursing into every branch. `SQLQuerySuite` covers the query above, in both its `CASE WHEN` and `IF` spellings. ANSI mode is pinned on because that is what turns the extra evaluation into a failure; with it off the division returns null, the branch still yields false, and the case would pass either way in the scheduled non-ANSI build. The SPARK-58211 case beside it gains the same pin, which it was missing for the same reason. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5 -- 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]
