feat: support multi value columns and aliases in unpivot#1969
feat: support multi value columns and aliases in unpivot#1969iffyio merged 5 commits intoapache:mainfrom
Conversation
chenkovsky
changed the title
| Unpivot { | ||
| table: Box<TableFactor>, | ||
| value: Ident, | ||
| value: Vec<Ident>, |
There was a problem hiding this comment.
should we use expr::Identifier ? otherwise we cannot visit this ident by vistor.
There was a problem hiding this comment.
Hmm yeah I think repr wise we could represent this as an arbitrary Expr, it would cover both the single_value and multi_value scenarios. Then similarly, we change to columns: Vec<ExprWithAlias>, which would additionally cover the fully qualified column_name variant
| "SELECT * FROM sales AS s ", | ||
| "UNPIVOT INCLUDE NULLS (quantity FOR quarter IN (Q1 AS Quater1, Q2 AS Quater2, Q3 AS Quater3, Q4 AS Quater4)) AS u (product, quarter, quantity)" | ||
| ); |
There was a problem hiding this comment.
it look like the formatting is a bit off here with cargo fmt, could you manually unindent these lines?
| sql_unpivot_with_alias | ||
| ); | ||
|
|
||
| let sql_unpivot_with_alias = concat!( |
There was a problem hiding this comment.
this has the same name as the preceeding sql_unpivot_with_alias scenario, but testing different aspects, do we need to rename this accordingly?
| Unpivot { | ||
| table: Box<TableFactor>, | ||
| value: Ident, | ||
| value: Vec<Ident>, |
There was a problem hiding this comment.
Hmm yeah I think repr wise we could represent this as an arbitrary Expr, it would cover both the single_value and multi_value scenarios. Then similarly, we change to columns: Vec<ExprWithAlias>, which would additionally cover the fully qualified column_name variant
| @@ -1351,9 +1392,9 @@ pub enum TableFactor { | |||
| /// See <https://docs.snowflake.com/en/sql-reference/constructs/unpivot>. | |||
There was a problem hiding this comment.
Can we include the link to the databricks documentation here as well?
| let value = match self.peek_token_ref().token { | ||
| Token::LParen => { | ||
| // multi value column unpivot | ||
| Expr::Tuple( | ||
| self.parse_parenthesized_column_list(Mandatory, false)? | ||
| .into_iter() | ||
| .map(Expr::Identifier) | ||
| .collect(), | ||
| ) | ||
| } | ||
| _ => { | ||
| // single value column unpivot | ||
| Expr::Identifier(self.parse_identifier()?) | ||
| } |
There was a problem hiding this comment.
Can we call self.parse_expr() and self.parse_expr_with_alias() transparently instead?
There was a problem hiding this comment.
parse_expr may accept some invalid sql.
There was a problem hiding this comment.
Ah yes I think it would be ok for the parser to be permissive in this case, downstream crates are expected to validate the AST further if needed.
Related can we add a test case that uses a fully qualified column name? e.g. FOR half_of_the_year IN ((foo.bar, bar.baz) AS x) - if I understand the intent correctly I think the parser is supposed to successfully parse that whereas with the current impl it is unable to
iffyio
left a comment
iffyio
left a comment
There was a problem hiding this comment.
Thanks @chenkovsky! Left a minor comment otherwise the changes look good to me
| self.parse_parenthesized_column_list_inner(optional, allow_empty, |p| { | ||
| p.parse_expr_with_alias() | ||
| }) |
There was a problem hiding this comment.
Can we inline this function since its only a couple lines and used once?
iffyio
changed the title
iffyio
left a comment
iffyio
left a comment
There was a problem hiding this comment.
LGTM! Thanks @chenkovsky!
cc @alamb
2 participants
for example, spark supports
https://spark.apache.org/docs/latest/sql-ref-syntax-qry-select-unpivot.html