2010YOUY01 commented on code in PR #15692:
URL: https://github.com/apache/datafusion/pull/15692#discussion_r2040878250
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -529,6 +523,12 @@ impl ExternalSorter {
/// Sorts the in-memory batches and merges them into a single sorted run,
then writes
/// the result to spill files.
async fn sort_and_spill_in_mem_batches(&mut self) -> Result<()> {
+ if self.in_mem_batches.is_empty() {
Review Comment:
> Now we handle this case in two places. I can remove the handling here
unless a more defensive approach is desired.
I think it's better to be more defensive, I'd prefer to throw an internal
error saying that we must assume the buffer is not empty when calling this
function.
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1552,6 +1571,62 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn test_batch_reservation_error() -> Result<()> {
Review Comment:
👍🏼
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -765,6 +765,25 @@ impl ExternalSorter {
Ok(())
}
+
+ /// Reserves memory to be able to accommodate the given batch.
+ /// If memory is scarce, tries to spill current in-memory batches to disk
first.
+ async fn reserve_memory_for_batch(&mut self, input: &RecordBatch) ->
Result<()> {
+ let size = get_reserved_byte_for_record_batch(input);
+
+ let result = self.reservation.try_grow(size);
+ if result.is_err() {
+ if self.in_mem_batches.is_empty() {
+ return result;
Review Comment:
It makes sense that we should preserve the raw resource error. In this case,
`ContextError` comes in handy:
https://github.com/apache/datafusion/blob/0b01fdf7f02f9097c319204058576f420b9790b4/datafusion/common/src/error.rs#L132
It can add extra notes to the original error message, to make it more
understandable.
Here, the extra note can be like "When the memory limit is reached and there
are no buffered batches to spill, external sort cannot continue. Consider
setting a higher memory limit."
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -765,6 +765,25 @@ impl ExternalSorter {
Ok(())
}
+
+ /// Reserves memory to be able to accommodate the given batch.
+ /// If memory is scarce, tries to spill current in-memory batches to disk
first.
+ async fn reserve_memory_for_batch(&mut self, input: &RecordBatch) ->
Result<()> {
Review Comment:
nit: I think a more obvious name for this function would be
`reserve_memory_for_batch_or_spill`
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
