Support remaining pipe operators #1879
Conversation
There was a problem hiding this comment.
Thanks @simonvandel! The changes look good to me overall! left some comments
| for (i, query) in queries.iter().enumerate() { | ||
| if i > 0 { | ||
| write!(f, ", ")?; | ||
| } | ||
| write!(f, "({})", query)?; | ||
| } |
There was a problem hiding this comment.
can this use the display_comma_separated() helper function?
| @@ -9947,6 +9947,51 @@ impl<'a> Parser<'a> { | |||
| Ok(IdentWithAlias { ident, alias }) | |||
| } | |||
|
|
|||
| /// Parse `identifier [AS] identifier` where the AS keyword is optional | |||
| pub fn parse_identifier_with_optional_alias(&mut self) -> Result<IdentWithAlias, ParserError> { | |||
There was a problem hiding this comment.
| pub fn parse_identifier_with_optional_alias(&mut self) -> Result<IdentWithAlias, ParserError> { | |
| fn parse_identifier_with_optional_alias(&mut self) -> Result<IdentWithAlias, ParserError> { |
| if self.parse_keywords(&[Keyword::DISTINCT, Keyword::BY, Keyword::NAME]) { | ||
| Ok(SetQuantifier::DistinctByName) | ||
| } else if self.parse_keyword(Keyword::DISTINCT) { | ||
| Ok(SetQuantifier::Distinct) | ||
| } else { | ||
| Err(ParserError::ParserError(format!( | ||
| "{} pipe operator requires DISTINCT modifier", | ||
| operator_name | ||
| ))) | ||
| } |
There was a problem hiding this comment.
Thinking since this block is a subset of the set_quantifier parsing logic here, it might be good to reuse? one way could be we pull the latter out into a function and call it here, rejecting any returned upexpected variants. another would be that we update the latter to call this parse_distinct_required_set_quantifier() function
| /// Parse optional alias (with or without AS keyword) for pipe operators | ||
| fn parse_optional_pipe_alias(&mut self) -> Result<Option<Ident>, ParserError> { | ||
| if self.parse_keyword(Keyword::AS) { | ||
| Some(self.parse_identifier()).transpose() |
There was a problem hiding this comment.
| Some(self.parse_identifier()).transpose() | |
| Ok(Some(self.parse_identifier()?)) |
thinking this variant would be more explicit in this case? (the transpose seemed to obfuscate a bit what was going on)
| } | ||
|
|
||
| /// Parse optional alias (with or without AS keyword) for pipe operators | ||
| fn parse_optional_pipe_alias(&mut self) -> Result<Option<Ident>, ParserError> { |
There was a problem hiding this comment.
| fn parse_optional_pipe_alias(&mut self) -> Result<Option<Ident>, ParserError> { | |
| fn parse_identifier_optional_alias(&mut self) -> Result<Option<Ident>, ParserError> { |
maybe we could rename this to something more generic? feels like it could be reusable in other contexts than pipe operators
| alias, | ||
| }); | ||
| } | ||
| Keyword::JOIN => { |
There was a problem hiding this comment.
for the joins, I'm wondering if its possible to pull out and reuse the existing logic here? e.g. here we'll prev_token() if needed whenever we see the next operator looks like a join and delegate to that helper function?
| if matches!(constraint, JoinConstraint::None) { | ||
| return Err(ParserError::ParserError( | ||
| "JOIN in pipe syntax requires ON or USING clause".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
thinking these constraint validations might not be ideal and we can skip them? in case the pipe syntax is later ported to other dialects, there the semantics might differ between dialects, prev related behavior
Fixes #1758
This PR adds support for the remaining SQL pipe operators as defined by BigQuery.