comphead commented on code in PR #26953:
URL: https://github.com/apache/datafusion/pull/26953#discussion_r4495950336
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1023,9 +1035,10 @@ impl ExternalSorter {
}
let expressions = self.expr.clone();
- let batch_size = self.batch_size;
let merge_pool = Arc::clone(&self.merge_pool);
let elapsed_compute = self.metrics.baseline.elapsed_compute().clone();
+ // Set when `merge_run_batch_size` split this run
+ let split = batch_size < self.batch_size;
Review Comment:
Thanks, applied in 093816758. `views_share_bytes` sits next to
`gc_view_arrays`, and `sort_run` only checks it when the run would be split, so
sorts without a memory limit don't scan the views. The dictionary-backed
`Utf8View` payload is a case of `test_merge_keys_larger_than_batches`, without
the pool checks as you suggested, and without the change it fails with `Not
enough memory to continue external sort`. The boundary test is
`test_views_share_bytes` in `spill/mod.rs`. Your query now passes at 256M, 512M
and 1G with the same spill counts as `main`.
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1036,7 +1049,16 @@ impl ExternalSorter {
let schema = batch.schema();
// Sort the batch immediately and get all output batches
- let sorted_batches = sort_batch_chunked(&batch, &expressions,
batch_size)?;
+ let mut sorted_batches =
+ sort_batch_chunked(&batch, &expressions, batch_size)?;
+ // `take` keeps the data buffers of view arrays, so splitting would
+ // charge every batch for all of them.
+ if split && sorted_batches.len() > 1 {
Review Comment:
Applied as suggested in 093816758, and the doc comment of
`compact_view_array` now covers it. The two clustered `Utf8View` keys with room
for 8 batches fail without it and pass now. Your query passes at 32M and 64M
with the same 507 and 113 spills as `main`. Since this also changes spilling
for other operators, I checked a `GROUP BY` on a `Utf8View` column at 64M and
128M, which spills the same as on `main`. A `BinaryView` payload, `Utf8View`
keys with overlapping runs, shared and unique views in one table, nine `Int64`
keys and a single `FixedSizeBinary(16)` key also match or beat `main`.
--
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]
