Add a SpillingPool to manage collections of spill files #18207

adriangb · 2025-10-21T18:08:17Z

Addresses #18014 (comment), potentially paves the path to solve #18011 for other operators as well

adriangb · 2025-10-21T18:15:54Z

Marking as draft for now. Open to input but needs a bit more work. I'm still familiarizing myself with the spilling infrastructure.

2010YOUY01 · 2025-10-22T03:20:54Z

This PR is setting size limit to spill files, when the size exceeds threshold, the spiller rotates to new file. I'm wondering why this design? Now the spill writer and reader is able to do streaming read/write, so a large spill file usually won't be the issue, unless it needs more parallelism somewhere.

adriangb · 2025-10-22T11:11:44Z

This PR is setting size limit to spill files, when the size exceeds threshold, the spiller rotates to new file. I'm wondering why this design? Now the spill writer and reader is able to do streaming read/write, so a large spill file usually won't be the issue, unless it needs more parallelism somewhere.

The issue with using a single FIFO file is that you accumulate dead data, bloating disk usage considerably. The idea is to cap that at say 100MB and then start a new file so that once all of the original file has been consumed we can garbage collect it.

adriangb · 2025-10-22T21:42:24Z

@2010YOUY01 let me know if that makes sense, there's an example of this issue in #18011

Copilot

Pull Request Overview

This PR introduces a SpillPool abstraction to centralize the management of spill files with FIFO semantics. The pool handles file rotation, batching multiple record batches into single files up to a configurable size limit, and provides streaming read access to spilled data.

Key changes:

Adds a new SpillPool module with FIFO queue semantics for managing spill files
Integrates SpillPool into RepartitionExec to replace the previous one-file-per-batch approach
Adds a new configuration option max_spill_file_size_bytes (default 100MB) to control when spill files rotate

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
datafusion/physical-plan/src/spill/spill_pool.rs	New module implementing SpillPool and SpillPoolStream with comprehensive tests
datafusion/physical-plan/src/spill/mod.rs	Exports the new spill_pool module
datafusion/physical-plan/src/repartition/mod.rs	Refactored to use SpillPool instead of one-file-per-batch spilling
datafusion/common/src/config.rs	Adds max_spill_file_size_bytes configuration option
docs/source/user-guide/configs.md	Documents the new max_spill_file_size_bytes configuration
datafusion/sqllogictest/test_files/information_schema.slt	Updates test expectations to include new configuration option

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

datafusion/physical-plan/src/repartition/mod.rs

datafusion/physical-plan/src/spill/spill_pool.rs

datafusion/physical-plan/src/repartition/mod.rs

2010YOUY01 · 2025-10-23T03:38:13Z

This PR is setting size limit to spill files, when the size exceeds threshold, the spiller rotates to new file. I'm wondering why this design? Now the spill writer and reader is able to do streaming read/write, so a large spill file usually won't be the issue, unless it needs more parallelism somewhere.

The issue with using a single FIFO file is that you accumulate dead data, bloating disk usage considerably. The idea is to cap that at say 100MB and then start a new file so that once all of the original file has been consumed we can garbage collect it.

This makes a lot of sense, operators should release disk usage sooner if possible.

I will to review it soon.

Copilot

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

2010YOUY01

Thank you, looks good in general.

I think it needs several additional test coverage:

e2e tests, potentially a query that triggers spilling in RepartitionExec. I think we can also do a quick benchmark on it to see how things work.
#18207 (comment)

I also left some suggestions to simplify the implementation, but they're optional.

2010YOUY01 · 2025-10-24T03:18:16Z

datafusion/common/src/config.rs

+        ///
+        /// A larger value reduces file creation overhead but may hold more disk space.
+        /// A smaller value creates more files but allows finer-grained space reclamation
+        /// (especially in LIFO mode where files are truncated after reading).


Now we're reclaiming disk space in the 'chunked file' granularity, perhaps this truncating way don't have to be mentioned, since it don't have a real usage yet.

yep was leftover from a previous implementation

2010YOUY01 · 2025-10-24T03:25:18Z

datafusion/physical-plan/src/spill/spill_pool.rs

+    /// Size of current write file in bytes (estimated)
+    current_write_size: usize,
+    /// Number of batches written to current file
+    current_batch_count: usize,


I think we can track them inside InProgressSpillFile, and expose an API. This approach can simplify SpillPool a bit.

2010YOUY01 · 2025-10-24T03:26:21Z

datafusion/physical-plan/src/spill/spill_pool.rs

+    /// SpillManager for creating files and tracking metrics
+    spill_manager: Arc<SpillManager>,
+    /// Schema for batches (used by SpillPoolStream to implement RecordBatchStream)
+    schema: SchemaRef,


To avoid duplication, the schema inside spill_manager can be used instead.

👍🏻 done

2010YOUY01 · 2025-10-24T03:28:49Z

datafusion/physical-plan/src/spill/spill_pool.rs

+    /// Shared reference to the spill pool
+    spill_pool: Arc<Mutex<SpillPool>>,
+    /// SpillManager for creating streams from spill files
+    spill_manager: Arc<SpillManager>,


Is it possible to use the spill_manager inside spill_pool, and eliminate this field?

2010YOUY01 · 2025-10-24T03:40:04Z

datafusion/physical-plan/src/repartition/mod.rs

+                        // Input finished and no more spilled data - we're done
+                        return Poll::Ready(None);
                    }
+                    // Otherwise check the channel


PerPartitionStream is for the order-preserving case of RepartitionExec, it seems a bit tricky to get the order correct, I recommend to find the existing tests for order-preserving repartition, and include spilling to it.

