Skip to content

Commit cc4945a

Browse files
committed
fix: ensure a maximum of buffer_len RecordBatches are cached in spawn_buffered
Fixes: #23559
1 parent 79692c1 commit cc4945a

1 file changed

Lines changed: 63 additions & 6 deletions

File tree

‎datafusion/physical-plan/src/common.rs‎

Lines changed: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,8 @@ pub fn project_plan_to_schema(
181181
}
182182

183183
/// If running in a tokio context spawns the execution of `stream` to a separate task
184-
/// allowing it to execute in parallel with an intermediate buffer of size `buffer`
184+
/// allowing it to execute in parallel with an intermediate buffer of size `buffer`.
185+
/// At most `buffer` record batches will be produced ahead of the consumer.
185186
pub fn spawn_buffered(
186187
mut input: SendableRecordBatchStream,
187188
buffer: usize,
@@ -196,11 +197,12 @@ pub fn spawn_buffered(
196197
let sender = builder.tx();
197198

198199
builder.spawn(async move {
199-
while let Some(item) = input.next().await {
200-
if sender.send(item).await.is_err() {
201-
// Receiver dropped when query is shutdown early (e.g., limit) or error,
202-
// no need to return propagate the send error.
203-
return Ok(());
200+
while let Ok(permit) = sender.reserve().await {
201+
// Receiver dropped when query is shutdown early (e.g., limit) or error,
202+
// no need to return propagate the send error.
203+
match input.next().await {
204+
Some(item) => permit.send(item),
205+
None => break,
204206
}
205207
}
206208

@@ -554,4 +556,59 @@ mod tests {
554556
let err = project_plan_to_schema(input, &expected_schema).unwrap_err();
555557
assert!(err.to_string().contains("schema metadata differ"));
556558
}
559+
560+
/// Verifies that `spawn_buffered` holds exactly `buffer + 1` record batches in memory
561+
/// when no receiver is polling: `buffer` slots in the channel and 1 held in the
562+
/// suspended `send()` future.
563+
#[tokio::test(flavor = "multi_thread")]
564+
async fn test_spawn_buffered_max_in_flight_batches() {
565+
use crate::stream::RecordBatchStreamAdapter;
566+
use arrow::array::Int32Array;
567+
use futures::stream;
568+
use std::sync::atomic::{AtomicUsize, Ordering};
569+
570+
let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
571+
let buffer_size = 1;
572+
let num_batches = 10;
573+
574+
let produced_count = Arc::new(AtomicUsize::new(0));
575+
let produced_clone = Arc::clone(&produced_count);
576+
let schema_clone = Arc::clone(&schema);
577+
578+
// Stream increments the counter each time a batch is pulled by the producer.
579+
let input_stream = stream::unfold(0usize, move |i| {
580+
let schema = Arc::clone(&schema_clone);
581+
let counter = Arc::clone(&produced_clone);
582+
async move {
583+
if i >= num_batches {
584+
return None;
585+
}
586+
let batch = RecordBatch::try_new(
587+
Arc::clone(&schema),
588+
vec![Arc::new(Int32Array::from(vec![i as i32]))],
589+
)
590+
.unwrap();
591+
counter.fetch_add(1, Ordering::SeqCst);
592+
Some((Ok(batch), i + 1))
593+
}
594+
});
595+
596+
let input = Box::pin(RecordBatchStreamAdapter::new(
597+
Arc::clone(&schema),
598+
input_stream,
599+
));
600+
// Drop the returned stream immediately so no receiver is ever polled.
601+
let _buffered = spawn_buffered(input, buffer_size);
602+
603+
// Give the producer task time to fill the channel and stall on send().
604+
tokio::time::sleep(std::time::Duration::from_millis(100)).await;
605+
606+
// With reserve(), the producer acquires a channel slot before pulling from
607+
// input, so at most buffer_size batches can exist in memory simultaneously.
608+
assert_eq!(
609+
produced_count.load(Ordering::SeqCst),
610+
buffer_size,
611+
"expected exactly {buffer_size} batch(es) in memory with no receiver polling"
612+
);
613+
}
557614
}

0 commit comments

Comments
 (0)