Add parser option enable_options_value_normalization

[xinlifoobar]

Which issue does this PR close?

Closes #10853

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[berkaysynnada]

Thanks @xinlifoobar working on this issue.

What I suggest is that we can let people to define their own normalizations. Rather than directing all values to lowercase, people can declare their own normalization methods for specific option keys. If we choose to do that in this PR, I believe the normalization process should be done during the assignment of table options.

[xinlifoobar]

Thanks @xinlifoobar working on this issue.

What I suggest is that we can let people to define their own normalizations. Rather than directing all values to lowercase, people can declare their own normalization methods for specific option keys. If we choose to do that in this PR, I believe the normalization process should be done during the assignment of table options.

The APIs that could be used in the datafusion-sql crate are limited. Do you think it is a good idea to add a method, e.g., get_options_normalized_function in ContextProvider?

datafusion/datafusion/expr/src/planner.rs

Lines 33 to 83 in 2f02c43

	pub trait ContextProvider {
	/// Getter for a datasource
	fn get_table_source(&self, name: TableReference) -> Result<Arc<dyn TableSource>>;
	fn get_file_type(&self, _ext: &str) -> Result<Arc<dyn FileType>> {
	not_impl_err!("Registered file types are not supported")
	}
	/// Getter for a table function
	fn get_table_function_source(
	&self,
	_name: &str,
	_args: Vec<Expr>,
	) -> Result<Arc<dyn TableSource>> {
	not_impl_err!("Table Functions are not supported")
	}
	/// This provides a worktable (an intermediate table that is used to store the results of a CTE during execution)
	/// We don't directly implement this in the logical plan's ['SqlToRel`]
	/// because the sql code needs access to a table that contains execution-related types that can't be a direct dependency
	/// of the sql crate (namely, the `CteWorktable`).
	/// The [`ContextProvider`] provides a way to "hide" this dependency.
	fn create_cte_work_table(
	&self,
	_name: &str,
	_schema: SchemaRef,
	) -> Result<Arc<dyn TableSource>> {
	not_impl_err!("Recursive CTE is not implemented")
	}
	/// Getter for a UDF description
	fn get_function_meta(&self, name: &str) -> Option<Arc<ScalarUDF>>;
	/// Getter for a UDAF description
	fn get_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>>;
	/// Getter for a UDWF
	fn get_window_meta(&self, name: &str) -> Option<Arc<WindowUDF>>;
	/// Getter for system/user-defined variable type
	fn get_variable_type(&self, variable_names: &[String]) -> Option<DataType>;
	/// Get configuration options
	fn options(&self) -> &ConfigOptions;
	/// Get all user defined scalar function names
	fn udf_names(&self) -> Vec<String>;
	/// Get all user defined aggregate function names
	fn udaf_names(&self) -> Vec<String>;
	/// Get all user defined window function names
	fn udwf_names(&self) -> Vec<String>;
	}

[berkaysynnada]

Thanks @xinlifoobar working on this issue.
What I suggest is that we can let people to define their own normalizations. Rather than directing all values to lowercase, people can declare their own normalization methods for specific option keys. If we choose to do that in this PR, I believe the normalization process should be done during the assignment of table options.

The APIs that could be used in the datafusion-sql crate are limited. Do you think it is a good idea to add a method, e.g., get_options_normalized_function a good idea?

SqlParserOptions is accessible, what do you think about adding it there? But I'm not sure if we can support dynamic method registration. My idea is to provide hard-coded functions like the one below, but I'm uncertain where to bind it:

lazy_static! {
    static ref NORMALIZATION_MAP: Mutex<HashMap<String, fn(String) -> String>> = {
        let mut m = HashMap::new();
        m.insert("hf.user_access_token".to_string(), lowercase as fn(String) -> String);
        // Other normalizations
        Mutex::new(m)
    };
}

fn lowercase(s: String) -> String {
    s.to_lowercase()
}
// Other normalizations

If the key is not found, use the default normalization or do nothing (according to your enable_options_value_normalization).

[xinlifoobar]

But

IMO it won't be a good idea... I thought the *configs are static values. What should you do if you would keep a config file for function values?

[alamb]

What I suggest is that we can let people to define their own normalizations. Rather than directing all values to lowercase, people can declare their own normalization methods for specific option keys. If we choose to do that in this PR, I believe the normalization process should be done during the assignment of table options.

I wonder what is the usecase for a user defined normalization? It may be more complicated to code

I like the approach proposed in this PR. As I understand it if follows what we do for SQL: by default apply some hard coded normalization (e.g. to lower_case) and let users opt out of that normalization either by:

1. Setting a config value like datafusion.sql_parser.enable_ident_normalization
2. Enclose the value in "

[berkaysynnada]

Hi again @xinlifoobar. The idea in my mind was something like this: synnada-ai@341b484.

