iffyio commented on code in PR #2398:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2398#discussion_r3577123787
##########
src/parser/mod.rs:
##########
@@ -19386,6 +19388,18 @@ impl<'a> Parser<'a> {
Ok(InterpolateExpr { column, expr })
}
+ /// Returns true if the parser is positioned at a `TOP` clause, i.e. the
+ /// `TOP` keyword followed by `(` or a number. When `TOP` is followed by
+ /// anything else it is an ordinary identifier (column, alias, or table
+ /// name), not the row-limit clause parsed by [`Self::parse_top`].
+ fn peek_top_clause(&self) -> bool {
+ self.peek_keyword(Keyword::TOP)
+ && matches!(
+ self.peek_nth_token_ref(1).token,
+ Token::LParen | Token::Number(_, _)
+ )
+ }
Review Comment:
actually instead of having a peek and a parse for the top clause, we could
change parse_top_clause to `maybe_parse_top_clause() -> Option<>`then the
callers only need to check the dialect support and call if set
##########
src/parser/mod.rs:
##########
@@ -15126,7 +15127,8 @@ impl<'a> Parser<'a> {
self.parse_all_or_distinct()?
};
- if !self.dialect.supports_top_before_distinct() &&
self.parse_keyword(Keyword::TOP) {
+ if !self.dialect.supports_top_before_distinct() &&
self.peek_top_clause() {
+ self.expect_keyword(Keyword::TOP)?;
top = Some(self.parse_top()?);
Review Comment:
thinking one improvement we could do while we're making this change, can we
move the expect_keyword into the parse_top function? that way the function is
standalone (we can change the other callsite below to remove its expect_keyword
to match)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
