LucaCappelletti94 commented on code in PR #2614:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2614#discussion_r4251045244
##########
src/parser/mod.rs:
##########
@@ -1902,7 +1902,16 @@ impl<'a> Parser<'a> {
Err(e) => {
self.failed_reserved_word_prefix_positions
.insert(next_token_index, (&e).into());
- if !self.dialect.is_reserved_for_identifier(w.keyword)
{
+ // `INTERVAL 1` is a malformed interval, not a name.
+ let interval_number = w.keyword == Keyword::INTERVAL
+ && match &self.peek_token_ref().token {
+ Token::Number(..) => true,
+ Token::Plus | Token::Minus => {
+ matches!(self.peek_nth_token_ref(1).token,
Token::Number(..))
+ }
+ _ => false,
+ };
Review Comment:
You should keep only the `Token::Number` arm. Spark 4.0.1 parses `SELECT
interval - 1 FROM t`, `SELECT interval + 1 FROM t` and `SELECT INTERVAL -1` as
arithmetic on the column `interval`, but the sign arm rejects all three while
`interval * 2` parses. `INTERVAL` is [non-reserved in
Spark](https://spark.apache.org/docs/4.0.1/sql-ref-ansi-compliance.html#sql-keywords-optional-disabled-by-default),
so the grammar reads it as a [column
reference](https://github.com/apache/spark/blob/v4.0.1/sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4#L1183)
inside [unary and binary
arithmetic](https://github.com/apache/spark/blob/v4.0.1/sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4#L1134-L1136).
Without the arm the identifier fallback parses them the way Spark does, and
`SELECT INTERVAL 1` keeps its error.
```suggestion
let interval_number = w.keyword == Keyword::INTERVAL
&& matches!(self.peek_token_ref().token,
Token::Number(..));
```
##########
src/parser/mod.rs:
##########
@@ -3431,14 +3440,30 @@ impl<'a> Parser<'a> {
// to match the different flavours of INTERVAL syntax, we only allow
expressions
// if the dialect requires an interval qualifier,
// see https://github.com/sqlparser-rs/sqlparser-rs/pull/1398 for more
details
- let value = if self.dialect.require_interval_qualifier() {
+ let literal_value =
self.dialect.supports_interval_string_without_qualifier();
+ let value = if literal_value {
+ // otherwise `interval` is read as a name
+ self.parse_interval_literal_value()?
+ } else if self.dialect.require_interval_qualifier() {
// parse a whole expression so `INTERVAL 1 + 1 DAY` is valid
self.parse_expr()?
} else {
// parse a prefix expression so `INTERVAL 1 DAY` is valid, but
`INTERVAL 1 + 1 DAY` is not
// this also means that `INTERVAL '5 days' > INTERVAL '1 day'`
treated properly
self.parse_prefix()?
};
+ // only an unsigned string may omit the qualifier
+ let qualifier_required = if literal_value {
+ !matches!(
+ &value,
+ Expr::Value(ValueWithSpan {
+ value: Value::SingleQuotedString(_),
Review Comment:
You should let a double-quoted string omit the qualifier too, since Spark's
[`stringLit`](https://github.com/apache/spark/blob/v4.0.1/sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4#L1540-L1543)
includes `DOUBLEQUOTED_STRING` and [`INTERVAL
stringLit`](https://github.com/apache/spark/blob/v4.0.1/sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4#L1208)
takes no unit. Both the
[Spark](https://spark.apache.org/docs/4.0.1/sql-ref-literals.html#string-literal)
and
[Databricks](https://docs.databricks.com/aws/en/sql/language-manual/data-types/string-type#literals)
docs allow `"..."` as a string literal, and Spark 4.0.1 parses `SELECT
INTERVAL "2 months"` as `INTERVAL '2' MONTH`.
```suggestion
value: Value::SingleQuotedString(_) |
Value::DoubleQuotedString(_),
```
##########
src/parser/mod.rs:
##########
@@ -3490,6 +3515,35 @@ impl<'a> Parser<'a> {
}))
}
+ /// A signed number or string literal.
+ fn parse_interval_literal_value(&mut self) -> Result<Expr, ParserError> {
+ let sign = match self.peek_token_ref().token {
+ Token::Plus => Some(UnaryOperator::Plus),
+ Token::Minus => Some(UnaryOperator::Minus),
+ _ => None,
+ };
+ if sign.is_some() {
+ self.advance_token();
+ }
+ let next = self.next_token();
+ let value = match next.token {
+ Token::Number(n, l) => {
+ Expr::value(Value::Number(Self::parse(n, next.span.start)?,
l).with_span(next.span))
+ }
+ Token::SingleQuotedString(s) => {
+ Expr::value(Value::SingleQuotedString(s).with_span(next.span))
+ }
Review Comment:
You should accept `Token::DoubleQuotedString` here, since as aforementioned
Spark's
[`intervalValue`](https://github.com/apache/spark/blob/v4.0.1/sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4#L1250-L1253)
takes any `stringLit`. `SELECT INTERVAL "1" DAY` parses on `main` and in Spark
4.0.1 but fails on this branch. `SELECT INTERVAL "2 months"` now parses as
`SELECT INTERVAL AS "2 months"`, a column `INTERVAL` aliased `"2 months"`.
```suggestion
Token::SingleQuotedString(s) => {
Expr::value(Value::SingleQuotedString(s).with_span(next.span))
}
Token::DoubleQuotedString(s) => {
Expr::value(Value::DoubleQuotedString(s).with_span(next.span))
}
```
##########
tests/sqlparser_common.rs:
##########
@@ -6764,9 +6766,66 @@ fn parse_interval_require_unit() {
)
}
+#[test]
+fn parse_interval_string_without_qualifier() {
+ let dialects = all_dialects_where(|d|
d.supports_interval_string_without_qualifier());
+ for sql in [
+ "SELECT INTERVAL '2 months'",
+ "SELECT INTERVAL '1' DAY",
+ "SELECT INTERVAL '1-2' YEAR TO MONTH",
+ "SELECT INTERVAL -'1' DAY",
+ "SELECT INTERVAL 3 DAY",
+ "SELECT INTERVAL -1 DAY, INTERVAL +2 HOURS",
+ "SELECT INTERVAL '2 seconds' * 2",
Review Comment:
You could cover the double-quoted forms. Both lines fail on this branch and
pass with the two changes above.
```suggestion
"SELECT INTERVAL '2 seconds' * 2",
"SELECT INTERVAL \"2 months\"",
"SELECT INTERVAL \"1\" DAY",
```
##########
tests/sqlparser_common.rs:
##########
@@ -6764,9 +6766,66 @@ fn parse_interval_require_unit() {
)
}
+#[test]
+fn parse_interval_string_without_qualifier() {
+ let dialects = all_dialects_where(|d|
d.supports_interval_string_without_qualifier());
+ for sql in [
+ "SELECT INTERVAL '2 months'",
+ "SELECT INTERVAL '1' DAY",
+ "SELECT INTERVAL '1-2' YEAR TO MONTH",
+ "SELECT INTERVAL -'1' DAY",
+ "SELECT INTERVAL 3 DAY",
+ "SELECT INTERVAL -1 DAY, INTERVAL +2 HOURS",
+ "SELECT INTERVAL '2 seconds' * 2",
+ ] {
+ dialects.verified_stmt(sql);
+ }
+ for sql in [
+ "SELECT INTERVAL 1",
+ "SELECT INTERVAL 1 + 1 DAY",
+ "SELECT INTERVAL x DAY",
+ ] {
+ assert!(
+ dialects.parse_sql_statements(sql).is_err(),
+ "{sql} should not parse"
+ );
+ }
+ dialects.verified_stmt("SELECT interval, x FROM t");
+ dialects.verified_stmt("SELECT max(interval) FROM t WHERE interval > 1");
Review Comment:
You could cover signed arithmetic on the column. The new line fails on this
branch and passes with the guard change above.
```suggestion
dialects.verified_stmt("SELECT max(interval) FROM t WHERE interval > 1");
dialects.verified_stmt("SELECT interval - 1, interval + 1 FROM t");
```
--
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]
