txwyy123 commented on issue #59787: URL: https://github.com/apache/spark/issues/59787#issuecomment-6405607528
## Benchmark Findings ### Local variable scoping prevents simple `addNewFunction` wrapping Attempting to wrap `consume()` in a separate method via `ctx.addNewFunction()` for SortMergeJoinExec/HashJoin fails because `consume()` generates code referencing **local variables** (`smj_bufferedRow_1`, `smj_value_4`, etc.) that only exist in the caller's method scope. Moving the code to a separate class method breaks Janino compilation. The existing `codegenFullOuter` pattern works because it was designed from scratch with **class-level mutable state** (`leftOutputRow`, `rightOutputRow` via `ctx.addMutableState`). The other join types (`codegenInner`, `codegenOuter`, etc.) use local loop variables. **The correct fix requires either:** 1. **Hoisting local variables to class-level fields** (like `codegenFullOuter`) — requires refactoring how `codegenInner`/`codegenOuter` declare loop variables. 2. **Using `constructConsumeParameters`** to convert locals to method parameters — this is what `splitConsumeFuncByOperator` does in `WholeStageCodegenExec.consume()`. 3. **Splitting the variable evaluation code** (the column extraction) into sub-methods using `ctx.splitExpressions`, keeping `consume()` inline. ### Pre-existing codegen compilation bug found With `spark.sql.codegen.maxFields` > 100 (to enable codegen on wide schemas), SortMergeJoin's generated code fails Janino compilation: ``` CompileException: File 'generated.java', Line 90, Column 39: Unknown variable or type "smj_bufferedRow_1" ``` This happens on **stock Spark master** (no modifications) because the inner class split mechanism (`addNewFunctionToClass`, triggered when code exceeds `GENERATED_CLASS_SIZE_THRESHOLD`) moves generated code to a nested class that can't access the outer method's local variables. The default `maxFields=100` masks this by disabling codegen entirely for wide schemas. This is a separate bug worth filing — it means the `maxFields` limit can't simply be raised without fixing the variable scoping issue. ### Revised approach Option 3 (splitting variable evaluation via `ctx.splitExpressions`) is the most practical path forward: - Keep `consume()` inline (avoids the local variable scoping issue) - Split the column extraction code in `genOneSideJoinVars` and `createStreamedVars` into batched helper methods - This reduces the `processNext()` method size without restructuring the variable declarations -- 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]
