kosiew commented on code in PR #23227:
URL: https://github.com/apache/datafusion/pull/23227#discussion_r3548956886
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -476,9 +476,8 @@ impl ExternalSorter {
// reserved again for the next spill.
self.merge_reservation.free();
- // No coalescing on the spill path: it raises per-run peak memory.
let mut sorted_stream =
- self.in_mem_sort_stream(self.metrics.baseline.intermediate(),
false)?;
+ self.in_mem_sort_stream(self.metrics.baseline.intermediate(),
true)?;
Review Comment:
I think re-enabling run coalescing on the spill path can exceed the memory
cap right when the sorter is spilling because memory is already exhausted.
`coalesce_in_mem_batches_into_runs` calls `concat_batches` before any
successful reservation for the newly concatenated run, so the old buffered
batches and the new concatenated batch can coexist temporarily. The accounting
is only realigned afterward at lines 716-723.
This was previously avoided explicitly on the spill path, and the method
docs still say it is disabled there to keep peak memory low. Could we either
keep this as `false`, or make coalescing reserve and account for the extra
allocation before concatenating, with a spill-path memory-limit regression test?
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -662,6 +661,18 @@ impl ExternalSorter {
.build()
}
+ fn should_coalesce_single_column_sort(&self) -> Result<bool> {
Review Comment:
Small naming suggestion: `should_coalesce_single_column_sort` checks the
data type of the sort expression, not whether it is a column.
Could we name it something like `should_coalesce_single_primitive_sort_expr`
so future callers do not infer a column-only invariant that is not actually
enforced?
--
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]
