Skip to content

Commit 67093a0

Browse files
test: cover sort preparation and workspace handoff
Use three focused tests to check producer completion, workspace handoff between preparation and merging, output and reservation cleanup, permit acquisition before spawning, and permit release on cancellation or error. Exercise the existing reduced-memory spill regression on a multithread runtime.
1 parent a75f2ac commit 67093a0

2 files changed

Lines changed: 121 additions & 1 deletion

File tree

‎datafusion/physical-plan/src/sorts/sort.rs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2330,6 +2330,46 @@ mod tests {
23302330
use insta::assert_snapshot;
23312331
use tokio::sync::Notify;
23322332

2333+
#[tokio::test]
2334+
async fn sort_preparation_waits_for_permit_and_cancels() {
2335+
let permits = Arc::new(Semaphore::new(1));
2336+
let held = permits.acquire().await.unwrap();
2337+
let stream = Box::pin(RecordBatchStreamAdapter::new(
2338+
Arc::new(Schema::empty()),
2339+
futures::stream::pending(),
2340+
));
2341+
let mut preparation =
2342+
ExternalSorter::prepare_sort_run(stream, Arc::clone(&permits)).boxed();
2343+
assert_is_pending(&mut preparation);
2344+
assert_eq!(Handle::current().metrics().num_alive_tasks(), 0);
2345+
drop(held);
2346+
assert_is_pending(&mut preparation);
2347+
// The current-thread runtime cannot run the spawned task until we yield.
2348+
assert_eq!(Handle::current().metrics().num_alive_tasks(), 1);
2349+
tokio::task::yield_now().await;
2350+
drop(preparation);
2351+
let _permit =
2352+
tokio::time::timeout(std::time::Duration::from_secs(10), permits.acquire())
2353+
.await
2354+
.expect("cancelled preparation must release its permit")
2355+
.unwrap();
2356+
}
2357+
2358+
#[tokio::test]
2359+
async fn sort_preparation_propagates_errors_and_releases_permit() {
2360+
let permits = Arc::new(Semaphore::new(1));
2361+
let stream = Box::pin(RecordBatchStreamAdapter::new(
2362+
Arc::new(Schema::empty()),
2363+
futures::stream::iter([Err(internal_datafusion_err!("sort failed"))]),
2364+
));
2365+
let error = ExternalSorter::prepare_sort_run(stream, Arc::clone(&permits))
2366+
.await
2367+
.err()
2368+
.expect("preparation must propagate the stream error");
2369+
assert!(error.to_string().contains("sort failed"), "{error}");
2370+
assert_eq!(permits.available_permits(), 1);
2371+
}
2372+
23332373
struct PendingWriteTempFileFactory {
23342374
write_started: Arc<Notify>,
23352375
aborted: Arc<Notify>,

‎datafusion/physical-plan/src/sorts/sort/spill_tests.rs‎

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ async fn assert_released(pool: &AdjustablePool, runtime: &RuntimeEnv) {
234234
assert_eq!(state.unchecked_over_limit, 0);
235235
}
236236

237-
#[tokio::test]
237+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
238238
async fn test_spill_preserves_merge_workspace_after_limit_decreases() -> Result<()> {
239239
let fixture = fixture()?;
240240
let pool = AdjustablePool::new(fixture.capacity);
@@ -522,6 +522,86 @@ async fn test_chunked_string_view_spill_can_use_workspace() -> Result<()> {
522522
check_chunked_string_view_workspace(true).await
523523
}
524524

525+
/// Two sort jobs share a tight memory budget. The first must return borrowed
526+
/// workspace when it hands off its first batch, so the second can prepare
527+
/// before the merge starts consuming output. During merging, looking ahead to
528+
/// the next batch must also return workspace for the merge's cursors.
529+
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
530+
async fn test_sort_preparation_returns_first_batch_workspace() -> Result<()> {
531+
let batch_size = 1024;
532+
let values: ArrayRef = Arc::new(StringViewArray::from_iter_values(
533+
(0..4096)
534+
.rev()
535+
.map(|i| format!("row-{i:08}-{}", "x".repeat(87))),
536+
));
537+
let batch = RecordBatch::try_from_iter([("key", values)])?;
538+
let ordering: LexOrdering = [PhysicalSortExpr::new_default(Arc::new(Column::new(
539+
"key", 0,
540+
)))]
541+
.into();
542+
let input_bytes = get_reserved_bytes_for_record_batch(&batch)?;
543+
let sorted_batches = sort_batch_chunked(&batch, &ordering, batch_size)?;
544+
let loan = sorted_batches
545+
.iter()
546+
.map(get_record_batch_memory_size)
547+
.sum::<usize>()
548+
- input_bytes;
549+
let first_bytes = get_record_batch_memory_size(&sorted_batches[0]);
550+
assert!(loan > first_bytes);
551+
552+
for consume_output in [false, true] {
553+
let workspace = if consume_output {
554+
// Also allow the merge's cursors, provided each run looks ahead.
555+
2 * loan + first_bytes
556+
} else {
557+
2 * loan - first_bytes
558+
};
559+
let runtime = RuntimeEnvBuilder::new()
560+
.with_memory_pool(Arc::new(GreedyMemoryPool::new(
561+
2 * input_bytes + workspace,
562+
)))
563+
.build_arc()?;
564+
let mut sorter = ExternalSorter::new(
565+
0,
566+
batch.schema(),
567+
ordering.clone(),
568+
batch_size,
569+
workspace,
570+
0,
571+
SpillCompression::Uncompressed,
572+
&ExecutionPlanMetricsSet::new(),
573+
Arc::clone(&runtime),
574+
)?;
575+
sorter.insert_batch(batch.clone()).await?;
576+
sorter.insert_batch(batch.clone()).await?;
577+
sorter.merge_reservation.free();
578+
let stream = sorter.in_mem_sort_stream(false, false).await?;
579+
// Buffered producers would leave two tasks waiting for the merge.
580+
let alive = tokio::runtime::Handle::current()
581+
.metrics()
582+
.num_alive_tasks();
583+
assert!(alive <= 1, "{alive} sort tasks remain before merging");
584+
assert_eq!(
585+
sorter.merge_pool.borrow(usize::MAX).size(),
586+
workspace - 2 * (loan - first_bytes)
587+
);
588+
if consume_output {
589+
let output: Vec<RecordBatch> = stream.try_collect().await?;
590+
let expected = sort_batch(
591+
&concat_batches(&batch.schema(), &[batch.clone(), batch.clone()])?,
592+
&ordering,
593+
None,
594+
)?;
595+
assert_eq!(concat_batches(&batch.schema(), &output)?, expected);
596+
} else {
597+
drop(stream);
598+
}
599+
drop(sorter);
600+
assert_eq!(runtime.memory_pool.reserved(), 0);
601+
}
602+
Ok(())
603+
}
604+
525605
#[tokio::test]
526606
async fn test_single_batch_spill_preserves_workspace_after_limit_decreases() -> Result<()>
527607
{

0 commit comments

Comments
 (0)