goldmedal commented on code in PR #13493:
URL: https://github.com/apache/datafusion/pull/13493#discussion_r1851732566
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -514,6 +516,57 @@ impl Unparser<'_> {
})
}
+ fn named_struct_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
+ if args.len() % 2 != 0 {
+ return internal_err!("named_struct must have an even number of
arguments");
+ }
+
+ let args = args
+ .chunks_exact(2)
+ .map(|chunk| {
+ let key = match &chunk[0] {
+ Expr::Literal(lit) =>
self.new_ident_quoted_if_needs(lit.to_string()),
Review Comment:
```suggestion
Expr::Literal(ScalarValue::Utf8(Some(s))) =>
self.new_ident_quoted_if_needs(s.to_string()),
```
Refer to the implementation of `named_struct`, we can match the string
literal only.
https://github.com/apache/datafusion/blob/9fb7aee95c5fcf177609963cedadf443ba6fe1b7/datafusion/functions/src/core/named_struct.rs#L49-L52
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -514,6 +516,57 @@ impl Unparser<'_> {
})
}
+ fn named_struct_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
+ if args.len() % 2 != 0 {
+ return internal_err!("named_struct must have an even number of
arguments");
+ }
+
+ let args = args
+ .chunks_exact(2)
+ .map(|chunk| {
+ let key = match &chunk[0] {
+ Expr::Literal(lit) =>
self.new_ident_quoted_if_needs(lit.to_string()),
+ _ => return internal_err!("named_struct expects even
arguments to be strings, but received: {:?}", &chunk[0])
+ };
+
+ Ok(ast::DictionaryField {
+ key,
+ value: Box::new(self.expr_to_sql(&chunk[1])?),
+ })
+ })
+ .collect::<Result<Vec<_>>>()?;
+
+ Ok(ast::Expr::Dictionary(args))
+ }
+
+ fn get_field_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
+ if args.len() != 2 {
+ return internal_err!("get_field must have exactly 2 arguments");
+ }
+
+ let mut id = match &args[0] {
+ Expr::Column(col) => match self.col_to_sql(col)? {
+ ast::Expr::Identifier(ident) => vec![ident],
+ ast::Expr::CompoundIdentifier(idents) => idents,
+ other => return internal_err!("expected col_to_sql to return
an Identifier or CompoundIdentifier, but received: {:?}", other),
+ },
+ _ => return internal_err!("get_field expects first argument to be
column, but received: {:?}", &args[0]),
+ };
Review Comment:
We should consider the more complex cases, a struct contains a struct field.
Consider the following SQL that uses another way to access the struct:
```
> explain SELECT s.a['a1'] FROM (SELECT {a:{a1:c1}, b:{a1:c1}} AS s from t1);
+---------------+-------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type | plan
|
+---------------+-------------------------------------------------------------------------------------------------------------------------------------------+
| logical_plan | Projection: get_field(get_field(named_struct(Utf8("a"),
__common_expr_1, Utf8("b"), __common_expr_1), Utf8("a")), Utf8("a1")) AS
s[a][a1] |
| | Projection: named_struct(Utf8("a1"), t1.c1) AS
__common_expr_1
|
| | TableScan: t1 projection=[c1]
```
You can see the `get_field` accepts another `get_field` function. However, I
think we can't do a roundtrip test for this kind of case currently because SQL
like `select s.a.a1` isn't supported
(https://github.com/apache/datafusion-sqlparser-rs/pull/1541 will address it).
By the way, I think we can file a follow-up issue for the nested case. We
don't need to support it in this PR.
--
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
