findepi commented on code in PR #13576:
URL: https://github.com/apache/datafusion/pull/13576#discussion_r1879689825
##########
datafusion/common/src/config.rs:
##########
@@ -212,11 +219,12 @@ config_namespace! {
pub enable_ident_normalization: bool, default = true
/// When set to true, SQL parser will normalize options value (convert
value to lowercase)
- pub enable_options_value_normalization: bool, default = true
+ pub enable_options_value_normalization: bool, default = false
Review Comment:
This is the important change in this PR, along with the definition of
`transform` for option values.
cc @alamb @comphead @jayzhan211
##########
datafusion/common/src/config.rs:
##########
@@ -444,7 +452,7 @@ config_namespace! {
/// Valid values are: "none", "chunk", and "page"
/// These values are not case sensitive. If NULL, uses
Review Comment:
Now the `These values are not case sensitive` can be removed
##########
datafusion/common/src/config.rs:
##########
@@ -212,11 +219,12 @@ config_namespace! {
pub enable_ident_normalization: bool, default = true
/// When set to true, SQL parser will normalize options value (convert
value to lowercase)
- pub enable_options_value_normalization: bool, default = true
+ pub enable_options_value_normalization: bool, default = false
/// Configure the SQL dialect used by DataFusion's parser; supported
values include: Generic,
/// MySQL, PostgreSQL, Hive, SQLite, Snowflake, Redshift, MsSQL,
ClickHouse, BigQuery, and Ansi.
- pub dialect: String, default = "generic".to_string()
+ pub dialect: String, default = "generic".to_string() // no need to
lowercase because
+ // [`sqlparser::dialect_from_str`] is case-insensitive
Review Comment:
Comment formatting is off after it was broken across the lines
Also, the ``[`...`]`` links probably don't work in ordinary comments (but
didn't check that)
```suggestion
// no need to lowercase because `sqlparser::dialect_from_str` is
case-insensitive
pub dialect: String, default = "generic".to_string()
```
##########
datafusion/core/tests/config_from_env.rs:
##########
@@ -22,10 +22,19 @@ use std::env;
fn from_env() {
// Note: these must be a single test to avoid interference from concurrent
execution
let env_key = "DATAFUSION_OPTIMIZER_FILTER_NULL_JOIN_KEYS";
- env::set_var(env_key, "true");
- let config = ConfigOptions::from_env().unwrap();
+ // valid testing in different cases
+ for bool_option in ["true", "TRUE", "True"] {
Review Comment:
```suggestion
for bool_option in ["true", "TRUE", "True", "tRUe"] {
```
##########
datafusion/common/src/config.rs:
##########
@@ -971,29 +979,46 @@ impl<F: ConfigField + Default> ConfigField for Option<F> {
}
}
+fn parse<T>(input: &str) -> Result<T>
+where
+ T: FromStr,
+ T::Err: Display,
Review Comment:
unused
```suggestion
```
##########
datafusion/common/src/config.rs:
##########
@@ -971,29 +979,46 @@ impl<F: ConfigField + Default> ConfigField for Option<F> {
}
}
+fn parse<T>(input: &str) -> Result<T>
Review Comment:
- let's add `default` to the name to indicate how it's used
- let's decide whether we call the thing "parse" or "transform" and use the
naming consistently
```suggestion
fn default_transform<T>(input: &str) -> Result<T>
```
##########
datafusion/common/src/config.rs:
##########
@@ -470,7 +478,7 @@ config_namespace! {
/// delta_byte_array, rle_dictionary, and byte_stream_split.
/// These values are not case sensitive. If NULL, uses
Review Comment:
> These values are not case sensitive.
this suggests the code reading this value does normalization and that it's
no longer needed, can be simplified
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
