LuciferYang opened a new pull request, #58868: URL: https://github.com/apache/spark/pull/58868
### What changes were proposed in this pull request? `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`, a `HigherOrderFunction` only its always-evaluated arguments, and a `With` binds references that cannot leave its scope. 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. Only the `With` case had a re-check after peeling. The always-evaluated operand is now computed once in `updateExprTree`, and both `childrenToRecurse` and `commonChildrenToRecurse` are asked about that operand. That removes the `With` re-check, and 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. One consequence beyond stopping the leak: master reached a peeled-to conditional's branches by recursing into all of its children, which incidentally gave a subexpression shared by every branch a use count of 2, so it was eliminated. That now goes through the branch-group intersection and lands at 1 unless the subexpression also occurs in an always-evaluated position, which is how a conditional met directly has always behaved. So a conditional behind an `And`/`Or` chain can lose an elimination that master made by the route this PR removes. Nothing changes while the config is off, where `skipForShortcut` returns its argument unchanged. ### 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 becomes a candidate, met directly and behind an `And`, an `Or` chain and a mixed chain, with the branch body's subexpression asserted absent from the recorded set rather than merely uneliminated. 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. ### 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]
