ulysses-you commented on PR #57742:
URL: https://github.com/apache/spark/pull/57742#issuecomment-5191303510
Thanks @cloud-fan -- all four are addressed in d95d6c52655 and f35a80c69ab.
**The remaining item was a real bug, and your reading of it was exactly
right.** When I moved the denominator to the operator level in the previous
round I only did half the job: the denominator counted both maps while the
increment stayed inside the regular-map branch, so rows the fast map absorbed
were missing from the numerator. A hot-key input the aggregation was collapsing
heavily could measure as ineffective and bypass. Results stayed correct --
pass-through preserves semantics -- but the policy decision was wrong and the
performance went the wrong way.
The test you asked for fails before the fix on exactly `wholeStage=true
twoLevelMap=true`, the only combination the bug can reach:
```
- rows absorbed by the fast map count toward the compaction ratio *** FAILED
***
wholeStage=true twoLevelMap=true 12 did not equal 0
```
Worth flagging that my first attempt at the fix was wrong in a way the suite
caught. Moving the increment out created a state that could not occur before --
the fast map serving a row *and* that row flipping pass-through -- and the
pass-through buffer condition still only checked the regular-map buffer, so
such a row was both aggregated in the fast map and emitted as a pass-through
row. Eight correctness tests failed. The condition on both sites now asks
whether *either* map holds the row.
`minRows = 0` needed no execution-path change, which I verified rather than
assumed: the count is only compared after being incremented past 0, so the
equality never holds. Two tests, since a spill-only test alone would not show
the periodic check is actually off -- one asserts nothing bypasses without a
spill (the same query bypasses with `minRows = 8` elsewhere in the suite), the
other asserts the spill-only mode works.
`f35a80c69ab` is a follow-up with no behavior change: the generated
fragments now sit under the same `adaptivePartialAggEnabled` guard as their use
sites (they interpolated `null` otherwise -- harmless, but only by convention),
and the per-row block is merged since its two branches are mutually exclusive
on whether a map accepted the row.
Suite is at 40 tests, all passing, plus `DataFrameAggregateSuite` and
`AggregateHashMapSuite` (513) as a regression check.
One open question from my earlier comment, in case it got lost: I used
strict `<` for the ratio test, so at exactly `rows / keys == minCompaction` the
aggregation is kept, whereas your description said `<=`. Happy to flip it --
one character plus inverting the boundary test's expectation.
--
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]
