cloud-fan commented on code in PR #58665:
URL: https://github.com/apache/spark/pull/58665#discussion_r4000445163
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -278,38 +275,115 @@ class CodegenContext extends Logging {
|${value.value} = ${defGen.value};
|$computed = true;
""".stripMargin
- // TODO(SPARK-59295): cover the local-variable case too, by passing the
`currentVars` values a
- // definition reads into the method as parameters, the way
- // `subexpressionEliminationForWholeStageCodegen` does. It needs a
decision first:
- // `getLocalInputVariableValues` hoists an input variable that is not
evaluated yet to
- // before the call, which for a reference behind a branch means
evaluating it on rows that
- // never reach the reference.
- val canPutInMethod = INPUT_ROW != null && currentVars == null
- // A definition that is or holds another `With` is the shape whose code
doubles per level,
- // and what this is aimed at. It is not the only one -- a definition
referencing a sibling
- // definition of the same `With` doubles the same way, and codegen
accepts that, since the
- // sibling's slots are in scope while this definition is generated
(`With.refsToBind` says
- // why nothing builds that tree, and that evaluating one raises). What
bounds those is not
- // the length arm below: `body` is assembled after `definition.genCode`
already ran
- // `reduceCodeSize`, so the arm fires only in the band just under the
threshold. It is
- // `reduceCodeSize` itself, which hoists whichever node's code first
passes the threshold as
- // generation walks up, capping what one level contributes, so the code
stays linear in the
- // depth either way. The length arm just keeps the same body from being
split once per
- // reference, which leaves the methods small and the code as large.
+ // A definition that is or holds another `With` is the shape whose code
doubles per level, and
+ // what this is aimed at. A definition referencing a sibling definition
of the same `With`
+ // doubles the same way and is covered by the same arm, though nothing
builds that tree today
Review Comment:
**Nit (P3):** This says sibling-definition references are covered by the
same method-extraction arm, but a short definition containing only
`CommonExpressionRef` leaves matches neither
`definition.containsPattern(WITH_EXPRESSION)` nor the body-length threshold.
Please adjust the comment to describe the actual boundary.
--
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]
