mvzink commented on code in PR #1765:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1765#discussion_r1992044192
##########
src/parser/mod.rs:
##########
@@ -10272,20 +10269,33 @@ impl<'a> Parser<'a> {
{
// MySQL style LIMIT x,y => LIMIT y OFFSET x.
// Check
<https://dev.mysql.com/doc/refman/8.0/en/select.html> for more details.
- offset = Some(Offset {
- value: limit.unwrap(),
- rows: OffsetRows::None,
- });
+ limit_comma_offset = limit.take();
limit = Some(self.parse_expr()?);
}
}
- let limit_by = if dialect_of!(self is ClickHouseDialect |
GenericDialect)
+ let limit_by = if limit_comma_offset.is_none()
+ && dialect_of!(self is ClickHouseDialect | GenericDialect)
&& self.parse_keyword(Keyword::BY)
{
- self.parse_comma_separated(Parser::parse_expr)?
+ Some(self.parse_comma_separated(Parser::parse_expr)?)
} else {
- vec![]
+ None
+ };
+
+ let limit_clause = if let Some(offset) = limit_comma_offset {
+ Some(LimitClause::OffsetCommaLimit {
+ offset,
+ limit: limit.expect("Expected <limit> expression for LIMIT
<offset>, <limit>"),
+ })
+ } else if limit.is_some() || offset.is_some() ||
limit_by.is_some() {
Review Comment:
I reorganized it even more because the loop approach was hard to reason
about. And in truth, all it is accomplishing is trying to parse `OFFSET` both
before and after trying to parse `LIMIT`. So I just... did that. It seems fine
because parsing offset is the simplest part. Let me know if the new approach
makes sense. Added some extra tests for cases like you are mentioning too.
--
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]
