comphead commented on code in PR #26960:
URL: https://github.com/apache/datafusion/pull/26960#discussion_r4457937981
##########
datafusion/execution/src/disk_manager.rs:
##########
@@ -578,7 +584,7 @@ impl SpillFile for RefCountedTempFile {
.try_clone()
.map_err(DataFusionError::IoError)?;
Ok(Box::new(FileSpillWriter {
- file,
+ file: BufWriter::with_capacity(SPILL_WRITE_BUFFER_SIZE, file),
Review Comment:
Thanks, agreed. Restored the per-batch coalescing from your diff in
03a6c93f4, so `disk_manager.rs` is unchanged now. `aggregate_memory_spill.slt`
reports a 2.2 MB peak-memory drift with it, against 5.6 MB with the `BufWriter`.
##########
datafusion/physical-plan/src/spill/mod.rs:
##########
@@ -677,14 +700,25 @@ impl IPCStreamEncoder {
let compression_type =
Option::<CompressionType>::from(spill_compression);
write_options = write_options.try_with_compression(compression_type)?;
- let encoder = StreamEncoder::try_new_with_options(schema,
write_options)?;
+ // Lengths take as much space as offsets when uncompressed
+ let lengths_schema =
+ compression_type.and_then(|_| lengths::encode_schema(schema));
Review Comment:
Added `MIN_LENGTHS_ROWS` and the boundary test as you wrote them, in
03a6c93f4. The #26627 reproducer with `batch_size = 64` now spills the same
bytes as `main` with both codecs, and at 128 rows it still saves 25% with lz4
(70.3 to 52.5 MB) and 1% with zstd. I also set `batch_size = 8192` for the byte
key case in `aggregate_memory_spill.slt`, since its 128-row batches sat exactly
at the threshold.
--
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]
