Skip to content

test: add tests for spill file sizes to verify View GC - #21750

Merged
adriangb merged 3 commits into
apache:mainfrom
RatulDawar:test/spill-file-size-gc
Apr 20, 2026
Merged

test: add tests for spill file sizes to verify View GC#21750
adriangb merged 3 commits into
apache:mainfrom
RatulDawar:test/spill-file-size-gc

Conversation

@RatulDawar

@RatulDawar RatulDawar commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds unit tests to verify that StringView and BinaryView arrays are correctly compacted (garbage collected) before being spilled to disk.

The tests address Issue #21683 by:

  1. Creating "bloated" batches with large underlying buffers.
  2. Slicing them to a small number of rows (1%).
  3. Spilling them to disk.
  4. Asserting that the resulting file size is small (proving that GC removed the unused "ghost" data).

Test plan

  • Run cargo test -p datafusion-physical-plan --lib spill::mod::tests::test_spill_file_size_gc_verification_string_view
  • Run cargo test -p datafusion-physical-plan --lib spill::mod::tests::test_spill_file_size_gc_verification_binary_view

Made with Cursor

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Apr 20, 2026
@RatulDawar

RatulDawar commented Apr 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Currently the 10KB threshold to trigger GC makes it a requirement to replicate that in tests, wonder if we can make it parametermized so that we can controller the threshold to trigger gc.

PS : Got to learn about about loser tree while deep diving into spill and sort implementation and a nice talk about it. 😄

@RatulDawar
RatulDawar force-pushed the test/spill-file-size-gc branch from 7037620 to d31c248 Compare April 20, 2026 18:33
@RatulDawar

RatulDawar commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

@adriangb please take a look at it.

adriangb
adriangb approved these changes Apr 20, 2026
View reviewed changes

@adriangb adriangb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 😄

Comment thread datafusion/physical-plan/src/spill/mod.rs Outdated
Comment on lines +1432 to +1435
let strings: Vec<String> = (0..num_rows)
.map(|i| format!("this_is_a_long_string_to_ensure_it_is_not_inlined_and_causes_waste_{i}"))
.collect();
let string_array = StringViewArray::from(strings);

@adriangb adriangb Apr 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't need this later I wonder if we can avoid the allocation by feeding an iterator into the StringView or at least drop it immediately to free up memory.

@RatulDawar
test: optimize spill GC tests to use iterators
bf21e9e 
Update StringView and BinaryView spill tests to use iterators instead of intermediate Vec allocations, reducing unnecessary memory overhead during test setup.

Made-with: Cursor
@RatulDawar
chore: fix formatting
46b31df 
Made-with: Cursor
@adriangb
adriangb added this pull request to the merge queue Apr 20, 2026
Merged via the queue into apache:main with commit 7acbe03 Apr 20, 2026
35 checks passed
Rich-T-kid pushed a commit to Rich-T-kid/datafusion that referenced this pull request Apr 21, 2026
@RatulDawar @Rich-T-kid
test: add tests for spill file sizes to verify View GC (apache#21750)
86f8937 
## Summary
This PR adds unit tests to verify that `StringView` and `BinaryView`
arrays are correctly compacted (garbage collected) before being spilled
to disk.

The tests address [Issue
apache#21683](apache#21683) by:
1. Creating "bloated" batches with large underlying buffers.
2. Slicing them to a small number of rows (1%).
3. Spilling them to disk.
4. Asserting that the resulting file size is small (proving that GC
removed the unused "ghost" data).

## Test plan
- Run `cargo test -p datafusion-physical-plan --lib
spill::mod::tests::test_spill_file_size_gc_verification_string_view`
- Run `cargo test -p datafusion-physical-plan --lib
spill::mod::tests::test_spill_file_size_gc_verification_binary_view`

Made with [Cursor](https://cursor.com)
@adriangb adriangb mentioned this pull request Aug 21, 2026
Merged
zhuqi-lucas pushed a commit to zhuqi-lucas/arrow-datafusion that referenced this pull request Aug 22, 2026
@adriangb @claude
test: end-to-end regression tests for view array spill compaction (re…
32badcb 
…places apache#21325) (apache#24547)

## Which issue does this PR close?

- Replaces (and closes) apache#21325.

## Rationale for this change

apache#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 — apache#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. apache#21750
added unit tests for the compaction at the `SpillManager` level.

What remains from apache#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 apache#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.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adriangb adriangb adriangb approved these changes

Assignees

No one assigned

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@RatulDawar @adriangb