adriangb commented on issue #25791: URL: https://github.com/apache/datafusion/issues/25791#issuecomment-5853230531
Yes, please go ahead, thanks! I think you mean #22862 (the `RecordBatchMemoryCounter` fix for hash join build batches), and yes, this is the same problem in `ExternalSorter`. Two things I found while prototyping locally, in case they help: 1. Deduplicating the first-batch reservation is not enough on its own. `sort_batch_stream` charges each sorted output chunk with `get_record_batch_memory_size`, and every chunk still references the input's view buffers, so they are charged once per chunk. With only the first-batch fix, the repro above still fails with "Failed to allocate additional 109.7 MB for ExternalSorter[0] with 23.8 MB already allocated". Charging the sorted output through `RecordBatchMemoryCounter` fixes it. 2. That second change makes `test_single_batch_spill_returns_live_workspace_loan_on_drop` (from #24740) fail. The workspace loan there only happens when sorted output costs more than the input estimate, and with deduplication I could not find a normal input that does that. It may be worth checking with the #24740 author whether the loan path is still needed. Separately, compacting sparse view arrays (the `BatchCoalescer` 2x rule) when the sort's reservation fails with nothing to spill also rescues the first-batch case. That could be a follow-up if you want to keep the first PR small. -- 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]
