comphead commented on code in PR #26953:
URL: https://github.com/apache/datafusion/pull/26953#discussion_r4457851904
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1078,6 +1087,23 @@ impl ExternalSorter {
})
}
+ /// Batch size of the sorted runs that feed a merge. A multi-column merge
holds
+ /// the row-format 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`.
+ fn merge_run_batch_size(&self) -> usize {
Review Comment:
Thanks, confirmed and fixed in 571f7f986. Split runs now go through
`gc_view_arrays`, and a `Utf8View` payload is a case of
`test_merge_keys_larger_than_batches`. I limited the compaction to runs split
for the merge. Runs chunked only because they exceed `batch_size` keep their
current charge, and the three workspace-loan tests stay unchanged, since they
create the loan with those unsplit chunks.
A high-cardinality dictionary payload failed the same way, and
`gc_view_arrays` cannot compact shared dictionary values. So schemas whose
split batches would still share buffers keep `batch_size`, and that payload is
a test case too.
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1078,6 +1087,23 @@ impl ExternalSorter {
})
}
+ /// Batch size of the sorted runs that feed a merge. A multi-column merge
holds
+ /// the row-format 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`.
+ fn merge_run_batch_size(&self) -> usize {
+ let limited = !matches!(
+ self.runtime.memory_pool.memory_limit(),
+ MemoryLimit::Infinite
+ );
+ if limited && self.expr.len() > 1 {
Review Comment:
Good catch, thanks. `merges_in_row_format` now sits next to
`StreamingMergeBuilder::build`, with a note in `build` that the two must match,
and `merge_run_batch_size` uses it. A single `FixedSizeBinary(16)` key is a
case of `test_merge_keys_larger_than_batches`. It fails on `main` and on the
previous head, and passes now.
--
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]
