cloud-fan commented on code in PR #58665:
URL: https://github.com/apache/spark/pull/58665#discussion_r3999314357
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -278,38 +275,88 @@ 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
+ // (`With.refsToBind` says why). The length arm keeps one body from
being split once per
+ // reference; it fires in the band just under the threshold where
`reduceCodeSize` applies,
+ // since `body` is assembled after `definition.genCode` already ran it.
val worthAMethod = definition.containsPattern(WITH_EXPRESSION) ||
body.length > SQLConf.get.methodSplitThreshold
- if (canPutInMethod && worthAMethod) {
- val funcName = freshName("computeCommonExpr")
- val funcFullName = addNewFunction(funcName,
- s"""
- |private void $funcName(InternalRow $INPUT_ROW) {
- | $body
- |}
+ (if (worthAMethod) methodArgs else None) match {
+ case Some(args) =>
+ val funcName = freshName("computeCommonExpr")
+ val params = args.map(a => s"${typeName(a.javaType)}
${a.variableName}").mkString(", ")
+ val funcFullName = addNewFunction(funcName,
+ s"""
+ |private void $funcName($params) {
+ | $body
+ |}
""".stripMargin)
- code"$funcFullName($INPUT_ROW);"
- } else {
- body
+ code"$funcFullName(${args.map(_.variableName).mkString(", ")});"
+ case None =>
+ body
+ }
+ }
+
+ /**
+ * The locals to pass the method, or None where a method is not possible.
What it collects are
+ * the values 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, a value
+ * subexpression elimination computed -- all of them declared in a scope
that encloses the call.
+ *
+ * A definition that reads an input variable the operator has *not*
evaluated yet gets no
+ * method. That variable's code cannot travel into one: it was generated
by the operator that
+ * produces the row, against that operator's scope, so it names a local of
that scope -- the
+ * column batch's row index, or the input adapter's row. Nor can it be
hoisted to before the
+ * call, the way `getLocalInputVariableValues` does for subexpression
elimination, since that
+ * evaluates it on rows that reach no reference.
+ */
+ private def methodArgs: Option[Seq[VariableValue]] = {
+ val args = mutable.LinkedHashMap.empty[String, VariableValue]
+ // False for a value no parameter can carry: `ExpandExec` hands out a
`VariableValue` naming a
+ // slot of a compacted mutable state array, and a `SimpleExprValue` is
an expression rather
+ // than a name. A field or a literal needs no parameter and is read as
it stands.
+ def canPass(v: ExprValue): Boolean = v match {
+ case local: VariableValue =>
+ val name = local.variableName
+ val isName = name.nonEmpty &&
Character.isJavaIdentifierStart(name.head) &&
+ name.forall(Character.isJavaIdentifierPart)
+ if (isName) {
+ args.getOrElseUpdate(name, local)
+ }
+ isName
+ case _: GlobalValue | _: LiteralValue => true
+ case _ => false
+ }
+ var possible = INPUT_ROW == null ||
+ canPass(JavaCode.variable(INPUT_ROW, classOf[InternalRow]))
+ val visited = mutable.HashSet.empty[Long]
+ val toVisit = mutable.Stack[Expression](definition)
+ while (possible && toVisit.nonEmpty) {
+ toVisit.pop() match {
+ case ref: BoundReference if currentVars != null &&
currentVars(ref.ordinal) != null =>
+ val input = currentVars(ref.ordinal)
+ possible = input.code == EmptyBlock && canPass(input.value) &&
canPass(input.isNull)
+ case ref: CommonExpressionRef =>
+ // A reference to an enclosing scope, since one to this scope is
refused. Its slots are
Review Comment:
**Nit (P3):** This says references here can only come from enclosing scopes,
but withCommonExprs registers sibling definitions and CommonExpressionRef can
resolve a same-scope sibling. The adjacent build comment explicitly relies on
that supported path, so this state-machine explanation is contradictory.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -278,38 +275,88 @@ 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
+ // (`With.refsToBind` says why). The length arm keeps one body from
being split once per
+ // reference; it fires in the band just under the threshold where
`reduceCodeSize` applies,
+ // since `body` is assembled after `definition.genCode` already ran it.
val worthAMethod = definition.containsPattern(WITH_EXPRESSION) ||
body.length > SQLConf.get.methodSplitThreshold
- if (canPutInMethod && worthAMethod) {
- val funcName = freshName("computeCommonExpr")
- val funcFullName = addNewFunction(funcName,
- s"""
- |private void $funcName(InternalRow $INPUT_ROW) {
- | $body
- |}
+ (if (worthAMethod) methodArgs else None) match {
+ case Some(args) =>
+ val funcName = freshName("computeCommonExpr")
+ val params = args.map(a => s"${typeName(a.javaType)}
${a.variableName}").mkString(", ")
+ val funcFullName = addNewFunction(funcName,
+ s"""
+ |private void $funcName($params) {
+ | $body
+ |}
""".stripMargin)
- code"$funcFullName($INPUT_ROW);"
- } else {
- body
+ code"$funcFullName(${args.map(_.variableName).mkString(", ")});"
+ case None =>
+ body
+ }
+ }
+
+ /**
+ * The locals to pass the method, or None where a method is not possible.
What it collects are
+ * the values 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, a value
+ * subexpression elimination computed -- all of them declared in a scope
that encloses the call.
+ *
+ * A definition that reads an input variable the operator has *not*
evaluated yet gets no
+ * method. That variable's code cannot travel into one: it was generated
by the operator that
+ * produces the row, against that operator's scope, so it names a local of
that scope -- the
+ * column batch's row index, or the input adapter's row. Nor can it be
hoisted to before the
+ * call, the way `getLocalInputVariableValues` does for subexpression
elimination, since that
+ * evaluates it on rows that reach no reference.
+ */
+ private def methodArgs: Option[Seq[VariableValue]] = {
+ val args = mutable.LinkedHashMap.empty[String, VariableValue]
+ // False for a value no parameter can carry: `ExpandExec` hands out a
`VariableValue` naming a
+ // slot of a compacted mutable state array, and a `SimpleExprValue` is
an expression rather
+ // than a name. A field or a literal needs no parameter and is read as
it stands.
+ def canPass(v: ExprValue): Boolean = v match {
+ case local: VariableValue =>
+ val name = local.variableName
+ val isName = name.nonEmpty &&
Character.isJavaIdentifierStart(name.head) &&
+ name.forall(Character.isJavaIdentifierPart)
+ if (isName) {
+ args.getOrElseUpdate(name, local)
+ }
+ isName
+ case _: GlobalValue | _: LiteralValue => true
+ case _ => false
+ }
+ var possible = INPUT_ROW == null ||
+ canPass(JavaCode.variable(INPUT_ROW, classOf[InternalRow]))
+ val visited = mutable.HashSet.empty[Long]
+ val toVisit = mutable.Stack[Expression](definition)
+ while (possible && toVisit.nonEmpty) {
+ toVisit.pop() match {
+ case ref: BoundReference if currentVars != null &&
currentVars(ref.ordinal) != null =>
+ val input = currentVars(ref.ordinal)
+ possible = input.code == EmptyBlock && canPass(input.value) &&
canPass(input.isNull)
+ case ref: CommonExpressionRef =>
+ // A reference to an enclosing scope, since one to this scope is
refused. Its slots are
+ // filled inside this method, so what that definition reads has to
come in as well.
+ if (visited.add(ref.id.id)) {
+ currentCommonExprs.get(ref.id.id).foreach(slot =>
toVisit.push(slot.definition))
+ }
+ case e =>
+ // Stopping where `Expression.genCode` stops: it reads a
subexpression's value off the
+ // state instead of generating the subtree again.
+ subExprEliminationExprs.get(ExpressionEquals(e)) match {
+ case Some(state) =>
+ possible = canPass(state.eval.value) &&
canPass(state.eval.isNull)
+ case None => toVisit.pushAll(e.children)
+ }
+ }
}
+ val params = args.values.toSeq
+ Option.when(
Review Comment:
**Non-blocking (P2):** No new test drives this CommonExprSlots signature
past the JVM descriptor limit. A regression in this guard could emit an
uncompilable helper and drop whole-stage codegen while all of the added tests
still pass.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -278,38 +275,88 @@ 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
+ // (`With.refsToBind` says why). The length arm keeps one body from
being split once per
+ // reference; it fires in the band just under the threshold where
`reduceCodeSize` applies,
+ // since `body` is assembled after `definition.genCode` already ran it.
val worthAMethod = definition.containsPattern(WITH_EXPRESSION) ||
body.length > SQLConf.get.methodSplitThreshold
- if (canPutInMethod && worthAMethod) {
- val funcName = freshName("computeCommonExpr")
- val funcFullName = addNewFunction(funcName,
- s"""
- |private void $funcName(InternalRow $INPUT_ROW) {
- | $body
- |}
+ (if (worthAMethod) methodArgs else None) match {
+ case Some(args) =>
+ val funcName = freshName("computeCommonExpr")
+ val params = args.map(a => s"${typeName(a.javaType)}
${a.variableName}").mkString(", ")
+ val funcFullName = addNewFunction(funcName,
+ s"""
+ |private void $funcName($params) {
+ | $body
+ |}
""".stripMargin)
- code"$funcFullName($INPUT_ROW);"
- } else {
- body
+ code"$funcFullName(${args.map(_.variableName).mkString(", ")});"
+ case None =>
+ body
+ }
+ }
+
+ /**
+ * The locals to pass the method, or None where a method is not possible.
What it collects are
+ * the values 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, a value
+ * subexpression elimination computed -- all of them declared in a scope
that encloses the call.
+ *
+ * A definition that reads an input variable the operator has *not*
evaluated yet gets no
+ * method. That variable's code cannot travel into one: it was generated
by the operator that
+ * produces the row, against that operator's scope, so it names a local of
that scope -- the
+ * column batch's row index, or the input adapter's row. Nor can it be
hoisted to before the
+ * call, the way `getLocalInputVariableValues` does for subexpression
elimination, since that
+ * evaluates it on rows that reach no reference.
+ */
+ private def methodArgs: Option[Seq[VariableValue]] = {
+ val args = mutable.LinkedHashMap.empty[String, VariableValue]
+ // False for a value no parameter can carry: `ExpandExec` hands out a
`VariableValue` naming a
+ // slot of a compacted mutable state array, and a `SimpleExprValue` is
an expression rather
+ // than a name. A field or a literal needs no parameter and is read as
it stands.
+ def canPass(v: ExprValue): Boolean = v match {
+ case local: VariableValue =>
+ val name = local.variableName
+ val isName = name.nonEmpty &&
Character.isJavaIdentifierStart(name.head) &&
+ name.forall(Character.isJavaIdentifierPart)
+ if (isName) {
+ args.getOrElseUpdate(name, local)
+ }
+ isName
+ case _: GlobalValue | _: LiteralValue => true
+ case _ => false
Review Comment:
**Non-blocking (P2):** The negative test covers only a non-identifier
VariableValue and never reaches the separate SimpleExprValue fallback.
GenerateExec supplies loop-local position nullness in that form; if this
rejection regressed, the expression could escape its loop scope while every
added test remained green.
--
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]
