alamb commented on code in PR #2812:
URL: https://github.com/apache/arrow-datafusion/pull/2812#discussion_r910502407
##########
datafusion/core/src/config.rs:
##########
@@ -179,7 +189,18 @@ impl ConfigOptions {
let mut options = HashMap::new();
let built_in = BuiltInConfigs::new();
for config_def in &built_in.config_definitions {
- options.insert(config_def.key.clone(),
config_def.default_value.clone());
+ let config_value = {
+ let mut env_key = config_def.key.replace('.', "_");
+ env_key.make_ascii_uppercase();
+ match env::var(env_key) {
+ Ok(value) => match scalar_from_string(value,
&config_def.data_type) {
+ Ok(value) => value,
+ Err(_) => config_def.default_value.clone(),
Review Comment:
I suggest telling the user about this via something like `warn!` or
`eprintln!("WARNING: ignoring unparsable value...."`?
##########
datafusion/core/src/config.rs:
##########
@@ -260,4 +281,24 @@ mod test {
assert!(config.get(invalid_key).is_none());
assert!(!config.get_bool(invalid_key));
}
+
+ #[test]
+ fn get_from_env() {
Review Comment:
I agree changing the environment from this test is likely to be problematic
as it would cause non deterministic behavior
I suggest creating a specific rust "integration test" (aka a file in
https://github.com/apache/arrow-datafusion/tree/master/datafusion/core/tests)
that tests the environment parsing
One alternate I can think of is to use a subprocess / fork so that the
environment of the calling process isn't modified. I think this may be a bit
unsafe for various reasons, so I suggest the dedicated test instead
##########
datafusion/core/src/config.rs:
##########
@@ -161,6 +163,14 @@ impl BuiltInConfigs {
}
}
+fn scalar_from_string(value: String, target_type: &DataType) ->
Result<ScalarValue> {
+ let value = ScalarValue::Utf8(Some(value));
+ let cast_options = cast::CastOptions { safe: false };
Review Comment:
This is clever -- it may be worth putting directly in `ScalarValue` perhaps,
perhaps something like
```rust
impl ScalarValue {
// ..
fn try_new_string(value: String, target_type: &DataType) -> Result<Self> {
...
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
