alamb commented on code in PR #3522:
URL: https://github.com/apache/arrow-datafusion/pull/3522#discussion_r974677014
##########
datafusion/core/src/config.rs:
##########
@@ -288,26 +288,37 @@ impl ConfigOptions {
}
/// get a boolean configuration option
- pub fn get_bool(&self, key: &str) -> bool {
+ pub fn get_bool(&self, key: &str) -> Option<bool> {
match self.get(key) {
- Some(ScalarValue::Boolean(Some(b))) => b,
- _ => false,
+ Some(ScalarValue::Boolean(b)) => b,
+ Some(b) => Some(
+ b.to_string()
+ .parse::<bool>()
+ .unwrap_or_else(|_| panic!("Cannot parse bool from {:?}",
&b)),
+ ),
+ None => None,
}
}
/// get a u64 configuration option
- pub fn get_u64(&self, key: &str) -> u64 {
+ pub fn get_u64(&self, key: &str) -> Option<u64> {
match self.get(key) {
- Some(ScalarValue::UInt64(Some(n))) => n,
- _ => 0,
+ Some(ScalarValue::UInt64(n)) => n,
+ Some(n) => Some(
Review Comment:
calling `to_string()` on a ScalarValue calls its `Display` implementation:
https://github.com/apache/arrow-datafusion/blob/master/datafusion/common/src/scalar.rs#L1991
I am not sure if trying to parse this as a `u64` is what someone may expect.
For example, if it is `ScalarValue::Utf8(None)` the value returned will be
`"None"` which will fail to parse and panic.
I would recommend only trying to parse `ScalarValue::Utf8(n))` rather than
any `ScalarValue`
##########
datafusion/core/src/config.rs:
##########
@@ -288,26 +288,37 @@ impl ConfigOptions {
}
/// get a boolean configuration option
- pub fn get_bool(&self, key: &str) -> bool {
+ pub fn get_bool(&self, key: &str) -> Option<bool> {
match self.get(key) {
- Some(ScalarValue::Boolean(Some(b))) => b,
- _ => false,
+ Some(ScalarValue::Boolean(b)) => b,
+ Some(b) => Some(
+ b.to_string()
+ .parse::<bool>()
+ .unwrap_or_else(|_| panic!("Cannot parse bool from {:?}",
&b)),
Review Comment:
🤔 I feel like in other PRs like
https://github.com/apache/arrow-datafusion/issues/3316 are going in the
opposite direction and removing the use of panic!
I would prefer this API returned None rather than panic'd (and maybe log a
`warn!` message)
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
