Repository navigation
Commit c0fa2f2
## Which issue does this PR close?
- Fixes #17340 on the `branch-55` release line.
- Backports the merged fix in #24592.
## Rationale for this change
In 55.x, a batch read back from a spill file can hold far more memory
than was recorded for it when it was
written: up to twice as much when its IPC message spans two of the 128
KiB chunks that `SpillReaderStream` reads,
and the whole 128 KiB chunk when a small batch fits inside one.
Operators that read spills back budget memory from
the size recorded at spill time, among them `SortExec`'s multi-level
merge and the spilling aggregate, so under a
memory limit they hold memory the pool does not know about. The `Record
batch memory usage (...) exceeds the
expected limit` warnings reported on #17340 come from this.
#24592 fixed it on `main`, but it is not in 55.1.0 or 55.2.0, and it
first ships in 56.0.0 (#24461). This
backport is for a 55.3.0 patch release, if one is made.
DataFusion Comet runs on 55.2.0 and hits this. In its spilling shuffled
hash join (apache/datafusion-comet#7057),
a spilled build side read back in batches of 256 KiB held up to twice
what it held when written, so the join now
copies every batch it reads back into buffers of their own size. Comet's
sorts and aggregates spill through the
same reader.
## Past backports of spill fixes
Fixes to spilling have been backported to earlier release lines:
| Release line | Backport | Fix |
| ------------ | -------- |
--------------------------------------------------------------------------------
|
| `branch-54` | #23654 | Lost wakeup in `SpillPoolReader` with multiple
concurrent readers (#23522) |
| `branch-53` | #21451 | Use the spill writer's schema instead of the
first batch's for spill files (#21293) |
| `branch-53` | #20918 | Duplicate group keys after hash aggregation
spill (#20724, #20858) |
| `branch-53` | #20792 | Repartition dropping data when spilling
(#20672) |
| `branch-52` | #21403 | Use the spill writer's schema instead of the
first batch's for spill files (#21293) |
| `branch-52` | #20917 | Duplicate group keys after hash aggregation
spill (#20724, #20858) |
| `branch-52` | #20777 | Repartition dropping data when spilling
(#20672) |
| `branch-52` | #20684 | Arrow spill underrun when reading spill files
back (#20159) |
#20684 is the closest: like this one, it fixed how spill files are read
back.
## What changes are included in this PR?
- A cherry-pick with `git cherry-pick -x` of the commit that #24592
merged (cf083e0).
It applies to `branch-55` without conflicts, and this PR changes nothing
else.
- `SpillReaderStream` reassembles each IPC message into allocations
sized from the message's own headers before
decoding it, so a decoded batch pins only its own bytes. The change is
confined to
`datafusion/physical-plan/src/spill/mod.rs`, with the tests from #24592.
## Are these changes tested?
Yes, with the tests that #24592 added, on `branch-55`:
- `cargo fmt --all -- --check`
- `cargo clippy -p datafusion-physical-plan --all-targets --all-features
-- -D warnings`
- `cargo test -p datafusion-physical-plan --lib spill`: 93 passed,
including the five tests #24592 added
(`test_read_back_does_not_inflate_batch_memory`,
`test_stringview_spill_read_back_memory_accounting`,
`test_message_framer_across_chunk_boundaries`,
`test_message_framer_reports_truncation_like_the_decoder` and
`test_truncated_spill_file_errors`)
- `cargo test -p datafusion-physical-plan --lib sorts`: 81 passed
- `cargo test -p datafusion-physical-plan --lib aggregates`: 179 passed
- `cargo test -p datafusion --test core_integration memory_limit`: 31
passed
## Are there any user-facing changes?
No API changes. Queries that spill hold less memory when they read their
spills back, and the warning from #17340
no longer fires for this cause.
Co-authored-by: Jay Zhan <[email protected]>
1 parent 500744d commit c0fa2f2
1 file changed
Lines changed: 629 additions & 7 deletions
0 commit comments