tinfoil-knight commented on code in PR #11330:
URL: https://github.com/apache/datafusion/pull/11330#discussion_r1683244040
##########
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 since it is
ideally supposed to be taking in a string and just changing it to lowercase.
How about we do the case 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),
};
```
--
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
