MaxGekk commented on PR #59069:
URL: https://github.com/apache/spark/pull/59069#issuecomment-5849776366
@dongjoon-hyun thank you for the careful trace - both correctness findings
were real. All items are addressed in 8750bdd8107, plus a commit with the
benchmark results.
**1. `With` and `INPUT_ROW`.** A `CommonExpressionRef` now sets `needsRow`
when `INPUT_ROW` is set, since `CommonExprSlots` may make its definition a
method that takes the row. New test: "a common expression in a branch under a
keyed aggregate takes the buffer", your query shape, asserting the stage splits
and matches the non-WSCG answer.
**2. Discarded `computeCommonExpr_N`.** Functions are now recorded as
discardable in `addNewFunctionToClass`, for the whole non-split pass, so
`computeCommonExpr_N` goes with the `caseWhen_*` methods it calls; an inner
pass's functions join the enclosing pass's. That replaced the name snapshot and
diff. New test: "functions added in a discarded pass of subexpression
elimination go", your `ProjectExec` example.
**3. Binary compatibility.** The six-argument
`splitExpressionsWithCurrentInputs` is back unchanged, and the split under WSCG
is a separate overload taking `sources: Seq[Seq[Expression]]` with no default
values. The other 22 callers use the old method, so they also no longer pay the
block regexes under WSCG (your first minor item).
**4. Tests for the arms.** Added a keyed `max_by`/`min_by` test (the
`needsRow` path through the buffer), the `With` and discarded-pass tests above,
and a refusal test: a CASE WHEN whose methods would need more parameter slots
than `validParamLength` stays in one piece.
**5. Join test.** It now asserts that the condition was split, in all three
queries, and the wrong comment about the streamed side is gone.
**6. ROLLUP / compacted slots.** You're right that no test reached the slot
branch, so I removed it: a compacted `mutableStateArray_0[3]` is not a name,
and the split is refused for it.
**7. `stageCodes`.** Removed; the helpers use `genCode(df)`, so they fail
when no stage is generated.
**Minor:**
- *Union of all branch inputs:* each split method now takes only what its
own block reads, and a method grouping calls takes the union of what its calls
take. Test: "each split method takes only what its own branches read", with 90
distinct nullable `bigint` columns.
- *Small `CASE WHEN`s:* added `CaseWhenCodegenBenchmark`, 2 to 300 branches,
with the split, without it, and with WSCG off, 2M rows; results for JDK
17/21/25 on the GitHub runner (EPYC 9V74) are committed. Best times in ms,
split / not split:
| branches | JDK 17 | JDK 21 | JDK 25 |
|---|---|---|---|
| 2 | 88 / 69 | 74 / 83 | 78 / 65 |
| 16 | 69 / 69 | 67 / 65 | 64 / 62 |
| 32 | 92 / 88 | 75 / 88 | 71 / 85 |
| 64 | 116 / 103 | 126 / 104 | 120 / 101 |
| 128 | 190 / 5395 | 287 / 4094 | 225 / 4793 |
| 300 | 450 / 11866 | 462 / 10791 | 458 / 11254 |
Up to 32 branches the two are within the noise - at 2 branches the
generated code is identical (280 bytes either way) and the times still differ
by up to 25%. At 64 branches the split is 13-21% slower on all three JDKs.
`-XX:+LogCompilation` on JDK 25 shows why: unsplit, C2 compiles
`project_doConsume_0` (6049 bytes) as one unit, under HotSpot's 8000-byte
limit. Split, `doConsume` (627 bytes) calls 32 `caseWhen_*` methods of about
198 bytes in sequence; C2 inlines the first 13 and then stops - 11 fail with
`size > DesiredMethodLimit` (the inlining budget is spent) and the last 8 with
`too big` (their call sites are reached by fewer rows, so they are not hot) -
so a row whose value matches a later branch makes real calls. From 128 branches
the unsplit method passes the 8000-byte limit and is not JIT-compiled, and the
split is 14-26x faster. So the split trades up to ~20% in the band where the
unsplit method still compiles for an order of magnitude past it - the same
trade the non
-WSCG split already makes.
- *`wholeStageSplitArguments` vs `CommonExprSlots.methodArgs`:* agreed that
they should be one walker. Item 1 closes the `INPUT_ROW` gap, and the scaladoc
lists which refusals they share and which are its own. Merging them,
parameterised by how a subexpression state is handled, is a follow-up with its
own JIRA - it changes `With`'s method arguments too, and I'd rather not widen
this PR to that.
- *`Cast` arm:* `Cast` now exposes the pass-through test
(`generatesChildCode`) and both places use it.
- *`LambdaVariable`:* the refusal now says the value is a field that the
loop assigns; the refusal itself stays, because no test reaches such a split.
- *`CodegenFallback` arm:* removed.
Could you take another look when you have a chance?
--
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]
