-
Notifications
You must be signed in to change notification settings - Fork 663
feat: support multiple value for pivot #1970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/ast/query.rs
Outdated
table: Box<TableFactor>, | ||
aggregate_functions: Vec<ExprWithAlias>, // Function expression | ||
value_column: Vec<Ident>, | ||
value_column: Vec<Expr>, // Expr is a identifier or a compound identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value_column: Vec<Expr>, // Expr is a identifier or a compound identifier | |
value_column: Vec<Expr>, |
assert_eq!( | ||
verified_stmt(sql_with_multiple_value_column).to_string(), | ||
sql_with_multiple_value_column | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this assertion is probably not needed, I think verified_stmt already does the same assertion
let value_column = if self.peek_token_ref().token == Token::LParen { | ||
self.parse_parenthesized_compound_identifier_list(Mandatory, false)? | ||
} else { | ||
vec![Expr::CompoundIdentifier( | ||
self.parse_period_separated(|p| p.parse_identifier())?, | ||
)] | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to unpivot can we call parse_expr directly here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we cannot do it here. (name, age) IN (('John', 30) AS c1, ('Mike', 40) AS c2)
will be parsed as an expr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right that makes sense, I think we can do something this instead to reuse existing capabilities of the parser?
let value_column = if self.peek_token_ref().token == Token::LParen {
self.parse_parenthesized_column_list_inner(Mandatory, false, |p| {
p.parse_subexpr(self.dialect.prec_value(Precedence::Between))
})?
} else {
vec![self.parse_subexpr(self.dialect.prec_value(Precedence::Between))?]
};
let value_column = if self.peek_token_ref().token == Token::LParen { | ||
self.parse_parenthesized_compound_identifier_list(Mandatory, false)? | ||
} else { | ||
vec![Expr::CompoundIdentifier( | ||
self.parse_period_separated(|p| p.parse_identifier())?, | ||
)] | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right that makes sense, I think we can do something this instead to reuse existing capabilities of the parser?
let value_column = if self.peek_token_ref().token == Token::LParen {
self.parse_parenthesized_column_list_inner(Mandatory, false, |p| {
p.parse_subexpr(self.dialect.prec_value(Precedence::Between))
})?
} else {
vec![self.parse_subexpr(self.dialect.prec_value(Precedence::Between))?]
};
tests/sqlparser_common.rs
Outdated
alias: None | ||
}], | ||
value_column: vec![Ident::new("year")], | ||
value_column: vec![Expr::CompoundIdentifier(vec![Ident::new("year")])], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm heads up that this representation wouldn't be ideal - a compound identifier would be expected to have more than one ident in it
}), | ||
aggregate_functions: vec![ | ||
expected_function("a", None), | ||
expected_function("b", Some("t")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add tests to demonstrate the new behavior? it seems like those are currently lacking in the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @chenkovsky!
FYI @chenkovsky please re-request review explicitly when ready after addressing comments to avoid the PR getting missed in the queue
https://spark.apache.org/docs/latest/sql-ref-syntax-qry-select-pivot.html
currently this sql is not supported.