LuciferYang opened a new pull request, #58845: URL: https://github.com/apache/spark/pull/58845
### What changes were proposed in this pull request? `RewriteWithExpression` hoists a common expression's definition into a child `Project`, so that every reference reads the one value the `With` promises. A definition reading columns from both sides of a join fits in no child, and the rule inlined it into every reference instead, because `With` could not be evaluated. It can now, so such a definition stays in the condition and memoizes per entry -- which is what a `With` in a conditional branch already does since SPARK-58818. The two TODOs the inlining carried go with it. The nondeterministic case the first one warns about is unreachable: a non-equi join condition that is not deterministic is rejected at analysis with `INVALID_NON_DETERMINISTIC_EXPRESSIONS`, so what inlining cost was a second evaluation per row pair, not a wrong answer. The second one asks for a reference count, which is `canSubstitute`'s question and is already asked before this point. The rest of the change is what leaving a `With` in a condition costs if nothing else moves. Each piece below was measured against the parent commit, and without it this PR is a plan regression rather than an improvement: - **`IsNotNull` inference reads a `With` as the expression it stands for.** `scanNullIntolerantAttribute` walks null-intolerant nodes down to attributes, and a `With` stops that walk twice over: it is not null intolerant itself, since its child decides, and its references are leaves. `splitConjunctivePredicates` cannot look inside one either, so the constraint stays a single `And`. Both places now substitute each definition into its references and read that -- the tree they saw before a `With` could survive. Without it, `left.join(right, "(v + w) BETWEEN 1 AND 10")` over nullable columns lost `isnotnull(v)` and `isnotnull(w)`, and with them the row pruning they push into each side. - **`InlineWithDefinitionsThatGainNothing` asks whether a kept `With` is still worth keeping**, running with the simplification rules rather than after them. `ON nullif(l.id = r.id, NULL)` reads its definition twice, so the rewrite keeps it; `NullPropagation` and `SimplifyConditionals` then leave one read, in a batch that runs after the rewrite's own. The equality stayed wrapped where `ExtractEquiJoinKeys` cannot see a join key, turning `BroadcastHashJoin [l], [r]` into `BroadcastNestedLoopJoin`. - **A `With` whose subtree holds another `With` is inlined as before.** This rule defers a nested `With` to its next pass, and a kept one is met again on that pass and defers it again, so a nested definition reading one side only would never reach a child `Project`: for `(l.a + r.x) BETWEEN nullif(expensive_left(l.a), 0) AND 1000`, `expensive_left(l.a)` went from one evaluation per left row to one per row pair. A definition that cannot be put in a `Project` for the other reason -- it holds an aggregate, window or generator expression -- is still inlined. Keeping the `With` there would leave those expressions where the planner does not look for them, and no query shape reaches that branch to test it. ### Why are the changes needed? The definition was evaluated once per reference where the `With` promises once per row pair. Over a 4x5 row-pair join whose condition reads its definition twice, a UDF in the definition was called 32 times before this change and 20 after -- one per pair, the 12 extra being the pairs that passed the first comparison and reached the second. The rule also stops carrying two TODOs for a case it can now handle. ### Does this PR introduce _any_ user-facing change? No. Results are unchanged: only a deterministic definition can reach this branch, so one evaluation and two agree. `EXPLAIN` shows a `With` in the condition where it used to show the definition at both references. ### How was this patch tested? New cases in `RewriteWithExpressionSuite`: the definition is kept, emitted once and read twice; one `With` with a kept definition and a hoisted one; the equi-join keys are still found by `ExtractEquiJoinKeys` with the `With` in `otherCondition`; a nested `With` keeps its hoist; a definition that gains nothing is still inlined. New cases in `InferFiltersFromConstraintsSuite` (`isnotnull` on both sides of a join whose condition holds a `With`) and in `ColumnExpressionSuite`: the condition's shape, the same answers across `BroadcastNestedLoopJoinExec`, `BroadcastHashJoinExec`, `SortMergeJoinExec` and a left outer join on each of the three evaluation paths, the evaluation count through an accumulator UDF, and the join key that a later simplification exposes. ### 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]
