adriangb commented on code in PR #25661:
URL: https://github.com/apache/datafusion/pull/25661#discussion_r4086135754
##########
datafusion/sql/src/unparser/dialect.rs:
##########
@@ -164,6 +164,16 @@ pub trait Dialect: Send + Sync {
BinaryOperator::Divide
}
+ /// Whether the dialect's parser accepts DuckDB-style dictionary syntax
+ /// (`{'a': 1, 'b': 2}`) for struct literals.
+ ///
+ /// When false, `named_struct` is unparsed as a `named_struct(...)`
+ /// function call instead of a dictionary literal, so the emitted SQL can
+ /// be parsed back under the same dialect.
+ fn supports_dictionary_syntax(&self) -> bool {
Review Comment:
The goal of this PR is that DataFusion can parse the unparsed SQL again with
the same dialect. Thus the question "does the unparser emit `{...}`?" is the
same as the question "does the `sqlparser` dialect accept `{...}`?".
`sqlparser` already has a method with this name:
`sqlparser::dialect::Dialect::supports_dictionary_syntax`.
The unparser `Dialect` trait does not have a link to a `sqlparser` dialect,
so the default method cannot call it. But each built-in unparser dialect can
call its matching `sqlparser` dialect. For example:
```rust
impl Dialect for PostgreSqlDialect {
fn supports_dictionary_syntax(&self) -> bool {
sqlparser::dialect::PostgreSqlDialect {}.supports_dictionary_syntax()
}
}
```
Do the same for `MySqlDialect`, `SqliteDialect`, `BigQueryDialect`,
`DuckDBDialect`, `SnowflakeDialect` and `DefaultDialect` (with
`GenericDialect`).
Why I recommend this:
- The output does not change. In `sqlparser` 0.63, Postgres, MySQL, SQLite
and BigQuery return `false`. DuckDB, Snowflake and Generic return `true`. This
is the same as the values in this PR.
- There is only one source of truth. If a new `sqlparser` version adds
`{...}` support to a parser dialect, the unparser follows it. You do not need
to change the code.
- The doc comment on this trait says that the trait "will eventually be
replaced by the Dialect in the SQLparser package". Delegation is a step in that
direction.
`CustomDialect` does not have a `sqlparser` dialect, so it still needs the
flag and `with_supports_dictionary_syntax`. For `CustomDialect`, please add one
sentence to the doc comment that refers to
`sqlparser::dialect::Dialect::supports_dictionary_syntax`. Then a user knows
which value to use.
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -740,24 +740,34 @@ impl Unparser<'_> {
"named_struct must have an even number of arguments"
);
- let args = args
+ let fields = args
.as_chunks::<2>()
.0
.iter()
- .map(|[name, value]| {
- let key = match name {
- Expr::Literal(ScalarValue::Utf8(Some(s)), _) =>
self.new_ident_quoted_if_needs(s.to_string()),
- _ => return internal_err!("named_struct expects even
arguments to be strings, but received: {name:?}")
- };
+ .map(|[name, value]| match name {
+ Expr::Literal(ScalarValue::Utf8(Some(name)), _) => Ok((name,
value)),
+ _ => internal_err!(
+ "named_struct expects even arguments to be strings, but
received: {name:?}"
+ ),
+ })
+ .collect::<Result<Vec<_>>>()?;
+
+ // dialects whose parsers reject dictionary syntax get a function call
+ if !self.dialect.supports_dictionary_syntax() {
Review Comment:
Question: why a new trait method and not an override in
`scalar_function_to_sql_overrides` for each dialect?
I think the flag is the better choice, because `CustomDialectBuilder` users
can set it with one call. I only ask so that the reason is on record.
Nit: in the function-call path, the code collects `fields` into a `Vec` only
for validation, and then does not use it. The cost is small. If you want, a
loop that only validates can prevent the allocation:
```rust
for [name, _] in args.as_chunks::<2>().0 {
if !matches!(name, Expr::Literal(ScalarValue::Utf8(Some(_)), _)) {
return internal_err!("named_struct expects even arguments to be
strings, but received: {name:?}");
}
}
```
This is optional. The current code is also clear.
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -394,6 +397,75 @@ fn roundtrip_statement_with_dialect_3() -> Result<(),
DataFusionError> {
Ok(())
}
+#[test]
+fn roundtrip_statement_named_struct_for_dialects() -> Result<(),
DataFusionError> {
+ type DialectCase = (
+ &'static str,
+ Box<dyn Dialect>,
+ Box<dyn UnparserDialect>,
+ char,
+ );
+
+ let dialects: [DialectCase; 4] = [
+ (
+ "postgres",
+ Box::new(PostgreSqlDialect {}),
+ Box::new(UnparserPostgreSqlDialect {}),
+ '"',
+ ),
+ (
+ "mysql",
+ Box::new(MySqlDialect {}),
+ Box::new(UnparserMySqlDialect {}),
+ '`',
+ ),
+ (
+ "sqlite",
+ Box::new(ParserSqliteDialect {}),
+ Box::new(SqliteDialect {}),
+ '`',
+ ),
+ (
+ "bigquery",
+ Box::new(ParserBigQueryDialect),
+ Box::new(BigQueryDialect {}),
+ '`',
+ ),
+ ];
+ let sql = "select named_struct('a', j1_id, 'b', j1_string) from j1";
+
+ for (dialect_name, parser_dialect, unparser_dialect, quote) in dialects {
+ let state = MockSessionState::default()
+ .with_scalar_function(datafusion_functions::core::named_struct())
+ .with_expr_planner(Arc::new(CoreFunctionPlanner::default()));
+ let context = MockContextProvider { state };
+ let sql_to_rel = SqlToRel::new(&context);
+
+ let statement = Parser::new(parser_dialect.as_ref())
+ .try_with_sql(sql)?
+ .parse_statement()?;
+ let plan = sql_to_rel.sql_statement_to_plan(statement)?;
+
+ let unparsed = Unparser::new(unparser_dialect.as_ref())
+ .plan_to_sql(&plan)?
+ .to_string();
+ let qualified_id = format!(
Review Comment:
Nit: `qualified_id` contains more than one identifier. It also contains
`'b'` and the second column. A name such as `args` or `expected_args` is more
accurate. Or build the full expected string in one `format!`.
--
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]
