LuciferYang opened a new pull request, #58665: URL: https://github.com/apache/spark/pull/58665
### What changes were proposed in this pull request? `CodegenContext.CommonExprSlots.fill` emits a `With` common expression's definition once per scope by putting its body in a private method, but it could only do that where the definition read the input row (`INPUT_ROW != null && currentVars == null`), the condition `Expression.reduceCodeSize` splits under. A whole-stage `Project` or `Filter` passes its input as local variables instead, so there the body was pasted at every `CommonExpressionRef`, doubling per nesting level. This is the `TODO(SPARK-59295)` left by SPARK-58818. This PR builds the method's parameter list, so the body goes into a method under whole-stage codegen too. The parameters are what the body would otherwise read from the scope the call replaces it in: the input row, an input variable the operator evaluated before generating this expression, and a value subexpression elimination computed. Everything else the body reads is a field -- the slots, a lambda variable, an expression's own mutable state -- and needs nothing passed. Two shapes are refused instead, and keep the inline body they have today: - A definition reading an input variable the operator has **not** evaluated yet, which is any attribute a `Project`'s list mentions only once. That variable's code was generated against the scope of the operator producing the row and names a local of it -- the column batch's row index, the input adapter's row -- so it cannot travel into the method. Hoisting it to before the call, the way `getLocalInputVariableValues` does for subexpression elimination, would evaluate it on rows that reach no reference, which is the laziness SPARK-58818 exists to keep. - A value whose name is not a Java identifier. `ExpandExec` allocates a varying output column as mutable state but hands it out as a local, and for a non-primitive type that name is a slot of the compacted array, `mutableStateArray_0[3]`. ### Why are the changes needed? Generated code size. For a `With` nested d levels deep, each level reading its definition twice, the whole-stage source of the expression was: | depth | before | after | |---|---|---| | 2 | 2087 | 1315 | | 3 | 4619 | 1810 | | 4 | 9755 | 2314 | | 5 | 19979 | 2812 | Copies of the innermost body: 2, 4, 8, 16, 32 before, and 2 at every depth after. Exponential growth reaches the method size limit a few levels further on, and dropping the whole stage costs far more than the one call per row this adds. ### Does this PR introduce _any_ user-facing change? No. The call replaces the body inside the same `if (!computed)` guard, so a definition is still evaluated at most once per row, at the first reference reached; only the generated Java differs. ### How was this patch tested? New cases in `WithExpressionEvalSuite` and `WholeStageCodegenSuite`. The first generates against the context a whole-stage operator sets up and asserts the innermost body is emitted twice at any depth, that an evaluated variable and its nullness arrive as parameters, and that each refused shape gets no method; the second runs a nested `With` in a branch over a range and over a Parquet scan, asserting the method count in the one case and, in the other, that the stage still compiles. ### 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]
