MaxGekk commented on PR #58881:
URL: https://github.com/apache/spark/pull/58881#issuecomment-6082783706
Hi @LuciferYang, this draft fixes a failure that is reachable, so it may be
worth finishing and taking as a bug fix rather than a latent trap.
**A reachable failure.** An UNPIVOT of string columns whose large common
subexpression is computed in a `subExpr` method:
```sql
CREATE OR REPLACE TEMP VIEW t AS
SELECT concat('a', CAST(id AS STRING)) AS a, concat('b', CAST(id AS
STRING)) AS b FROM range(10);
SELECT c,
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')) AS x,
concat(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')), 'z') AS y
FROM t UNPIVOT (v FOR c IN (a, b));
```
The `subExpr` method takes `UTF8String expand_mutableStateArray_0[1]` as a
parameter, so the stage fails to compile (Janino: `')' expected instead of
'['`) and falls back to the non-codegen path, or fails with the fallback off.
@dongjoon-hyun reproduced it on released 4.1.3, 4.2.0 and 4.3.0 while reviewing
#59225, and checked that with the two labels changed as in this PR the query
compiles under Janino and javac and matches the non-codegen results, for
aggregate, consume-function, broadcast-join-key and ROLLUP-over-UNPIVOT shapes
too. I filed SPARK-60021 for it before finding this PR, and will link it as a
duplicate of SPARK-59589. So SPARK-59589 could be a Bug affecting 4.1.3, 4.2.0
and 4.3.0 (SPARK-35329 put the outputs in mutable state in 3.2.0), and a
backport candidate.
**What SPARK-33301 (#59225, merged yesterday) changed around this.** It
works around the mislabel on master and branch-4.x:
`CodegenContext.collectInputs` reads a compacted slot as the field it is when
`spark.sql.codegen.wholeStage.splitExpressions` is on, so the query above now
compiles with that conf on, and still fails with it off. With the relabel the
workaround is redundant and could go in the same change: `CompactedSlot`,
`isCompactedSlot` and the slot clause in `collectInputs`
(`CodeGenerator.scala`), the slot case in `CollectInputsSuite`, and the two
slot tests in `WholeStageCodegenSuite` ("a split method reads a slot of a
compacted mutable state array as a field" and "with the split on, a
subexpression method reads a compacted slot as a field"). I applied this PR on
today's master (it applies cleanly) and ran these: the second of those fails,
at its conf-off half, which asserts that a `subExpr` method takes a
`UTF8String` parameter (`WholeStageCodegenSuite.scala:2847`), w
hich the relabel makes impossible; the first, your new test and
`CollectInputsSuite` pass, so the `CollectInputsSuite` case goes only because
it is redundant. The query above makes a good replacement test, since it fails
before the change and passes after.
Happy to help with any of this if useful.
--
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]
