tinfoil-knight commented on code in PR #11330:
URL: https://github.com/apache/datafusion/pull/11330#discussion_r1680823703
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -149,6 +153,68 @@ fn parse_ident_normalization() {
}
}
+#[test]
+fn test_parse_options_value_normalization() {
+ let test_data = [
+ (
+ "CREATE EXTERNAL TABLE test OPTIONS ('location' 'LoCaTiOn') STORED
AS PARQUET LOCATION 'fake_location'",
+ "CreateExternalTable: Bare { table: \"test\" }",
+ HashMap::from([("format.location", "LoCaTiOn")]),
+ false,
+ ),
+ (
+ "CREATE EXTERNAL TABLE test OPTIONS ('location' 'LoCaTiOn') STORED
AS PARQUET LOCATION 'fake_location'",
+ "CreateExternalTable: Bare { table: \"test\" }",
+ HashMap::from([("format.location", "location")]),
+ true,
+ ),
+ (
+ "COPY test TO 'fake_location' STORED AS PARQUET OPTIONS
('location' 'LoCaTiOn')",
+ "CopyTo: format=csv output_url=fake_location options:
(format.location LoCaTiOn)\n TableScan: test",
+ HashMap::from([("format.location", "LoCaTiOn")]),
+ false,
+ ),
+ (
+ "COPY test TO 'fake_location' STORED AS PARQUET OPTIONS
('location' 'LoCaTiOn')",
+ "CopyTo: format=csv output_url=fake_location options:
(format.location location)\n TableScan: test",
+ HashMap::from([("format.location", "location")]),
+ true,
+ ),
+ ];
+
+ for (sql, expected_plan, expected_options,
enable_options_value_normalization) in
+ test_data
+ {
+ let plan = logical_plan_with_options(
+ sql,
+ ParserOptions {
+ parse_float_as_decimal: false,
+ enable_ident_normalization: false,
+ support_varchar_with_length: false,
+ enable_options_value_normalization,
+ },
+ );
+ if plan.is_ok() {
+ let plan = plan.unwrap();
Review Comment:
```suggestion
if let Ok(plan) = plan {
```
##########
datafusion/sql/src/utils.rs:
##########
@@ -263,6 +263,34 @@ pub(crate) fn normalize_ident(id: Ident) -> String {
}
}
+pub(crate) fn normalize_value(value: &Value) -> Result<String> {
+ value_to_string(value).map(|v| v.to_ascii_lowercase())
+}
+
+pub(crate) fn value_to_string(value: &Value) -> Result<String> {
+ match value {
+ Value::SingleQuotedString(s) => Ok(s.to_string()),
+ Value::DollarQuotedString(s) => Ok(s.to_string()),
+ Value::Number(_, _) | Value::Boolean(_) => Ok(value.to_string()),
+ Value::DoubleQuotedString(_)
+ | Value::EscapedStringLiteral(_)
+ | Value::NationalStringLiteral(_)
+ | Value::SingleQuotedByteStringLiteral(_)
+ | Value::DoubleQuotedByteStringLiteral(_)
+ | Value::TripleSingleQuotedString(_)
+ | Value::TripleDoubleQuotedString(_)
+ | Value::TripleSingleQuotedByteStringLiteral(_)
+ | Value::TripleDoubleQuotedByteStringLiteral(_)
+ | Value::SingleQuotedRawStringLiteral(_)
+ | Value::DoubleQuotedRawStringLiteral(_)
+ | Value::TripleSingleQuotedRawStringLiteral(_)
+ | Value::TripleDoubleQuotedRawStringLiteral(_)
+ | Value::HexStringLiteral(_)
+ | Value::Null
+ | Value::Placeholder(_) => plan_err!("Unsupported Value to normalize
{}", value),
Review Comment:
The error doesn't make sense in the context of this function. This function
isn't supposed to be changing the string to lowercase.
##########
datafusion/sql/src/statement.rs:
##########
@@ -1187,11 +1151,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
// parse value string from Expr
let value_string = match &value[0] {
SQLExpr::Identifier(i) => ident_to_string(i),
- SQLExpr::Value(v) => match value_to_string(v) {
- None => {
+ SQLExpr::Value(v) => match crate::utils::value_to_string(v) {
+ Err(_) => {
return plan_err!("Unsupported Value {}", value[0]);
}
Review Comment:
```suggestion
Err(_) => return plan_err!("Unsupported Value {}", value[0]),
```
##########
datafusion/sql/src/statement.rs:
##########
@@ -1080,6 +1017,33 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)))
}
+ fn parse_options_map(
+ &self,
+ options: Vec<(String, Value)>,
+ allow_duplicates: bool,
Review Comment:
@alamb Is there any reason why we were historically allowing duplicates for
option values in the CopyTo statement but not the CreateTable statement?
IMO we should dis-allow duplicates for both statements.
##########
datafusion/sql/src/utils.rs:
##########
@@ -263,6 +263,34 @@ pub(crate) fn normalize_ident(id: Ident) -> String {
}
}
+pub(crate) fn normalize_value(value: &Value) -> Result<String> {
+ value_to_string(value).map(|v| v.to_ascii_lowercase())
+}
+
+pub(crate) fn value_to_string(value: &Value) -> Result<String> {
+ match value {
+ Value::SingleQuotedString(s) => Ok(s.to_string()),
+ Value::DollarQuotedString(s) => Ok(s.to_string()),
+ Value::Number(_, _) | Value::Boolean(_) => Ok(value.to_string()),
+ Value::DoubleQuotedString(_)
+ | Value::EscapedStringLiteral(_)
+ | Value::NationalStringLiteral(_)
+ | Value::SingleQuotedByteStringLiteral(_)
+ | Value::DoubleQuotedByteStringLiteral(_)
+ | Value::TripleSingleQuotedString(_)
+ | Value::TripleDoubleQuotedString(_)
+ | Value::TripleSingleQuotedByteStringLiteral(_)
+ | Value::TripleDoubleQuotedByteStringLiteral(_)
+ | Value::SingleQuotedRawStringLiteral(_)
+ | Value::DoubleQuotedRawStringLiteral(_)
+ | Value::TripleSingleQuotedRawStringLiteral(_)
+ | Value::TripleDoubleQuotedRawStringLiteral(_)
+ | Value::HexStringLiteral(_)
+ | Value::Null
+ | Value::Placeholder(_) => plan_err!("Unsupported Value to normalize
{}", value),
Review Comment:
This function is also used in the match case for `SQLExpr::Value(v)` and
even though we're returning `"Unsupported Value {}"` error there to users, this
error message might cause confusion for some contributor stepping through the
code.
--
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]
