sunchao commented on PR #25800: URL: https://github.com/apache/datafusion/pull/25800#issuecomment-6455648003
Sorry for the late reply @tohuya6 , @andygrove and @alamb ! According to my Codex bot, it is still worthwhile to have the `sort_batch_stream`. Below is its reasoning. Please check if that makes sense ------------------------------ Thanks for checking. Deduplicating the shared buffers makes sense, but I found a case where we still need the workspace-loan fallback. With 4,096 Boolean rows and `batch_size = 1`, the input reservation is 1,024 bytes, while the sorted chunks retain 4,096 bytes of distinct buffers. Splitting the packed Boolean data into separate chunks causes real growth, so deduplication doesn’t eliminate it. I confirmed this with an `ExternalSorter` regression test: set the pool capacity to the spill workspace plus the input reservation, then insert a second batch to force spilling. The test passes before this PR (b7ccbbd), fails with `ResourcesExhausted` on this PR (c0e4d35), and passes when I restore only the loan while keeping the new deduplication. The passing versions return all 8,192 rows correctly sorted and release their reservations afterward. This is a small-batch edge case, but it shows that unique output memory can still exceed the input estimate. Could we keep the loan for any remaining excess after deduplication and add this regression test? That preserves the accounting improvement while allowing spilling to use workspace it already owns.v -- 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]
