[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2812: Allow setting of config options via environment variables

[GitBox <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org