spark-test commented on code in PR #59741:
URL: https://github.com/apache/spark/pull/59741#discussion_r4356241701
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/aggregate/AdaptivePartialAggregationSuite.scala:
##########
@@ -1139,26 +1139,61 @@ class AdaptivePartialAggregationSuite extends QueryTest
with SharedSparkSession
}
}
- test("the spill check fires when the map would spill on high-cardinality
input") {
- // Force the regular map to fall back quickly. High-cardinality input that
reaches the fallback
- // point should bypass via the spill check rather than spilling. Use a
`minRows` larger than the
- // input so the periodic check cannot fire first and the spill check is
the one exercised.
+ test("the spill check does not bypass an epoch smaller than minRows") {
+ // The spill check, like the periodic check, judges an epoch only once it
holds `minRows` rows,
+ // because the bypass cannot be undone within the task. Force the regular
map to fall back
+ // every 16 rows with a `minRows` larger than the input: every epoch is
below the floor, so
Review Comment:
**Nit (P3):** Because this case picks a `minRows` (100000) larger than the
whole 200-row input, both every 16-row epoch and the task's cumulative row
count stay below the floor. The floor this PR adds is per epoch: both guards
read `processedRows`, which every spill resets. But neither this test nor the
positive one (which reaches the floor in its first epoch) would fail if the
floor were measured against task-cumulative rows. That regression would let the
spill check bypass on a 16-rows-over-16-keys epoch once enough earlier epochs
had spilled, which is exactly the small-sample decision this PR is meant to
prevent. Setting `minRows` between the epoch size and the input size, for
example `32`, pins the per-epoch scope in all four codegen x two-level-map
modes. Every epoch (at most 20 rows, i.e. 4 fast-map + 16 regular-map rows in
the first two-level epoch) stays below 32, so the existing `skipped == 0` and
`tasksFallBacked > 0` assertions still hold. A cumulative floor would inste
ad be reached at the second spill and bypass. The comment here would then need
to say the case keeps each epoch, not the whole input, below `minRows`.
**Verification:**
- **Regression:** With distinct input, a forced regular-map fallback that
keeps every epoch below minRows, and a total row count above minRows, the
adaptive partial aggregate reports zero bypassed rows and a sort fallback in
codegen and interpreted mode, with and without the two-level map. A floor
counted across epochs would instead bypass once the cumulative count passed
minRows.
--
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]
