tinfoil-knight commented on code in PR #11330:
URL: https://github.com/apache/datafusion/pull/11330#discussion_r1683257362
##########
datafusion/sql/src/utils.rs:
##########
@@ -263,6 +263,37 @@ pub(crate) fn normalize_ident(id: Ident) -> String {
}
}
+pub(crate) fn normalize_value(value: &Value) -> Result<String> {
Review Comment:
Also, `exec_err` is only for Execution Errors. Code running in the planning
stage should ideally use `plan_err`.
##########
datafusion/sql/src/statement.rs:
##########
@@ -881,25 +857,8 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
}
};
- let mut options = HashMap::new();
- for (key, value) in statement.options {
- let value_string = match value_to_string(&value) {
- None => {
- return plan_err!("Unsupported Value in COPY statement {}",
value);
- }
- Some(v) => v,
- };
-
- if !(&key.contains('.')) {
- // If config does not belong to any namespace, assume it is
- // a format option and apply the format prefix for backwards
- // compatibility.
- let renamed_key = format!("format.{}", key);
- options.insert(renamed_key.to_lowercase(),
value_string.to_lowercase());
- } else {
- options.insert(key.to_lowercase(),
value_string.to_lowercase());
- }
- }
+ let options_map: HashMap<String, String> =
+ self.parse_options_map(statement.options, true)?;
Review Comment:
```suggestion
let options_map = self.parse_options_map(options, true)?;
```
##########
datafusion/sql/src/planner.rs:
##########
@@ -86,6 +88,35 @@ impl IdentNormalizer {
}
}
+/// Value Normalizer
+#[derive(Debug)]
+pub struct ValueNormalizer {
+ normalize: bool,
+}
+
+impl Default for ValueNormalizer {
+ fn default() -> Self {
+ Self { normalize: true }
+ }
+}
+
+impl ValueNormalizer {
+ pub fn new(normalize: bool) -> Self {
+ Self { normalize }
+ }
+
+ pub fn normalize(&self, value: Value) -> Result<String> {
+ if self.normalize {
+ crate::utils::normalize_value(&value)
+ } else {
+ match crate::utils::value_to_string(&value) {
+ Some(s) => Ok(s),
+ None => internal_err!("Unsupport value type to string: {:?}",
value),
+ }
+ }
+ }
+}
Review Comment:
```suggestion
pub fn normalize(&self, value: String) -> String {
if self.normalize {
value.to_ascii_lowercase()
} else {
value
}
}
}
```
I don't think the normalize function should return an error. How about we do
the string conversion and normalization in separate steps in the
`parse_options_map` method.
Something like:
```rs
let value_string = match value_to_string(&value) {
Some(s) => self.value_normalizer.normalize(s),
None => return plan_err!("Unsupported Value in Options : {}", value),
};
```
##########
datafusion/sql/src/utils.rs:
##########
@@ -263,6 +263,37 @@ pub(crate) fn normalize_ident(id: Ident) -> String {
}
}
+pub(crate) fn normalize_value(value: &Value) -> Result<String> {
Review Comment:
`utils` is generally for things that are used across multiple files
repeatedly.
How about we remove this function and just do the normalization in the
`normalize` method under the `ValueNormalizer` struct?
If we separate the string conversion and normalization (as suggested in
another comment), then we can move back the `value_to_string` fn too.
--
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...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
