adriangb opened a new pull request, #24547:
URL: https://github.com/apache/datafusion/pull/24547
## Which issue does this PR close?
- Replaces (and closes) #21325.
## Rationale for this change
#21325 set out to gc `StringViewArray`/`BinaryViewArray` batches in the hash
aggregation and sort-merge join spill paths to prevent spill write
amplification, but went stale with unaddressed review feedback. Re-deriving it
from scratch on today's `main`: the runtime fix is already fully superseded —
#21633 centralized the fix by running `gc_view_arrays` in
`InProgressSpillFile::append_batch`, which every spill write path (sort, all
aggregation streams, sort-merge join, nested-loop join, the repartition spill
pool) funnels through, covering both view types plus view arrays nested inside
container types. #21750 added unit tests for the compaction at the
`SpillManager` level.
What remains from #21325 — and what the review feedback on it asked for
("Unless we make assertions on the sizes … we won't catch regressions") — is
end-to-end coverage: nothing asserts that the operators with the heaviest
view-array spill traffic actually keep routing their spills through the
compaction. The existing e2e spill tests only assert `spilled_bytes <
disk_limit`, which would not catch a 10x view-buffer amplification.
## What changes are included in this PR?
Test-only: a new `memory_limit::view_spill_compaction` module with two
end-to-end regression tests that run spilling queries over `Utf8View` +
`BinaryView` data — one through the hash aggregation spill path, one through
the sort spill path — and assert the total `spilled_bytes` stays proportional
to the logical data size.
Compared to the benchmark in #21325, per the review feedback there:
- deterministic single runs with hard assertions instead of benchmark loops
with printouts
- reads `spilled_bytes` from plan metrics instead of parsing `EXPLAIN
ANALYZE` text
- bounds derived from the input data's logical size rather than magic
constants
Measured on `main` (4.4 MB logical data):
| Path | spill_count | spilled_bytes | ratio |
|---|---|---|---|
| sort | 7 | 4,428,968 | 1.007x |
| aggregate | 2 | 2,214,224 | 0.50x (spills part of its input) |
With `gc_view_arrays` turned into a no-op (simulated regression), both tests
fail: at these memory limits the un-compacted buffer sizes flow into the merge
memory estimates and both queries die with `ResourcesExhausted`; at looser
limits the spill files inflate instead (e.g. sort: 7,553,600 bytes, 1.7x at a
12 MB pool) and grow past the asserted bound as runs get sliced more finely.
Either failure mode trips the tests.
## Are these changes tested?
They are tests. Verified both directions: green on `main`, red with
compaction disabled.
## Are there any user-facing changes?
No.
--
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]