2010YOUY01 · 2025-10-24T03:56:38Z

datafusion/physical-plan/src/spill/spill_pool.rs

+
+        // Append batch to current file
+        if let Some(ref mut file) = self.current_write_file {
+            file.append_batch(batch)?;


A potential follow-up to do: #18261

adriangb · 2025-10-24T17:27:00Z

datafusion/sqllogictest/test_files/repartition.slt


 # End repartition on empty columns test
+
+# Start spilling tests


This test passes on main but fails on datafusion-cli v50:

❯ datafusion-cli DataFusion CLI v50.0.0 > CREATE UNBOUNDED EXTERNAL TABLE annotated_data_infinite2 ( a0 INTEGER, a INTEGER, b INTEGER, c INTEGER, d INTEGER ) STORED AS CSV WITH ORDER (a ASC, b ASC, c ASC) LOCATION 'datafusion/core/tests/data/window_2.csv' OPTIONS ('format.has_header' 'true'); SET datafusion.runtime.memory_limit = '12K'; EXPLAIN ANALYZE SELECT SUM(a) OVER(partition by a, b order by c) as sum1, SUM(a) OVER(partition by b, a order by c) as sum2, SUM(a) OVER(partition by a, d order by b) as sum3, SUM(a) OVER(partition by d order by a) as sum4 FROM annotated_data_infinite2; 0 row(s) fetched. Elapsed 0.026 seconds. 0 row(s) fetched. Elapsed 0.001 seconds. Resources exhausted: Additional allocation failed with top memory consumers (across reservations) as: RepartitionExec[Merge 11]#85(can spill: false) consumed 1896.0 B, peak 1896.0 B, RepartitionExec[Merge 5]#73(can spill: false) consumed 1448.0 B, peak 1448.0 B, RepartitionExec[Merge 3]#59(can spill: false) consumed 1384.0 B, peak 1384.0 B, RepartitionExec[Merge 1]#56(can spill: false) consumed 1304.0 B, peak 1304.0 B, RepartitionExec[8]#48(can spill: false) consumed 1216.0 B, peak 1856.0 B. Error: Failed to allocate additional 240.0 B for RepartitionExec[Merge 6] with 0.0 B already allocated for this reservation - 8.0 B remain available for the total pool

It's not this PR that enabled it to pass, it was #18014, but worth adding anyway.

adriangb requested a review from crepererum October 21, 2025 18:09

github-actions bot added sqllogictest SQL Logic Tests (.slt) common Related to common crate physical-plan Changes to the physical-plan crate labels Oct 21, 2025

adriangb marked this pull request as draft October 21, 2025 18:15

adriangb removed the request for review from crepererum October 21, 2025 18:15

github-actions bot added the documentation Improvements or additions to documentation label Oct 21, 2025

adriangb force-pushed the spilling-pool branch from 6b801ae to f7c84fe Compare October 22, 2025 19:58

adriangb added 7 commits October 22, 2025 16:35

implement SpillingPool

5b5016e

clippy

61e17da

use buffered stream

e9787f9

lint

ce6189f

rework

821ca81

Add some tests

903822a

fix lints

c5b40ee

adriangb force-pushed the spilling-pool branch from f7c84fe to c5b40ee Compare October 22, 2025 21:35

bugfix

2f7d4a4

adriangb requested a review from Copilot October 22, 2025 21:44

Copilot AI reviewed Oct 22, 2025

View reviewed changes

address pr feedback

1c6b1e8

adriangb added 5 commits October 22, 2025 23:23

fix build

823bdf6

fix dropped task

93c539b

updarte docstrings

0844a10

remove wrapper struct

ec7b997

hide stream behind a trait to avoid making more public stuff

8ee03c9

adriangb marked this pull request as ready for review October 23, 2025 05:35

adriangb requested a review from Copilot October 23, 2025 12:50

Copilot AI reviewed Oct 23, 2025

View reviewed changes

2010YOUY01 mentioned this pull request Oct 24, 2025

Move the spill pre-processing steps into SpillManager #18261

Open

2010YOUY01 reviewed Oct 24, 2025

View reviewed changes

2010YOUY01 mentioned this pull request Oct 24, 2025

feat: add baseline metrics tracking to RepartitionExec #18262

Draft

adriangb added 4 commits October 24, 2025 10:38

wip on cleanup

e7aa56d

add a detailed test

8acf40e

Add slt test

5b80175

use 128MB as the default spill file size

c57fbd8

adriangb force-pushed the spilling-pool branch from e7c1e6e to c57fbd8 Compare October 24, 2025 17:25

make SpillPool pub(crate)

6cbcf1e

adriangb commented Oct 24, 2025

View reviewed changes

adriangb added 3 commits October 24, 2025 12:31

fmt

ade30f8

lint

193685f

update slt

443b0b9


		# End repartition on empty columns test

		# Start spilling tests

Add a SpillingPool to manage collections of spill files #18207

Are you sure you want to change the base?

Add a SpillingPool to manage collections of spill files #18207

Conversation

adriangb commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented Oct 21, 2025

Uh oh!

2010YOUY01 commented Oct 22, 2025

Uh oh!

adriangb commented Oct 22, 2025

Uh oh!

adriangb commented Oct 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

2010YOUY01 commented Oct 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

2010YOUY01 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adriangb commented Oct 21, 2025 •

edited

Loading

2010YOUY01 left a comment •

edited

Loading

adriangb Oct 24, 2025 •

edited

Loading