Nachiket-Roy commented on PR #19695: URL: https://github.com/apache/datafusion/pull/19695#issuecomment-3724023233
@2010YOUY01 @EmilyMatt Thanks for the detailed review, I understand the problem more clearly now. This PR is not intended to make impossible cases possible, nor to bypass memory limits. If sorting a single batch truly requires more memory than is available (e.g. 1 GB of sort keys with only 100 MB available), the operation will still fail. The intent of this change is narrower: to remove an unnecessary additional memory peak caused by buffering the entire sorted output before spilling. You are absolutely right that the current implementation of `sort_batch_chunked` is eager. It materializes all sorted chunks into a `Vec<RecordBatch>` before spilling, which means peak memory can still approach roughly `original batch + sort buffers + chunked output`, rather than strictly `original batch + a small output buffer`. In that sense this PR does not fully address the worst-case memory behavior you described. I also agree with the broader point that emitting arbitrarily large batches violates the implicit `batch_size` convention used across operators, and that `AggregateExec` is a primary source of such oversized batches today. Addressing this upstream (e.g. by adding internal spilling or chunked emission in `AggregateExec`) would significantly reduce the need for defensive handling in downstream operators. These seem to be complementary concerns: - This PR fixes a correctness gap in `SortExec` where progress was impossible even when memory pressure could have been handled incrementally. - A follow-up change to make `AggregateExec` (and other emitting operators) respect `batch_size` would address the root cause and simplify downstream operators. I agree that the approach you outlined -> computing sort indices once and then performing `take` on `batch_size`-sized index slices and spilling each chunk immediately, is the correct long-term direction for `SortExec`. That improvement can be covered in a follow-up PR. -- 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]
