cloud-fan commented on code in PR #58881:
URL: https://github.com/apache/spark/pull/58881#discussion_r4323282482
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/ExpandExec.scala:
##########
@@ -187,9 +187,15 @@ case class ExpandExec(
"resultValue",
v => s"$v = ${CodeGenerator.defaultValue(firstExpr.dataType)};")
+ // `addMutableState` hands back a field: a name of its own, or a
slot in a compacted array
+ // (`mutableStateArray_0[3]`) for a type it cannot inline. Either is
reachable from every
Review Comment:
**Nit (P3):** Compaction isn't decided by type alone. `addMutableState`
inlines a primitive only while the class holds fewer than
`OUTER_CLASS_VARIABLES_THRESHOLD` (10000) inlined states. Past that, the
boolean `resultIsNull` here also comes back as a slot of a compacted
`boolean[]`, since it is allocated without `forceInline`. That is the case that
makes `isNullGlobal` necessary for the null flag too, but "for a type it cannot
inline" reads as if only the non-primitive value can be a slot. Maybe:
```suggestion
// (`mutableStateArray_0[3]`) for a state not inlined. Either is
reachable from every
```
**Verification:**
- **Inspection:** The comment's description of when addMutableState returns
a compacted-array slot matches its documented and implemented inline conditions
(forced inlining, a primitive while under the outer-class threshold, a
multi-dimensional array), so both the value and the null flag are covered by
the explanation for labelling them as globals.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala:
##########
@@ -1741,6 +1741,35 @@ class WholeStageCodegenSuite extends SharedSparkSession
"sinh(v) should be evaluated only once per input row without function
splitting")
}
+ test("SPARK-59589: a subexpression method reads an Expand's varying output
as a field") {
+ // `ExpandExec` holds an UNPIVOT's string output in a slot of a compacted
mutable state array,
+ // such as `expand_mutableStateArray_0[1]`. A large common subexpression
reading it is computed
+ // in a method of its own, which takes the local variables it reads as
parameters. Taken for a
+ // local variable, the slot became a parameter `UTF8String
expand_mutableStateArray_0[1]` and
+ // the stage did not compile. Handed out as a field, it is read as one,
split on or off.
+ withTempView("t") {
+ spark.range(10).selectExpr("concat('a', CAST(id AS STRING)) AS a",
+ "concat('b', CAST(id AS STRING)) AS b").createOrReplaceTempView("t")
+ val f = "concat(upper(v), lower(v), reverse(v), trim(v), ltrim(v),
rtrim(v), initcap(v), " +
+ "repeat(v, 2), lpad(v, 10, 'x'), rpad(v, 10, 'y'))"
+ def df: DataFrame =
+ sql(s"SELECT c, $f AS x, concat($f, 'z') AS y FROM t UNPIVOT (v FOR c
IN (a, b))")
+ val expected = withoutWholeStage(df)
+ val stringParameter = "private void \\w*subExpr_\\d+\\([^)]*UTF8String
".r
+ Seq("true", "false").foreach { split =>
+ withSQLConf(SQLConf.WHOLESTAGE_SPLIT_EXPRESSIONS.key -> split) {
+ val code = genCode(df)
+ assert(code.exists { c =>
+ methodsHolding(c,
"expand_mutableStateArray").exists(_.contains("subExpr"))
Review Comment:
**Nit (P3):** This holds even if no `subExpr` method reads the Expand's
slot. In the split path, whole-stage subexpression elimination stores each
string result with `addMutableState("UTF8String", "subExprValue")`. That takes
a slot of the same per-type array `ExpandExec` created first, so it is also
`expand_mutableStateArray_0[k]`, and the `subExpr` body writes
`expand_mutableStateArray_0[k] = ...;`. So every split string `subExpr` method
contains the text, whatever it reads.
The negative regex and the compile do catch the bug in today's plan. But if
a later change stopped routing `v` through the Expand's slot, everything here
would still pass and the test would quietly stop covering the bug. The removed
SPARK-33301 test guarded against that with its conf-off `nonEmpty` check, which
this fix makes impossible. Could the anchor require a read of the slot instead?
For example, a method call on it, like `expand_mutableStateArray_0[1].lpad(`
from `lpad(v, 10, 'x')`, which the method's own `... = ...` write never matches.
**Verification:**
- **Regression:** Under both split conf values, the positive assertion
passes for the reproducer and does not accept a subExpr method that only writes
its own compacted result slot without reading an Expand varying-output slot, so
the test cannot pass once it stops exercising the fixed shape.
- **Behavior:** Under both split conf values, the reproducer's generated
code still has no subExpr method taking a UTF8String parameter, compiles, and
returns the rows computed without whole-stage codegen.
--
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]
