comphead commented on code in PR #26953:
URL: https://github.com/apache/datafusion/pull/26953#discussion_r4515674393
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1078,6 +1106,31 @@ impl ExternalSorter {
})
}
+ /// Batch size of the sorted runs that feed a merge. A merge in the row
format
+ /// holds the keys of every run's current batch at once, and these keys
can be
+ /// larger than the batches. With a memory limit, runs therefore use an
eighth
+ /// of `batch_size`, but at least 1024 rows to keep the per-batch merge
cost
+ /// small. The merge output still uses `batch_size`. Runs keep `batch_size`
+ /// when their batches would share buffers other than the data of top-level
+ /// view columns, such as dictionary values, since every batch is charged
for
+ /// them. `sort_run` also keeps it for a run whose views share bytes.
+ fn merge_run_batch_size(&self) -> usize {
Review Comment:
Thanks, this is a good catch. Applied in 8eb93eb64, with one change:
`merge_run_batch_size` checks the key size before `views_share_bytes`, so runs
that keep `batch_size` skip the scan of their views. With your repro at `-m
16G`, the previous version was 14% slower than `main` with 35% more peak RSS,
and this one is within 4% on both. The issue's reproducer and the
memory-limited probes from the earlier rounds still pass with the same spill
counts. Your boundary test is in as
`test_merge_run_batch_size_follows_key_size`.
For variable-length keys the estimate counts their bytes without the row
format's padding, so string keys that only slightly outgrow a run are not
split, as on `main`. I noted that under the known limitations.
##########
datafusion/physical-plan/src/spill/mod.rs:
##########
@@ -836,7 +838,9 @@ fn compact_view_array<T: ByteViewType>(
let (remap, unique) = dedup_buffer_entries(buffers);
let unique_size: usize = unique.iter().map(|b| b.len()).sum();
- if should_gc_view_array(array, unique_size) {
+ if should_gc_view_array(array, unique_size)
+ || (unique.len() > 1 && array.total_buffer_bytes_used() <= unique_size)
Review Comment:
Added as `test_gc_keeps_shared_views_in_two_buffers` in 8eb93eb64. It fails
when the condition is only `unique.len() > 1`. Thanks!
--
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]
