adriangb commented on code in PR #2436:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2436#discussion_r3810256442
##########
src/dialect/mod.rs:
##########
@@ -990,11 +990,11 @@ pub trait Dialect: Debug + Any {
Precedence::Caret => 22,
Precedence::Pipe => 21,
Precedence::Colon => 21,
+ Precedence::PgOther => 21,
Review Comment:
Worth a comment here, since this line is the one that changes behaviour
beyond `IS [NOT] DISTINCT FROM` and its placement is not self-evident:
````suggestion
// "any other operator" -- `->`, `@>`, custom operators.
PostgreSQL
// places this row above `BETWEEN` / `LIKE` and below `+` / `-`
// (`%left Op OPERATOR RIGHT_ARROW '|'` in gram.y), so it must
bind
// more tightly than `IS`, whose right operand would otherwise
stop
// short of it.
Precedence::PgOther => 21,
````
##########
tests/sqlparser_mysql.rs:
##########
@@ -4946,3 +4946,48 @@ fn parse_adjacent_string_literal_concatenation() {
fn parse_group_by_with_rollup() {
mysql().verified_stmt("SELECT * FROM tbl GROUP BY col1, col2 WITH ROLLUP");
}
+
+#[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'")
+ );
Review Comment:
The `PgOther` change is in the **default** `prec_value`, so it applies to
every dialect that does not override it, not just MySQL. Testing only through
`mysql()` under-covers the hunk. `mysql_and_generic()` is the cheap widening
here, and `->>` is worth adding since `LongArrow` rides the same row:
````suggestion
mysql_and_generic().verified_expr("a IS DISTINCT FROM b -> 'k'")
);
assert_eq!(
Expr::IsNotDistinctFrom(
Box::new(Expr::Identifier(Ident::new("a"))),
Box::new(Expr::BinaryOp {
left: Box::new(Expr::Identifier(Ident::new("b"))),
op: BinaryOperator::LongArrow,
right: Box::new(Expr::Value(
Value::SingleQuotedString("k".into()).with_empty_span()
)),
}),
),
mysql_and_generic().verified_expr("a IS NOT DISTINCT FROM b ->> 'k'")
);
````
##########
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:
Non-blocking, but a one-line note here would save the next reader from
re-deriving why this is not `parse_expr()`:
````suggestion
// The right operand binds no more loosely than `IS`
// itself, so that e.g. `a IS DISTINCT FROM b AND c`
// parses as `(a IS DISTINCT FROM b) AND c`.
let expr2 = self.parse_subexpr(precedence)?;
````
##########
tests/sqlparser_common.rs:
##########
@@ -1984,6 +1984,98 @@ 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,
+ ..
+ }
+ );
Review Comment:
These do catch the bug this PR fixes (the unfixed parser roots these at
`IsDistinctFrom`, not `BinaryOp`). But matching only on the root operator
leaves the left subtree unpinned, so a future partial regression that produced
`IsDistinctFrom(a, 1 AND b) OR c` would still be rooted at `Or` and pass. Since
`AND`-binds-tighter-than-`OR` is the specific property being claimed, it is
worth asserting the whole tree:
````suggestion
// `AND` binds tighter than `OR` within the surrounding expression.
assert_eq!(
BinaryOp {
left: Box::new(BinaryOp {
left: Box::new(IsDistinctFrom(
Box::new(Identifier(Ident::new("a"))),
Box::new(Expr::value(number("1"))),
)),
op: BinaryOperator::And,
right: Box::new(Identifier(Ident::new("b"))),
}),
op: BinaryOperator::Or,
right: Box::new(Identifier(Ident::new("c"))),
},
verified_expr("a IS DISTINCT FROM 1 AND b OR c")
);
assert_eq!(
BinaryOp {
left: Box::new(IsDistinctFrom(
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::And,
right: Box::new(Identifier(Ident::new("c"))),
}),
},
verified_expr("a IS DISTINCT FROM 1 OR b AND c")
);
````
##########
tests/sqlparser_common.rs:
##########
@@ -1984,6 +1984,98 @@ 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)")
+ );
+
+ // 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.
+ assert_eq!(
+ IsNull(Box::new(IsDistinctFrom(
+ Box::new(Identifier(Ident::new("a"))),
+ Box::new(Identifier(Ident::new("b"))),
+ ))),
+ verified_expr("a IS DISTINCT FROM b IS NULL")
+ );
+
+ // Operators that bind tighter than `IS` are still part of the right
operand.
+ assert_eq!(
+ IsDistinctFrom(
+ Box::new(Identifier(Ident::new("a"))),
+ Box::new(BinaryOp {
+ left: Box::new(Identifier(Ident::new("b"))),
+ op: BinaryOperator::Plus,
+ right: Box::new(Expr::value(number("1"))),
+ }),
+ ),
+ verified_expr("a IS DISTINCT FROM b + 1")
+ );
Review Comment:
Three gaps I would add here. All three pass on this branch as written.
**Prefix `NOT`.** `UnaryNot` (15) is below `Is` (17), so `NOT` takes the
whole predicate. This is untested, and it is the case where a wrong grouping is
most dangerous: `NOT (A AND B)` vs `(NOT A) AND B` are silently different
results rather than a type error.
**Comparison inside the right operand.** `Eq` (20) is above `Is` (17), the
opposite side of the boundary from `AND`/`OR`. The `b + 1` case covers
arithmetic; this covers the row that is only three points away and therefore
the more likely one to get wrong.
**`IS ... DISTINCT FROM` on both sides of `AND`.** The existing cases use `b
= 2` as the right conjunct. The shape that actually broke in the wild
(apache/datafusion#23692, multi-column equality-delete resolution in Iceberg)
has the operator on both sides, which exercises re-entry into the same parse
path.
````suggestion
verified_expr("a IS DISTINCT FROM b + 1")
);
// Prefix `NOT` binds less tightly than `IS`, so it takes the whole
predicate.
assert_eq!(
UnaryOp {
op: UnaryOperator::Not,
expr: Box::new(IsDistinctFrom(
Box::new(Identifier(Ident::new("a"))),
Box::new(Identifier(Ident::new("b"))),
)),
},
verified_expr("NOT a IS DISTINCT FROM b")
);
// Comparison binds tighter than `IS`, so it stays in the right operand.
assert_eq!(
IsDistinctFrom(
Box::new(Identifier(Ident::new("a"))),
Box::new(BinaryOp {
left: Box::new(Identifier(Ident::new("b"))),
op: BinaryOperator::Eq,
right: Box::new(Identifier(Ident::new("c"))),
}),
),
verified_expr("a IS DISTINCT FROM b = c")
);
// The shape reported in apache/datafusion#23692: the operator on both
sides
// of `AND`, so each conjunct re-enters this parse path.
assert_eq!(
BinaryOp {
left: Box::new(IsNotDistinctFrom(
Box::new(Identifier(Ident::new("a"))),
Box::new(Identifier(Ident::new("b"))),
)),
op: BinaryOperator::And,
right: Box::new(IsNotDistinctFrom(
Box::new(Identifier(Ident::new("c"))),
Box::new(Identifier(Ident::new("d"))),
)),
},
verified_expr("a IS NOT DISTINCT FROM b AND c IS NOT DISTINCT FROM
d")
);
````
##########
tests/sqlparser_mysql.rs:
##########
@@ -4946,3 +4946,48 @@ fn parse_adjacent_string_literal_concatenation() {
fn parse_group_by_with_rollup() {
mysql().verified_stmt("SELECT * FROM tbl GROUP BY col1, col2 WITH ROLLUP");
}
+
+#[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'")
+ );
+}
+
+#[test]
+fn parse_json_arrow_comparison_precedence() {
+ // The same "any other operator" class also binds tighter than the
+ // comparison operators and `LIKE`, so the JSON extraction is the left
+ // operand rather than swallowing the right-hand side.
+ assert_eq!(
+ Expr::BinaryOp {
+ left: Box::new(Expr::BinaryOp {
+ left: Box::new(Expr::Identifier(Ident::new("a"))),
+ op: BinaryOperator::Arrow,
+ right: Box::new(Expr::Value(
+ Value::SingleQuotedString("k".into()).with_empty_span()
+ )),
+ }),
+ op: BinaryOperator::Eq,
+ right: Box::new(Expr::value(number("1"))),
+ },
+ mysql().verified_expr("a -> 'k' = 1")
+ );
+
+ assert_matches!(
+ mysql().verified_expr("a -> 'k' LIKE 'x'"),
+ Expr::Like { .. }
+ );
+}
Review Comment:
Since `parse_json_arrow_comparison_precedence` is really testing the default
precedence table rather than anything MySQL-specific, it may belong in
`tests/sqlparser_common.rs` run across dialects. Something like this covers all
11 non-lambda dialects in `all_dialects()` in one assertion:
```rust
#[test]
fn parse_pg_other_operator_precedence() {
// The "any other operator" row -- `->`, `@>`, custom operators -- binds
more
// tightly than comparison, `LIKE`, `BETWEEN` and the `IS` family,
matching
// PostgreSQL's `%left Op OPERATOR RIGHT_ARROW` placement. Dialects that
// support lambda functions consume `->` in prefix position instead.
let dialects = all_dialects_where(|d| !d.supports_lambda_functions());
assert_eq!(
Expr::BinaryOp {
left: Box::new(Expr::BinaryOp {
left: Box::new(Expr::Identifier(Ident::new("a"))),
op: BinaryOperator::Arrow,
right:
Box::new(Expr::value(Value::SingleQuotedString("k".to_string()))),
}),
op: BinaryOperator::Eq,
right: Box::new(Expr::Identifier(Ident::new("b"))),
},
dialects.verified_expr("a -> 'k' = b")
);
// A lambda is only recognised when `->` directly follows the parameter
list,
// so a qualified left operand reaches this precedence in EVERY dialect
--
// including those that support lambdas.
assert_eq!(
Expr::BinaryOp {
left: Box::new(Expr::BinaryOp {
left: Box::new(Expr::CompoundIdentifier(vec![
Ident::new("t"),
Ident::new("a"),
])),
op: BinaryOperator::Arrow,
right:
Box::new(Expr::value(Value::SingleQuotedString("k".to_string()))),
}),
op: BinaryOperator::Eq,
right: Box::new(Expr::Identifier(Ident::new("b"))),
},
all_dialects().verified_expr("t.a -> 'k' = b")
);
}
```
That second assertion is the one I would most want in the suite: it is the
only arrow coverage that reaches DuckDB, ClickHouse, Databricks and Snowflake,
which this hunk *does* affect whenever the left operand is not a bare
identifier. (Note `SparkSqlDialect` is not in `all_dialects()` at all, so
nothing here reaches it either way.)
--
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]
