Re: [PR] [SPARK-59589][SQL] Hand Expand's varying output columns to the consumer as globals [spark]

["MaxGekk (via GitHub)" <gi...@apache.org>]

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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org