iffyio commented on code in PR #1732:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1732#discussion_r1969053806
##########
src/parser/mod.rs:
##########
@@ -7646,6 +7646,39 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_index_exprs(&mut self) -> Result<Vec<IndexExpr>, ParserError>
{
+ self.parse_parenthesized(|p|
p.parse_comma_separated(Parser::parse_index_expr))
+ }
+
+ pub fn parse_index_expr(&mut self) -> Result<IndexExpr, ParserError> {
+ let expr = self.parse_expr()?;
+ let collation = if self.parse_keyword(Keyword::COLLATE) {
+ Some(self.parse_object_name(false)?)
+ } else {
+ None
+ };
+
+ let (operator_class, order_options) = if
self.peek_keyword(Keyword::ASC)
+ || self.peek_keyword(Keyword::DESC)
+ || self.peek_keyword(Keyword::NULLS)
+ {
+ let order_options = self.parse_order_by_options()?;
+ (None, order_options)
+ } else {
+ let operator_class = self.maybe_parse(|p| p.parse_expr())?;
Review Comment:
hmm I couldn't see `operator_class` being used in the introduced tests or
documented so its unclear what the syntax is?
##########
src/parser/mod.rs:
##########
@@ -7646,6 +7646,39 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_index_exprs(&mut self) -> Result<Vec<IndexExpr>, ParserError>
{
+ self.parse_parenthesized(|p|
p.parse_comma_separated(Parser::parse_index_expr))
+ }
+
+ pub fn parse_index_expr(&mut self) -> Result<IndexExpr, ParserError> {
Review Comment:
Since this is flagged as a public function can we add a short description
mentioning what it does?
##########
src/parser/mod.rs:
##########
@@ -7646,6 +7646,39 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_index_exprs(&mut self) -> Result<Vec<IndexExpr>, ParserError>
{
+ self.parse_parenthesized(|p|
p.parse_comma_separated(Parser::parse_index_expr))
+ }
+
+ pub fn parse_index_expr(&mut self) -> Result<IndexExpr, ParserError> {
+ let expr = self.parse_expr()?;
+ let collation = if self.parse_keyword(Keyword::COLLATE) {
+ Some(self.parse_object_name(false)?)
+ } else {
+ None
+ };
+
+ let (operator_class, order_options) = if
self.peek_keyword(Keyword::ASC)
+ || self.peek_keyword(Keyword::DESC)
+ || self.peek_keyword(Keyword::NULLS)
Review Comment:
can we introduce a test case that covers the `ASC/DESC` and `nulls_first`
syntax?
##########
src/parser/mod.rs:
##########
@@ -14688,6 +14721,8 @@ impl Word {
#[cfg(test)]
mod tests {
+ use std::vec;
+
Review Comment:
hmm is this needed?
##########
src/parser/mod.rs:
##########
@@ -7646,6 +7646,39 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_index_exprs(&mut self) -> Result<Vec<IndexExpr>, ParserError>
{
+ self.parse_parenthesized(|p|
p.parse_comma_separated(Parser::parse_index_expr))
+ }
+
+ pub fn parse_index_expr(&mut self) -> Result<IndexExpr, ParserError> {
+ let expr = self.parse_expr()?;
+ let collation = if self.parse_keyword(Keyword::COLLATE) {
+ Some(self.parse_object_name(false)?)
+ } else {
+ None
+ };
+
+ let (operator_class, order_options) = if
self.peek_keyword(Keyword::ASC)
+ || self.peek_keyword(Keyword::DESC)
+ || self.peek_keyword(Keyword::NULLS)
Review Comment:
Instead of using `peek`, we can probably use `self.maybe_parse()` so that we
don't need to maintain this list of keywords in sync with the
`parse_order_by_options` function e.g.
```rust
let (operator_class, order_options) = if let Some(order_options ) =
self.maybe_parse(|p| p.parse_order_by_options())? {
(None, order_options)
} else {
let operator_class = self.maybe_parse(|p| p.parse_expr())?;
let order_options = self.parse_order_by_options()?;
(operator_class, order_options)
};
```
--
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
