adriangb commented on code in PR #25910:
URL: https://github.com/apache/datafusion/pull/25910#discussion_r4446005797
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -998,6 +1008,38 @@ impl ExternalSorter {
}
}
+/// The bound on the in-memory size of each batch that [`ExternalSorter`]
spills.
+///
+/// The merge of the spill files starts with `sort_spill_reservation_bytes` of
headroom.
+/// It reads at least [`MIN_SPILL_MERGE_FAN_IN`] files, and reserves
+/// [`get_reserved_bytes_for_record_batch_size`] of the largest batch for
each, with
+/// read-ahead reduced to one batch under pressure. The bound is the largest
batch size for
+/// which that minimum merge fits in the headroom, found from the same formula.
+///
+/// The bound is at least [`MIN_SPILL_MAX_BATCH_BYTES`], so that a small or
zero headroom
+/// does not write, read and merge tiny batches. Below that headroom the merge
cannot fit
+/// in it anyway, and takes the rest from the memory pool, as it does without
a bound.
+fn spill_max_batch_bytes(sort_spill_reservation_bytes: usize) -> usize {
+ let fits = |batch: usize| {
+ MIN_SPILL_MERGE_FAN_IN *
get_reserved_bytes_for_record_batch_size(batch, batch)
+ <= sort_spill_reservation_bytes
+ };
+ // Binary search for the largest batch size that fits: `fits` is true up
to it.
+ let (mut fitting, mut too_large) = (0, sort_spill_reservation_bytes + 1);
Review Comment:
Done in d8a922f84, using your formula. It gives the same result as the
search. Added a boundary test for usize::MAX.
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -998,6 +1008,38 @@ impl ExternalSorter {
}
}
+/// The bound on the in-memory size of each batch that [`ExternalSorter`]
spills.
+///
+/// The merge of the spill files starts with `sort_spill_reservation_bytes` of
headroom.
+/// It reads at least [`MIN_SPILL_MERGE_FAN_IN`] files, and reserves
+/// [`get_reserved_bytes_for_record_batch_size`] of the largest batch for
each, with
+/// read-ahead reduced to one batch under pressure. The bound is the largest
batch size for
+/// which that minimum merge fits in the headroom, found from the same formula.
+///
+/// The bound is at least [`MIN_SPILL_MAX_BATCH_BYTES`], so that a small or
zero headroom
+/// does not write, read and merge tiny batches. Below that headroom the merge
cannot fit
+/// in it anyway, and takes the rest from the memory pool, as it does without
a bound.
+fn spill_max_batch_bytes(sort_spill_reservation_bytes: usize) -> usize {
+ let fits = |batch: usize| {
+ MIN_SPILL_MERGE_FAN_IN *
get_reserved_bytes_for_record_batch_size(batch, batch)
+ <= sort_spill_reservation_bytes
+ };
+ // Binary search for the largest batch size that fits: `fits` is true up
to it.
+ let (mut fitting, mut too_large) = (0, sort_spill_reservation_bytes + 1);
Review Comment:
Done in d8a922f84, using your formula. It gives the same result as the
search. Added a boundary test for `usize::MAX`.
--
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]