I haven't been able to work on it much, and it can't be compiled right now, but you can understand what I mean as an idea. By removing the lowercase transformations we made while parsing the statement, just like in this PR, it seems that we can easily normalize the fields we want in the desired way.

cc @alamb

[xinlifoobar]

Hi again @xinlifoobar. The idea in my mind was something like this: synnada-ai@341b484.

I haven't been able to work on it much, and it can't be compiled right now, but you can understand what I mean as an idea. By removing the lowercase transformations we made while parsing the statement, just like in this PR, it seems that we can easily normalize the fields we want in the desired way.

cc @alamb

Hi, I got the idea and am okay to make the change if we agree. The question is still the normalize go beyond the config itself. I am still unsure about this design because the normalize function is just for a small set of configs. in the future, if we have multiple functions for different purposes, the macro might become huge and difficult to read.

[xinlifoobar]

Hi again @xinlifoobar. The idea in my mind was something like this: synnada-ai@341b484.
I haven't been able to work on it much, and it can't be compiled right now, but you can understand what I mean as an idea. By removing the lowercase transformations we made while parsing the statement, just like in this PR, it seems that we can easily normalize the fields we want in the desired way.
cc @alamb

Hi, I got the idea and am okay to make the change if we agree. The question is still the normalize go beyond the config itself. I am still unsure about this design because the normalize function is just for a small set of configs. in the future, if we have multiple functions for different purposes, the macro might become huge and difficult to read.

Hey @berkaysynnada @alamb are we all ok with this design? I plan to update this PR tomorrow.

[alamb]

Hi @xinlifoobar -- I haven't had a chance to review this PR unfortunately. Perhaps @tinfoil-knight has some time to review the design as the original filer of #10853

[tinfoil-knight]

PR looks good overall. Some minor changes requested. I prefer the current implementation that changes the normalization for all option values.

@berkaysynnada 's solution allows option-specific normalization but I don't think that kind of fine-grained control over option values is needed.

---

Sidenote:
We could add traits later on that'd allow users specify custom normalization behavior for identifiers and option values through SqlToRel::new_with_options.

// Example
pub trait Normalizer {
	fn normalize(&self, key: &str, value: Value) -> Result<String>;
}

[tinfoil-knight]

@alamb The current design and PR looks good to me. Can we get this merged?

[alamb]

I agree -- this looks great to me. Than you @xinlifoobar and @tinfoil-knight

I'll plan to merge this tomorrow unless there are other comments.

cc @berkaysynnada

[ozankabak]

@berkaysynnada 's solution allows option-specific normalization but I don't think that kind of fine-grained control over option values is needed.

We actually do need this in our use case. Let's do the general solution to avoid API churn on such a user-visible part of DataFusion.

@xinlifoobar and @tinfoil-knight, do you have time to apply @berkaysynnada's suggestion? It's OK if not, we can work on it instead.

cc @berkaysynnada

[xinlifoobar]

@berkaysynnada 's solution allows option-specific normalization but I don't think that kind of fine-grained control over option values is needed.

We actually do need this in our use case. Let's do the general solution to avoid API churn on such a user-visible part of DataFusion.

@xinlifoobar and @tinfoil-knight, do you have time to apply @berkaysynnada's suggestion? It's OK if not, we can work on it instead.

cc @berkaysynnada

Hey, since this is pending long. I could do this as a follow up after this is merged.

[ozankabak]

Sure -- let's just make sure to avoid API churn and complete the work 🙂

[alamb]

Sure -- let's just make sure to avoid API churn and complete the work 🙂

How about we file a ticket explaining what else is needed prior to merging this PR. I can do this but I want to make sure I have the use case down correctly

@berkaysynnada says:

What I suggest is that we can let people to define their own normalizations. Rather than directing all values to lowercase, people can declare their own normalization methods for specific option keys. If we choose to do that in this PR, I believe the normalization process should be done during the assignment of table options.

The code in synnada-ai@341b484 I think seems to show a way to add an annotation to a built in config setting value that controls how it will be normalized.

Is the use case to

1. Provide a new set of ConfigOptions (not in DataFusion) that have custom normalization rules?
2. Avoid normalization on some of the built in configs?

Something else?

[ozankabak]

Both. We would like to have key-level control on what normalization means for config options, built-in and extension.

[alamb]

Both. We would like to have key-level control on what normalization means for config options, built-in and extension.

In the interest of keeping the code moving, I filed #11650 to track the additional feature and I merged up from main to resolve a conflict

[ozankabak]

Sounds good -- this is good to go from my perspective.

[alamb]

Clippy failure seems unrelated to this PR. I think it is #11651

[alamb]

Merged up to get the clippy fix

[alamb]

🚀 thanks and sorry for the delay @xinlifoobar