LuciferYang commented on code in PR #58881:
URL: https://github.com/apache/spark/pull/58881#discussion_r4360362094
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1322,24 +1324,25 @@ class CodegenContext extends Logging {
val args = mutable.LinkedHashMap.empty[String, VariableValue]
val inputsToEvaluate = mutable.ArrayBuffer.empty[ExprCode]
// Whether the code can read `v` from the method: false for a value no
parameter can carry
- // where the policy asks for names. `ExpandExec` hands out a
`VariableValue` naming a slot of a
- // compacted mutable state array, and a `SimpleExprValue` is an expression
rather than a name
+ // where the policy asks for names. A `SimpleExprValue` is an expression
rather than a name
// -- `posexplode_outer` gives its position the nullness `index == -1`,
and a `Byte` or `Short`
// literal's value is `(byte)1`. A field or a literal needs no parameter
and is read as it
- // stands; so is a field handed out as a `VariableValue` where the policy
says so, passing one
- // being refused by the split methods' own check, a slot of a compacted
array included.
+ // stands; so is a `VariableValue` naming a field `addMutableState`
registered, where the
+ // policy says so, passing one being refused by the split methods' own
check in tests. A slot
Review Comment:
Reworded in fadfa0208c1: "by the whole stage split's own check (`splitCall`)
in tests". The last sentences now say where testing checks a slot handed out as
a variable: in `collectInputs` where the policy asks for names, and in
`assertParametersAreNames` otherwise.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala:
##########
@@ -1771,6 +1801,41 @@ class WholeStageCodegenSuite extends SharedSparkSession
}
}
+ test("SPARK-59589: split switch cases and aggregate functions read an
Expand's output as a " +
Review Comment:
Done in fadfa0208c1: both tests go through a new
`checkExpandSlotRead(method, query, check)` next to `methodsHolding`, and the
first one passes its `UTF8String` parameter check as `check`, which runs with
the method and the split setting as a clue.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:
##########
@@ -1322,24 +1324,25 @@ class CodegenContext extends Logging {
val args = mutable.LinkedHashMap.empty[String, VariableValue]
val inputsToEvaluate = mutable.ArrayBuffer.empty[ExprCode]
// Whether the code can read `v` from the method: false for a value no
parameter can carry
- // where the policy asks for names. `ExpandExec` hands out a
`VariableValue` naming a slot of a
- // compacted mutable state array, and a `SimpleExprValue` is an expression
rather than a name
+ // where the policy asks for names. A `SimpleExprValue` is an expression
rather than a name
// -- `posexplode_outer` gives its position the nullness `index == -1`,
and a `Byte` or `Short`
// literal's value is `(byte)1`. A field or a literal needs no parameter
and is read as it
- // stands; so is a field handed out as a `VariableValue` where the policy
says so, passing one
- // being refused by the split methods' own check, a slot of a compacted
array included.
+ // stands; so is a `VariableValue` naming a field `addMutableState`
registered, where the
+ // policy says so, passing one being refused by the split methods' own
check in tests. A slot
+ // of a compacted array is not a name at all, so it has to be handed out
as a `GlobalValue`;
+ // where the policy does not ask for names, testing checks for one handed
out as a variable.
def take(v: ExprValue): Boolean = v match {
case local: VariableValue =>
val name = local.variableName
val isName = name.nonEmpty &&
Character.isJavaIdentifierStart(name.head) &&
name.forall(Character.isJavaIdentifierPart)
- // A slot of a compacted array is a field under every policy, since no
parameter can be
- // named `array[i]`, where the whole stage split is on; with it off,
every policy answers
- // as it did before that split existed. Another field is passed where
the policy says so.
- val isField = (isCompactedSlot(name) && wholeStageSplit !=
WholeStageSplit.Off) ||
- (policy.fieldsNeedNoArgument && mutableStateNames.contains(name))
- val taken = isName || isField || !policy.onlyNames
+ val isField = policy.fieldsNeedNoArgument &&
mutableStateNames.contains(name)
+ val taken = isName || !policy.onlyNames
if (taken && !isField) {
Review Comment:
This went away with the move (see L1342): `assertParametersAreNames` runs
only where the `operatorMethod` arguments become a method's parameters, and the
check left in `collectInputs` runs only under `onlyNames` (see L1340).
--
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]
