andygrove opened a new pull request, #26435:
URL: https://github.com/apache/datafusion/pull/26435
## 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
(cf083e0815e03a88b1230cdb92ada716ec620027).
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.
--
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]
