You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "2010YOUY01 (via GitHub)" <gi...@apache.org> on 2026/06/20 12:21:54 UTC
[PR] minor: Validate `batch_size` configuration when setting it [datafusion]
2010YOUY01 opened a new pull request, #23054:
URL: https://github.com/apache/datafusion/pull/23054
## 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 #123` indicates that this PR will close issue #123.
-->
- Closes #.
## 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.
-->
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?
<!--
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.
-->
Implement a custom type for non-zero usize. Rust native `NonZero<u32>` is not used, because custom type allows to provide better error message on errors, and the implementation complexity is similar.
## 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
3. 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)?
-->
Yes, sqllogictest
## 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.
-->
No
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447291918
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
Review Comment:
lets return `config_err` it would super confusing if client crashes
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447784607
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
Review Comment:
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](https://github.com/apache/datafusion/pull/23054/commits/b37fe74d71df09bcc58b4b03e9e3ca4d8a7a3188), 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()`
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447784607
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
Review Comment:
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 be exposed to the caller, so in [b37fe74](https://github.com/apache/datafusion/pull/23054/commits/b37fe74d71df09bcc58b4b03e9e3ca4d8a7a3188), 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()`
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447320149
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
+ }
+ }
+
+ /// Creates a [`ConfigNonZeroUsize`], returning a configuration error if
+ /// `value` is zero.
+ pub fn try_new(value: usize) -> Result<Self> {
+ NonZeroUsize::new(value).map(Self).ok_or_else(|| {
+ DataFusionError::Configuration("value must be greater than 0".to_string())
Review Comment:
```suggestion
config_datafusion_err!("value must be greater than 0".to_string())
```
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447785011
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
+ }
+ }
+
+ /// Creates a [`ConfigNonZeroUsize`], returning a configuration error if
+ /// `value` is zero.
+ pub fn try_new(value: usize) -> Result<Self> {
Review Comment:
See above comment.
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447782521
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
+ }
+ }
+
+ /// Creates a [`ConfigNonZeroUsize`], returning a configuration error if
+ /// `value` is zero.
+ pub fn try_new(value: usize) -> Result<Self> {
+ NonZeroUsize::new(value).map(Self).ok_or_else(|| {
+ DataFusionError::Configuration("value must be greater than 0".to_string())
Review Comment:
[b37fe74](https://github.com/apache/datafusion/pull/23054/commits/b37fe74d71df09bcc58b4b03e9e3ca4d8a7a3188)
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447931136
##########
nonzero_test:
##########
Review Comment:
what is this file?
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 merged PR #23054:
URL: https://github.com/apache/datafusion/pull/23054
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447295760
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
+ }
+ }
+
+ /// Creates a [`ConfigNonZeroUsize`], returning a configuration error if
+ /// `value` is zero.
+ pub fn try_new(value: usize) -> Result<Self> {
Review Comment:
maybe new with `Result<Self>` is more obvious choice?
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#issuecomment-4763735311
Thanks for the review.
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447782717
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
+ }
+ }
+
+ /// Creates a [`ConfigNonZeroUsize`], returning a configuration error if
+ /// `value` is zero.
+ pub fn try_new(value: usize) -> Result<Self> {
+ NonZeroUsize::new(value).map(Self).ok_or_else(|| {
+ DataFusionError::Configuration("value must be greater than 0".to_string())
+ })
+ }
+
+ /// Returns the wrapped `usize`.
+ pub const fn get(self) -> usize {
+ self.0.get()
+ }
+}
+
+impl From<ConfigNonZeroUsize> for usize {
+ fn from(value: ConfigNonZeroUsize) -> Self {
+ value.get()
+ }
+}
+
+impl FromStr for ConfigNonZeroUsize {
+ type Err = DataFusionError;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ Self::try_new(default_config_transform(s)?)
+ }
+}
+
+impl ConfigField for ConfigNonZeroUsize {
+ fn visit<V: Visit>(&self, v: &mut V, key: &str, description: &'static str) {
+ v.some(key, self, description)
+ }
+
+ fn set(&mut self, key: &str, value: &str) -> Result<()> {
+ if !key.is_empty() {
+ return _config_err!(
+ "Config field is a scalar ConfigNonZeroUsize and does not have nested field \"{}\"",
Review Comment:
[b37fe74](https://github.com/apache/datafusion/pull/23054/commits/b37fe74d71df09bcc58b4b03e9e3ca4d8a7a3188)
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#issuecomment-4760612032
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
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
```
</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
Re: [PR] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447317118
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
+ }
+ }
+
+ /// Creates a [`ConfigNonZeroUsize`], returning a configuration error if
+ /// `value` is zero.
+ pub fn try_new(value: usize) -> Result<Self> {
+ NonZeroUsize::new(value).map(Self).ok_or_else(|| {
+ DataFusionError::Configuration("value must be greater than 0".to_string())
+ })
+ }
+
+ /// Returns the wrapped `usize`.
+ pub const fn get(self) -> usize {
+ self.0.get()
+ }
+}
+
+impl From<ConfigNonZeroUsize> for usize {
+ fn from(value: ConfigNonZeroUsize) -> Self {
+ value.get()
+ }
+}
+
+impl FromStr for ConfigNonZeroUsize {
+ type Err = DataFusionError;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ Self::try_new(default_config_transform(s)?)
+ }
+}
+
+impl ConfigField for ConfigNonZeroUsize {
+ fn visit<V: Visit>(&self, v: &mut V, key: &str, description: &'static str) {
+ v.some(key, self, description)
+ }
+
+ fn set(&mut self, key: &str, value: &str) -> Result<()> {
+ if !key.is_empty() {
+ return _config_err!(
+ "Config field is a scalar ConfigNonZeroUsize and does not have nested field \"{}\"",
Review Comment:
would be nice to add config field name here
##########
datafusion/common/src/config.rs:
##########
@@ -582,6 +583,87 @@ impl Display for SpillCompression {
}
}
+/// A `usize` configuration value that rejects zero when set from strings.
+///
+/// Use this for options where zero is never a meaningful runtime value. The
+/// default should be constructed with [`ConfigNonZeroUsize::new`] so invalid
+/// defaults fail at compile/test time, while user-provided values return a
+/// configuration error through [`ConfigField`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ConfigNonZeroUsize(NonZeroUsize);
+
+impl ConfigNonZeroUsize {
+ /// Creates a [`ConfigNonZeroUsize`], panicking if `value` is zero.
+ pub const fn new(value: usize) -> Self {
+ match NonZeroUsize::new(value) {
+ Some(value) => Self(value),
+ None => panic!("value must be greater than 0"),
+ }
+ }
+
+ /// Creates a [`ConfigNonZeroUsize`], returning a configuration error if
+ /// `value` is zero.
+ pub fn try_new(value: usize) -> Result<Self> {
+ NonZeroUsize::new(value).map(Self).ok_or_else(|| {
+ DataFusionError::Configuration("value must be greater than 0".to_string())
+ })
+ }
+
+ /// Returns the wrapped `usize`.
+ pub const fn get(self) -> usize {
+ self.0.get()
+ }
+}
+
+impl From<ConfigNonZeroUsize> for usize {
+ fn from(value: ConfigNonZeroUsize) -> Self {
+ value.get()
+ }
+}
+
+impl FromStr for ConfigNonZeroUsize {
+ type Err = DataFusionError;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ Self::try_new(default_config_transform(s)?)
+ }
+}
+
+impl ConfigField for ConfigNonZeroUsize {
+ fn visit<V: Visit>(&self, v: &mut V, key: &str, description: &'static str) {
+ v.some(key, self, description)
+ }
+
+ fn set(&mut self, key: &str, value: &str) -> Result<()> {
+ if !key.is_empty() {
+ return _config_err!(
+ "Config field is a scalar ConfigNonZeroUsize and does not have nested field \"{}\"",
Review Comment:
would be nice to add config name here
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#discussion_r3447932516
##########
nonzero_test:
##########
Review Comment:
🤔
--
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] minor: Validate `batch_size` configuration when setting it [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #23054:
URL: https://github.com/apache/datafusion/pull/23054#issuecomment-4760621153
> Thank you for opening this pull request!
>
> Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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.
--
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