You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "DevShiba (via GitHub)" <gi...@apache.org> on 2026/08/09 12:44:10 UTC
[PR] fix: reject max_buffered_batches_per_output_file values below 2 [datafusion]
DevShiba opened a new pull request, #24204:
URL: https://github.com/apache/datafusion/pull/24204
## Which issue does this PR close?
- Part of https://github.com/apache/datafusion/issues/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 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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: reject max_buffered_batches_per_output_file values below 2 [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 merged PR #24204:
URL: https://github.com/apache/datafusion/pull/24204
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: reject max_buffered_batches_per_output_file values below 2 [datafusion]
Posted by "DevShiba (via GitHub)" <gi...@apache.org>.
DevShiba commented on code in PR #24204:
URL: https://github.com/apache/datafusion/pull/24204#discussion_r3746213214
##########
docs/source/user-guide/configs.md:
##########
@@ -130,7 +130,7 @@ The following configuration settings are available:
| datafusion.execution.meta_fetch_concurrency | 32 | Number of files to read in parallel when inferring schema and statistics
|
| 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.
|
Review 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.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: reject max_buffered_batches_per_output_file values below 2 [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #24204:
URL: https://github.com/apache/datafusion/pull/24204#discussion_r3746002094
##########
datafusion/common/src/config.rs:
##########
@@ -663,6 +663,91 @@ impl Display for ConfigNonZeroUsize {
}
}
+/// A `usize` configuration value that rejects 0 and 1 when set from strings.
+///
+/// Use this for options whose consumer divides the value in half to size an
+/// internal buffer (e.g. a bounded channel capacity): values below 2 would
+/// 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);
Review 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.
##########
docs/source/user-guide/configs.md:
##########
@@ -130,7 +130,7 @@ The following configuration settings are available:
| datafusion.execution.meta_fetch_concurrency | 32 | Number of files to read in parallel when inferring schema and statistics
|
| 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.
|
Review 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: https://github.com/apache/datafusion/blob/bc48a4f7acd87a9779cb5a88d9b1a3f4ce506e45/datafusion/datasource/src/write/demux.rs#L287
Would be nice if we can explain it better.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: reject max_buffered_batches_per_output_file values below 2 [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24204:
URL: https://github.com/apache/datafusion/pull/24204#issuecomment-5248006882
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24204?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `73.17073%` with `11 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.98%. Comparing base ([`918013e`](https://app.codecov.io/gh/apache/datafusion/commit/918013e8e5b4bb3766ac40c3657244a790351670?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`21af09c`](https://app.codecov.io/gh/apache/datafusion/commit/21af09cfa0d364b93a73343a5f66706df42d81f3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 18 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24204?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/common/src/config.rs](https://app.codecov.io/gh/apache/datafusion/pull/24204?src=pr&el=tree&filepath=datafusion%2Fcommon%2Fsrc%2Fconfig.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL2NvbmZpZy5ycw==) | 70.27% | [10 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24204?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24204?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org