Skip to content

minor: Validate batch_size configuration when setting it - #23054

Merged
2010YOUY01 merged 3 commits into
apache:mainfrom
2010YOUY01:safe-batch-size
Jun 22, 2026
Merged

minor: Validate batch_size configuration when setting it#23054
2010YOUY01 merged 3 commits into
apache:mainfrom
2010YOUY01:safe-batch-size

Conversation

@2010YOUY01

@2010YOUY01 2010YOUY01 commented Jun 20, 2026 •
edited by Jefffrey
Loading

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

In datafusion-cli:

DataFusion CLI v53.1.0
> set datafusion.execution.batch_size=0;
0 row(s) fetched.
Elapsed 0.001 seconds.

A '0' batch_size is a invalid config value, and it's widely used in most operators to control behavior. If operators don't validate batch_size, the 0 value might cause bugs/panics. This PR rejects it when setting the config.

I triggered a bug on my first attempt:

> set datafusion.execution.batch_size=0;
0 row(s) fetched.
Elapsed 0.000 seconds.

> select * from generate_series(100);
+-------+
| value |
+-------+
+-------+
0 row(s) fetched.
Elapsed 0.004 seconds.

What changes are included in this PR?

Implement a custom type for non-zero usize configuration value on batch_size, and rejects '0' value on initialization. Rust native NonZero<u32> is not used, because custom type allows to provide better error message on errors, and the implementation complexity is similar.

Note other invalid inputs like '-1' 'a' are already rejected and covered by existing tests.

Are these changes tested?

Yes, sqllogictest

Are there any user-facing changes?

No

@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate execution Related to the execution crate functions Changes to functions implementation physical-plan Changes to the physical-plan crate labels Jun 20, 2026
comphead
comphead reviewed Jun 20, 2026
View reviewed changes
Comment thread datafusion/common/src/config.rs Outdated
pub const fn new(value: usize) -> Self {
match NonZeroUsize::new(value) {
Some(value) => Self(value),
None => panic!("value must be greater than 0"),

@comphead comphead Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets return config_err it would super confusing if client crashes

@2010YOUY01 2010YOUY01 Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default constructor for configurations is wrapped inside config_namespace! macro, and it can't handle ? operator on result, so we have to panic here.

But I agree this unsafe API should not be exposed to the caller, so in b37fe74, a private helper is added for unsafe construction, its input is guaranteed to be always valid, since it's only for the hard-coded default value, outside caller can only use the safe try_new()

comphead
comphead reviewed Jun 20, 2026
View reviewed changes
Comment thread datafusion/common/src/config.rs

/// Creates a [`ConfigNonZeroUsize`], returning a configuration error if
/// `value` is zero.
pub fn try_new(value: usize) -> Result<Self> {

@comphead comphead Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe new with Result<Self> is more obvious choice?

@2010YOUY01 2010YOUY01 Jun 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment.

comphead
comphead approved these changes Jun 20, 2026
View reviewed changes

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @2010YOUY01 just some minors

comphead
comphead reviewed Jun 20, 2026
View reviewed changes
Comment thread datafusion/common/src/config.rs Resolved
comphead
comphead reviewed Jun 20, 2026
View reviewed changes
Comment thread datafusion/common/src/config.rs Resolved
@github-actions github-actions Bot added the auto detected api change Auto detected API change label Jun 21, 2026
@2010YOUY01

2010YOUY01 commented Jun 21, 2026

Copy link
Copy Markdown
Member Author

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

error: `cargo metadata` exited with an error:     Updating crates.io index
error: failed to get `wasm-streams` as a dependency of package `reqwest v0.12.28`
    ... which satisfies dependency `reqwest = "^0.12"` (locked to 0.12.28) of package `object_store v0.13.2`
    ... which satisfies dependency `object_store = "^0.13.2"` (locked to 0.13.2) of package `datafusion-benchmarks v54.0.0 (/home/runner/work/datafusion/datafusion/benchmarks)`

Caused by:
  failed to load source for dependency `wasm-streams`

Caused by:
  unable to update registry `crates-io`

Caused by:
  download of wa/sm/wasm-streams failed

Caused by:
  curl failed

Caused by:
  [16] Error in the HTTP2 framing layer

Looks like it's a false positive due to the flaky Github CI runner.

@Jefffrey Jefffrey removed the auto detected api change Auto detected API change label Jun 21, 2026
Jefffrey
Jefffrey reviewed Jun 21, 2026
View reviewed changes
Comment thread nonzero_test Outdated

@Jefffrey Jefffrey Jun 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this file?

@comphead comphead Jun 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

@2010YOUY01

2010YOUY01 commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review.

@2010YOUY01
2010YOUY01 added this pull request to the merge queue Jun 22, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 22, 2026
@2010YOUY01
2010YOUY01 added this pull request to the merge queue Jun 22, 2026
Merged via the queue into apache:main with commit 625359c Jun 22, 2026
38 checks passed
@2010YOUY01
2010YOUY01 deleted the safe-batch-size branch June 22, 2026 01:06
@2010YOUY01 2010YOUY01 mentioned this pull request Jul 15, 2026
Merged
adriangb pushed a commit to pydantic/datafusion that referenced this pull request Jul 15, 2026
@2010YOUY01
minor: validate config recursion_limit when setting it (apache#23592)
f3939df 
## Which issue does this PR close?

<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes apache#123` indicates that this PR will close issue apache#123.
-->

Follows up to apache#23054

- Part of apache#17498

## Rationale for this change

<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->
Similar to apache#23054, 0 value in `recursion_limit` is invalid, and carrying
such value to the SQL parsing logic can cause bugs or panics. A better
alternative would be to catch the invalid `0` at config value setting
step.

## What changes are included in this PR?

<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
1. Use a safe type `ConfigNonZeroUsize` instead of `usize` for this
configuration
2. Propogate changes to the implementation
3. Add one `.slt` test

## Are these changes tested?

<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
4. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->

## Are there any user-facing changes?

<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->

<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
kosiew pushed a commit to kosiew/datafusion that referenced this pull request Aug 12, 2026
@2010YOUY01 @kosiew
minor: validate config recursion_limit when setting it (apache#23592)
f1410b9 
## Which issue does this PR close?

<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes apache#123` indicates that this PR will close issue apache#123.
-->

Follows up to apache#23054

- Part of apache#17498

## Rationale for this change

<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->
Similar to apache#23054, 0 value in `recursion_limit` is invalid, and carrying
such value to the SQL parsing logic can cause bugs or panics. A better
alternative would be to catch the invalid `0` at config value setting
step.

## What changes are included in this PR?

<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
1. Use a safe type `ConfigNonZeroUsize` instead of `usize` for this
configuration
2. Propogate changes to the implementation
3. Add one `.slt` test

## Are these changes tested?

<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
4. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->

## Are there any user-facing changes?

<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->

<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
@tripleaceme tripleaceme mentioned this pull request Oct 19, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@comphead comphead comphead approved these changes
@Jefffrey Jefffrey Jefffrey approved these changes

Assignees

No one assigned

Labels

common Related to common crate core Core DataFusion crate execution Related to the execution crate functions Changes to functions implementation optimizer Optimizer rules physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt) v55.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@2010YOUY01 @comphead @Jefffrey @nuno-faria