LucaCappelletti94 commented on code in PR #2436:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2436#discussion_r3740343976
##########
src/parser/mod.rs:
##########
@@ -4071,11 +4071,11 @@ impl<'a> Parser<'a> {
} else if self.parse_keywords(&[Keyword::NOT,
Keyword::UNKNOWN]) {
Ok(Expr::IsNotUnknown(Box::new(expr)))
} else if self.parse_keywords(&[Keyword::DISTINCT,
Keyword::FROM]) {
- let expr2 = self.parse_expr()?;
+ let expr2 = self.parse_subexpr(precedence)?;
Review Comment:
This regresses `->` / `@>` in the non-PostgreSQL dialects, such as MySQL. I
suggest you add the following red test in `tests/sqlparser_mysql.rs`, and
proceed from there.
```rust
#[test]
fn parse_is_distinct_from_json_arrow_precedence() {
// MySQL's `->` binds tighter than `IS [NOT] DISTINCT FROM`, so the JSON
// extraction must stay inside the right operand.
assert_eq!(
Expr::IsDistinctFrom(
Box::new(Expr::Identifier(Ident::new("a"))),
Box::new(Expr::BinaryOp {
left: Box::new(Expr::Identifier(Ident::new("b"))),
op: BinaryOperator::Arrow,
right: Box::new(Expr::Value(
Value::SingleQuotedString("k".into()).with_empty_span()
)),
}),
),
mysql().verified_expr("a IS DISTINCT FROM b -> 'k'")
);
}
```
The issue to be clear is found in the default table, while your patch merely
exposes this, but then it is a good occasion to fix it.
##########
tests/sqlparser_common.rs:
##########
@@ -1984,6 +1984,96 @@ fn parse_is_not_distinct_from() {
);
}
+#[test]
+fn parse_is_distinct_from_precedence() {
+ use self::Expr::*;
+
+ // The right operand of `IS [NOT] DISTINCT FROM` binds tighter than
`AND`/`OR`,
+ // so the boolean operator must end up at the root of the tree.
+ assert_eq!(
+ BinaryOp {
+ left: Box::new(IsDistinctFrom(
+ Box::new(Identifier(Ident::new("a"))),
+ Box::new(Expr::value(number("1"))),
+ )),
+ op: BinaryOperator::And,
+ right: Box::new(BinaryOp {
+ left: Box::new(Identifier(Ident::new("b"))),
+ op: BinaryOperator::Eq,
+ right: Box::new(Expr::value(number("2"))),
+ }),
+ },
+ verified_expr("a IS DISTINCT FROM 1 AND b = 2")
+ );
+
+ assert_eq!(
+ BinaryOp {
+ left: Box::new(IsNotDistinctFrom(
+ Box::new(Identifier(Ident::new("a"))),
+ Box::new(Expr::value(number("1"))),
+ )),
+ op: BinaryOperator::Or,
+ right: Box::new(BinaryOp {
+ left: Box::new(Identifier(Ident::new("b"))),
+ op: BinaryOperator::Eq,
+ right: Box::new(Expr::value(number("2"))),
+ }),
+ },
+ verified_expr("a IS NOT DISTINCT FROM 1 OR b = 2")
+ );
+
+ // `AND` binds tighter than `OR` within the surrounding expression.
+ assert_matches!(
+ verified_expr("a IS DISTINCT FROM 1 AND b OR c"),
+ BinaryOp {
+ op: BinaryOperator::Or,
+ ..
+ }
+ );
+ assert_matches!(
+ verified_expr("a IS DISTINCT FROM 1 OR b AND c"),
+ BinaryOp {
+ op: BinaryOperator::Or,
+ ..
+ }
+ );
+
+ // Explicit parentheses still push the boolean expression into the right
operand.
+ assert_eq!(
+ IsDistinctFrom(
+ Box::new(Identifier(Ident::new("a"))),
+ Box::new(Nested(Box::new(BinaryOp {
+ left: Box::new(Expr::value(number("1"))),
+ op: BinaryOperator::And,
+ right: Box::new(Identifier(Ident::new("b"))),
+ }))),
+ ),
+ verified_expr("a IS DISTINCT FROM (1 AND b)")
+ );
+
+ // The `IS` family is left-associative.
Review Comment:
````suggestion
// sqlparser resolves the IS family left-associatively, consistent with
how
// `a IS NULL IS NULL` already parses. Deliberately more permissive than
// PostgreSQL, which declares IS as %nonassoc and rejects the chain.
````
--
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]
