LuciferYang commented on PR #58045: URL: https://github.com/apache/spark/pull/58045#issuecomment-5428585049
Thanks. The finding is real, and the comment sitting above that code said the opposite, which bothers me more than the code does. I measured it before touching anything, and the numbers moved my view of what the fix is worth. Copies of a marked innermost body in the generated source, nested `With`s, depths 1 to 8, default `methodSplitThreshold` of 1024, leaf body 237 characters. The two rows differ in what the expression is generated against: an input row (`INPUT_ROW` set, `currentVars` null), or the child's output columns as local variables, which is what whole-stage codegen hands an expression. ``` against a row, before: 2 4 4 4 4 4 4 4 source 919 -> 7221 against a row, after: 2 2 2 2 2 2 2 2 source 919 -> 4457 against locals, both: 2 4 8 16 32 64 128 256 source 823 -> 163371 ``` Generating against a row was never exponential. `Expression.genCode` runs `reduceCodeSize` on every expression, including a `With` serving as a definition, under the same `INPUT_ROW != null && currentVars == null` guard and the same threshold. Once a level's text crosses it, that level becomes a method and its parent's two references emit two calls rather than two bodies, so the growth resets. Hence the plateau at 4. The exponential case is the one you named. Whole-stage codegen has `reduceCodeSize` disabled by the condition you quoted, and it is the path this fix cannot reach, because a method cannot see the `currentVars` locals a definition reads. So what the fix buys is smaller than the comment claimed: the body once per scope instead of once per reference, and a bound that no longer grows with depth wherever the threshold sits. Raise `methodSplitThreshold` and the "before" plateau rises with it, while the "after" stays at 2. The body goes into `private void computeCommonExpr_N(InternalRow)` and each reference keeps a call behind its `computed` flag. The new test in `WithExpressionEvalSuite` holds the count at 2 for depths 1 to 6 with the threshold pinned, so the expectation does not ride on the default. It reads 4 at depth 2 without the change. Covering whole-stage codegen means passing the `currentVars` locals a definition needs into the method as parameters. That is the `// TODO: support whole stage codegen` on `splitExpressionsWithCurrentInputs` (`CodeGenerator.scala:1051`), blocked by the same condition for the same reason. I would rather not fold it in here. The whole-stage half therefore stays as it was, which I do not think is a regression. On master a nested `nullif` in a conditional branch gets inlined by `RewriteWithExpression`, so the plan is already 2^depth nodes before codegen sees it. This PR makes the plan linear, and the generated code has a size-based net already: `spark.sql.codegen.hugeMethodLimit` and the 64KB method limit demote oversized whole-stage output, and the projection it falls back to is where this fix applies. If you would rather close that half here too, the cheap lever is `CollapseCodegenStages.supportCodegen` returning false for a `With` whose definition holds another one, which keeps it out of the fused stage. I left it out because whole-stage source at depth 2 is only 2091 characters, so the trade is giving up whole-stage codegen for an ordinary `nullif(nullif(a, b), c)`, and bounding by depth instead means picking a magic number. Happy to add it if you prefer that trade. The three nits were all right and are applied. The `CodegenFallback` note now says `CollapseCodegenStages` finds a non-leaf descendant fallback structurally, so a non-trait caller only has to be named at the two sites that dispatch on the node in hand. `refsToBind` names the curried `With(commonExprs: _*)(replaced)` helper instead of claiming the case class constructor cannot build a definition that references one of these ids, and says what happens when a caller does: the reference stays unbound and evaluating it raises. And the `randstr` comment no longer explains a raise that a foldable length of `3` cannot produce. -- 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]
