LuciferYang commented on code in PR #58881:
URL: https://github.com/apache/spark/pull/58881#discussion_r4360359881
##########
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) {
+ if (Utils.isTesting) {
Review Comment:
Moved in fadfa0208c1. `collectInputs` no longer checks.
`CodegenContext.assertParametersAreNames` runs at the three places that build a
method from these arguments, right before `addNewFunction` and inside the
branch that generates the method: the `subExpr` method, `doAggregate_*` and
`switchCaseCode`. With `ExpandExec` handing out `JavaCode.variable` again, it
names `project_subExpr`, `expand_switchCaseCode` and `hashAgg_doAggregate_sum`
in the two SPARK-59589 tests. `CollectInputsSuite` now asserts that collecting
under the operator policy does not throw and that the check does.
##########
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
Review Comment:
Since this PR removes the rule that read such a slot as a field, I covered
these two policies here in fadfa0208c1: under testing, `collectInputs` now
fails on a `VariableValue` that is not a name where the policy asks for names,
instead of refusing the method silently. It fires wherever such a method is
considered, including the whole stage split's recording pass, which is where
the refusal used to happen silently. `WithExpressionEvalSuite` and
`CollectInputsSuite` now expect the assertion for a slot, and `MergeRowsExec`,
which wrapped its bitmap slot as a variable, now hands it out with
`JavaCode.global`.
A check in `JavaCode.variable` itself would fire on every value created,
whether or not any code moves into a method, and `collectInputs` wraps
`INPUT_ROW` with it too, so it needs its own design. I filed SPARK-60510 for
it, with the remaining gap: `wholeStageSplitArguments` appends `INPUT_ROW`
without going through `collectInputs`.
##########
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) {
+ if (Utils.isTesting) {
+ assert(isName, s"$name is handed out as a local variable, but no
parameter can " +
Review Comment:
Good catch. The check now says `set INPUT_ROW to a local or a field added
with forceInline = true` when the parameter is `INPUT_ROW`, and keeps the other
advice otherwise. The comment above `take` mentions `INPUT_ROW` too. A new
`CollectInputsSuite` test sets `INPUT_ROW` to a slot and checks that both the
collection for a common expression's method and an operator's method that takes
it fail with that message.
--
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]
