Uh oh!
There was an error while loading. Please reload this page.
fix: reject max_buffered_batches_per_output_file values below 2 - #24204
Merged
2010YOUY01 merged 5 commits intoAug 11, 2026
Conversation
Setting datafusion.execution.max_buffered_batches_per_output_file to 0 or 1 panics at write time: two call sites (demux.rs, orchestration.rs) halve the value to size a bounded mpsc channel's capacity, and integer division rounds 0 or 1 down to 0, which Tokio's mpsc::channel rejects with "mpsc bounded channel requires buffer > 0". Reproduced directly: set datafusion.execution.max_buffered_batches_per_output_file = 1; COPY (SELECT 1) TO '/tmp/x.parquet'; -> panics Add ConfigMinTwoUsize, mirroring the existing ConfigNonZeroUsize pattern used for sibling fields (batch_size, meta_fetch_concurrency, etc.), so the value is rejected with a clear configuration error at set-time instead of panicking later. A plain non-zero check would not have been sufficient here since 1 also divides down to 0. Part of apache#17498
2010YOUY01
approved these changes
Aug 10, 2026
2010YOUY01
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thank you. I left a few minor and optional suggestions.
| /// round down to a zero-capacity buffer and panic. Invalid values return a | ||
| /// configuration error through [`ConfigField`] instead. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct ConfigMinTwoUsize(usize); |
Contributor
There was a problem hiding this comment.
Perhaps we can make the min value a configurable k, but it's not necessary now, we can reuse it when there is a need in other configurations.
| | datafusion.execution.minimum_parallel_output_files | 4 | Guarantees a minimum level of output files running in parallel. RecordBatches will be distributed in round robin fashion to each parallel writer. Each writer is closed and a new file opened once soft_max_rows_per_output_file is reached. | | ||
| | datafusion.execution.soft_max_rows_per_output_file | 50000000 | Target number of rows in output files when writing multiple. This is a soft max, so it can be exceeded slightly. There also will be one file smaller than the limit if the total number of rows written is not roughly divisible by the soft max | | ||
| | datafusion.execution.max_buffered_batches_per_output_file | 2 | This is the maximum number of RecordBatches buffered for each output file being worked. Higher values can potentially give faster write performance at the cost of higher peak memory consumption | | ||
| | datafusion.execution.max_buffered_batches_per_output_file | 2 | This is the maximum number of RecordBatches buffered for each output file being worked. Higher values can potentially give faster write performance at the cost of higher peak memory consumption. Must be at least 2: this value is halved to size internal buffering channels, and a value of 0 or 1 would create a zero-capacity channel and panic at write time. | |
Contributor
There was a problem hiding this comment.
I don't fully understand why it need to be halved, in the implementation it seems to be shared evenly between input and output file buffering:
Would be nice if we can explain it better.
ContributorAuthor
There was a problem hiding this comment.
Good catch, you're right that just saying "halved" wasn't enough. Found the original rationale in the demuxer design (#7791) — the budget is split between two independent points in the pipeline: how many files can be in flight from the demuxer to a writer task (orchestration.rs), and how many RecordBatches are buffered for a single file's own writer (demux.rs). Updated the doc comment to explain that explicitly instead of just stating the mechanical halving.
Addresses review feedback from @2010YOUY01 on apache#24204: the field's doc comment said the value gets halved but not why. Per the original demuxer design (apache#7791), the configured budget is split between two independent points in the write pipeline: how many files can be in flight from the demuxer to a writer task (orchestration.rs), and how many RecordBatches are buffered for a single file's own writer (demux.rs). Explain that split explicitly instead of just stating the mechanical halving.
…/DevShiba/datafusion into fix/max-buffered-batches-min-two
The information_schema.slt SHOW ALL VERBOSE test hardcodes every config's description text as expected output. Forgot to update it when the max_buffered_batches_per_output_file doc comment changed, which broke both the cargo test (amd64) and verify benchmark results (amd64) CI jobs (both run this same sqllogictest suite). Confirmed locally after initializing the testing/ submodules (git submodule update --init --recursive, which this environment hadn't done) that datafusion/sqllogictest/test_files/information_schema.slt now passes cleanly.
codecov-commenter
commented
Aug 11, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #24204 +/- ##
==========================================
- Coverage 81.06% 80.98% -0.08%
==========================================
Files 1106 1106 Lines 381891 383196 +1305 Branches 381891 383196 +1305 ==========================================
+ Hits 309578 310350 +772 - Misses 54034 54524 +490 - Partials 18279 18322 +43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
kosiew pushed a commit
to kosiew/datafusion
that referenced
this pull request
Aug 12, 2026
…he#24204) ## Which issue does this PR close? - Part of apache#17498 ## Rationale for this change ``` DataFusion CLI v54.1.0 > set datafusion.execution.max_buffered_batches_per_output_file = 1; > COPY (SELECT 1 as a) TO '/tmp/x.parquet'; thread 'tokio-rt-worker' panicked at datafusion/datasource/src/write/demux.rs:287:30: mpsc bounded channel requires buffer > 0 ``` Two call sites (`demux.rs::create_new_file_stream`, `orchestration.rs::spawn_writer_tasks_and_join`) divide `max_buffered_batches_per_output_file` in half to size a bounded `mpsc` channel's capacity. Integer division rounds both 0 *and* 1 down to 0, and Tokio's `mpsc::channel` panics on a zero capacity. A plain non-zero check (like the existing `ConfigNonZeroUsize`) would not have been sufficient here, since 1 also triggers the panic. A third call site (`demux.rs::hive_style_partitions_demuxer`) uses the raw value directly without dividing, so it would panic on 0 alone. ## What changes are included in this PR? Adds `ConfigMinTwoUsize`, mirroring the existing `ConfigNonZeroUsize` pattern already used for sibling fields (`batch_size`, `meta_fetch_concurrency`, `minimum_parallel_output_files`, etc.), and applies it to `max_buffered_batches_per_output_file`. Invalid values are now rejected with a clear configuration error at set-time instead of panicking later at write time. Updated the three read sites to call `.get()`, updated the field doc comment to explain the constraint, and regenerated `docs/source/user-guide/configs.md` via `dev/update_config_docs.sh`. ## Are these changes tested? Yes. Added two `statement error` cases to `datafusion/sqllogictest/test_files/set_variable.slt` (values 0 and 1), following the exact pattern already used for the sibling `ConfigNonZeroUsize` fields in that file. Verified manually with `datafusion-cli` that 0 and 1 now return a clean error instead of panicking, and that 2 (the default) and 3 still work correctly. Ran `cargo test -p datafusion-datasource --lib` (178 passed), the `set_variable.slt` sqllogictest suite, and `cargo check --workspace --all-targets` — all clean. ## Are there any user-facing changes? Yes: setting `datafusion.execution.max_buffered_batches_per_output_file` to `0` or `1` now returns a configuration error instead of panicking. No change for any value `>= 2` (including the default of 2).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Two call sites (
demux.rs::create_new_file_stream,orchestration.rs::spawn_writer_tasks_and_join) dividemax_buffered_batches_per_output_filein half to size a boundedmpscchannel's capacity. Integer division rounds both 0 and 1 down to 0, and Tokio'smpsc::channelpanics on a zero capacity. A plain non-zero check (like the existingConfigNonZeroUsize) would not have been sufficient here, since 1 also triggers the panic.A third call site (
demux.rs::hive_style_partitions_demuxer) uses the raw value directly without dividing, so it would panic on 0 alone.What changes are included in this PR?
Adds
ConfigMinTwoUsize, mirroring the existingConfigNonZeroUsizepattern already used for sibling fields (batch_size,meta_fetch_concurrency,minimum_parallel_output_files, etc.), and applies it tomax_buffered_batches_per_output_file. Invalid values are now rejected with a clear configuration error at set-time instead of panicking later at write time. Updated the three read sites to call.get(), updated the field doc comment to explain the constraint, and regenerateddocs/source/user-guide/configs.mdviadev/update_config_docs.sh.Are these changes tested?
Yes. Added two
statement errorcases todatafusion/sqllogictest/test_files/set_variable.slt(values 0 and 1), following the exact pattern already used for the siblingConfigNonZeroUsizefields in that file. Verified manually withdatafusion-clithat 0 and 1 now return a clean error instead of panicking, and that 2 (the default) and 3 still work correctly. Rancargo test -p datafusion-datasource --lib(178 passed), theset_variable.sltsqllogictest suite, andcargo check --workspace --all-targets— all clean.Are there any user-facing changes?
Yes: setting
datafusion.execution.max_buffered_batches_per_output_fileto0or1now returns a configuration error instead of panicking. No change for any value>= 2(including the default of 2).