Uh oh!
There was an error while loading. Please reload this page.
Unparse struct to sql - #13493
Conversation
jonathanc-n
left a comment
There was a problem hiding this comment.
This look good to me! Just left some minor error changes.
| ast::Expr::CompoundIdentifier(idents) => idents, | ||
| _ => unreachable!(), | ||
| }, | ||
| _ => return internal_err!("get_field column is invalid"), |
There was a problem hiding this comment.
| _ => returninternal_err!("get_field column is invalid"), | |
| _ => returninternal_err!("get_field expects first argument to be column, but received {:?}", args[0]), |
Error message should be more clear here for feedback
| Expr::Column(col) => match self.col_to_sql(col)? { | ||
| ast::Expr::Identifier(ident) => vec![ident], | ||
| ast::Expr::CompoundIdentifier(idents) => idents, | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
| _ => unreachable!(), | |
| _ => returninternal_err!("get_field expects an Identifier or CompoundIdentifier, but received: {:?}", other) |
I feel like unreachable should be avoided
velaskuez
commented
Nov 20, 2024
Thanks @jonathanc-n, I've updated the errors |
goldmedal
left a comment
There was a problem hiding this comment.
Thanks @delamarch3 overall looks good to me 👍 Only some suggestions.
Uh oh!
There was an error while loading. Please reload this page.
| 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]), | ||
| }; |
There was a problem hiding this 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 (apache/datafusion-sqlparser-rs#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.
Co-authored-by: Jax Liu <liugs963@gmail.com>
velaskuez
commented
Nov 21, 2024
Thanks for the review @goldmedal |
alamb
commented
Nov 21, 2024
🚀 |
Which issue does this PR close?
Closes#13477
Rationale for this change
Support roundtrip for statements containing
named_structandget_fieldWhat changes are included in this PR?
Implement expr_to_sql for
named_structandget_fieldAre these changes tested?
Yes
Are there any user-facing changes?
No