iffyio commented on code in PR #1765:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1765#discussion_r1990717603
##########
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()
Review Comment:
should this condition rather be `if limit.is_some()`? it looks like `SELECT
* FROM T BY abc` would parse successfully but the `BY clause` will be dropped,
since all the limit/offset options above will be None whereas here we will
parse the `BY` without using it
##########
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:
for the full flow, thinking the post processing could be combined with the
loop in order to avoid the is_some() checks and would make a few currently
invalid syntax be rejected, thinking something like this?
```rust
let limit_clause = None;
let limit = None;
let offset = None;
for _ in 0..3 {
if offset.is_none() && self.parse_keyword(OFFSET) {
offset = Some(self.parse_offset()?);
continue
}
if limit.is_none() && self.parse_keyword(LIMIT) {
let expr = self.parse_limit()?;
if offset.is_none() && self.dialect.supports_limit_comma() &&
self.consume_token(Comma) {
limit_clause = Some(LimitClause::OffsetCommLimit {
offset: expr,
limit: self.parse_expr()?;
});
break
}
continue
}
if limit.is_some() && dialect_of!(self is ClickHouseDialect |
GenericDialect) && self.parse_keyword(BY) {
limit_clause = Some(LimitClause::LimitOffset{
limit: limit.take(),
offset: offset.take(),
limit_by: Some(self.parse_comma_separated(Parser::parse_expr)?)
})
break
}
limit_clause = if limit.is_some() || offset_is_some() {
limit_clause = Some(LimitClause::LimitOffset{
limit: limit,
offset: offset,
limit_by: None
});
};
break
}
```
atm looks like there are some queries like `SELECT * FROM T OFFSET 5 BY foo`
which look like they'd be accepted but aren't valid, figured it might be
clearer to make those terminal states visible in the code with above snippet
--
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
