test: end-to-end regression tests for view array spill compaction (replaces #21325) - #24547
Conversation
StringView/BinaryView batches produced by take/slice share the parent's data buffers; without gc before spilling, the IPC writer duplicates every referenced buffer per batch. The fix is centralized in InProgressSpillFile::append_batch, but nothing asserted end-to-end that the operator spill paths keep routing through it. Add spilling aggregation and sort queries over Utf8View/BinaryView data that assert total spilled_bytes stays proportional to the logical data size. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24547 +/- ##
==========================================
+ Coverage 81.24% 81.36% +0.12%
==========================================
Files 1113 1116 +3
Lines 392744 397509 +4765
Branches 392744 397509 +4765
==========================================
+ Hits 319090 323451 +4361
- Misses 54900 55116 +216
- Partials 18754 18942 +188 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jayzhan211
left a comment
There was a problem hiding this comment.
Any chance we could note why these numbers were picked? Not critical, but might help future readers. LGTM either way!
spilled_bytes > logical_bytes / 4,
spilled_bytes < logical_bytes * 3,
The 3x upper bound was never load-bearing: with gc_view_arrays disabled at batch_size 1024 the sort spill files only inflate to 1.66-1.73x, so the assertion could not fire in any configuration that completes. Amplification is proportional to the number of sliced batches sharing one parent buffer, so it is driven by batch_size (sort at an 8 MB pool, gc off: 9.34x at 256, 2.56x at 512, 1.68x at 1024, 1.04x at 8192). Run the sort test at batch_size 256 with an 8 MB pool, where compacted measures 1.0152x and regressed 9.3384x, and tighten the bound to 2x -- ~97% above the compacted ratio and ~4.7x below the regressed one. Both ratios reproduce byte for byte across runs, so the headroom is for portability rather than noise. Record the measurements behind each bound, including that the aggregation path has no configuration where a regression both completes and spills: it is caught by ResourcesExhausted instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Thanks for asking. They were arbitrary, and measuring showed the 3x bound could never actually fire (with gc_view_arrays disabled the sort spill files only inflate to 1.73x at batch_size=1024). I retuned the sort test to batch_size=256 / 8 MB pool where compacted measures 1.0152x against 9.3384x regressed, tightened the bound to 2x, and recorded those measurements inline. |
Which issue does this PR close?
Rationale for this change
#21325 set out to gc
StringViewArray/BinaryViewArraybatches 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'smain: the runtime fix is already fully superseded — #21633 centralized the fix by runninggc_view_arraysinInProgressSpillFile::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 theSpillManagerlevel.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_compactionmodule with two end-to-end regression tests that run spilling queries overUtf8View+BinaryViewdata — one through the hash aggregation spill path, one through the sort spill path — and assert the totalspilled_bytesstays proportional to the logical data size.Compared to the benchmark in #21325, per the review feedback there:
spilled_bytesfrom plan metrics instead of parsingEXPLAIN ANALYZEtextMeasured on
main(4.4 MB logical data):With
gc_view_arraysturned 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 withResourcesExhausted; 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.
🤖 Generated with Claude Code