Skip to content

Honour operator precedence in IS [NOT] DISTINCT FROM #2436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zvonimir-dd wants to merge 3 commits into
base: main
Choose a base branch
from
+235 −3
Open

Honour operator precedence in IS [NOT] DISTINCT FROM #2436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54e8e47
Fix `IS [NOT] DISTINCT FROM` right-operand precedence
zvonimir-dd Aug 7, 2026
4c67348
Fix `PgOther` precedence in the default table
zvonimir-dd Aug 10, 2026
ebee692
Address review feedback on precedence fix
zvonimir-dd Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/dialect/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,11 +990,16 @@ pub trait Dialect: Debug + Any {
Precedence::Caret => 22,
Precedence::Pipe => 21,
Precedence::Colon => 21,
// "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,
Comment thread
zvonimir-dd marked this conversation as resolved.
Precedence::Between => 20,
Precedence::Eq => 20,
Precedence::Like => 19,
Precedence::Is => 17,
Precedence::PgOther => 16,
Precedence::UnaryNot => 15,
Precedence::And => 10,
Precedence::Or => 5,
Expand Down
7 changes: 5 additions & 2 deletions src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4071,11 +4071,14 @@ 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()?;
// 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)?;
Comment thread
zvonimir-dd marked this conversation as resolved.

@adriangb adriangb Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, but a one-line note here would save the next reader from re-deriving why this is not parse_expr():

Suggested change
let expr2 = self.parse_subexpr(precedence)?;
// 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)?;

@zvonimir-dd zvonimir-dd Aug 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied, on the DISTINCT FROM arm only — the NOT DISTINCT FROM arm two lines down is identical, so repeating it there felt like noise. Happy to add it to both if you'd rather I do that.

Ok(Expr::IsDistinctFrom(Box::new(expr), Box::new(expr2)))
} else if self.parse_keywords(&[Keyword::NOT, Keyword::DISTINCT, Keyword::FROM])
{
let expr2 = self.parse_expr()?;
let expr2 = self.parse_subexpr(precedence)?;
Ok(Expr::IsNotDistinctFrom(Box::new(expr), Box::new(expr2)))
} else if self.parse_keyword(Keyword::JSON) {
self.parse_is_json_predicate(expr, false)
Expand Down
191 changes: 191 additions & 0 deletions tests/sqlparser_common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1984,6 +1984,197 @@ 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_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")
);

// 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")
);
Comment thread
zvonimir-dd marked this conversation as resolved.

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")
);

// `NOT` binds more loosely than `IS`, so it applies to the whole comparison.
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")
);

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")
);
}

#[test]
fn parse_pg_other_operator_precedence() {
let arrow_k = |left: Expr| Expr::BinaryOp {
left: Box::new(left),
op: BinaryOperator::Arrow,
right: Box::new(Expr::Value(
Value::SingleQuotedString("k".into()).with_empty_span(),
)),
};
let t_a = || Expr::CompoundIdentifier(vec![Ident::new("t"), Ident::new("a")]);

// `->` binds tighter than comparison operators, so the arrow expression is
// the left operand rather than the comparison being the arrow's key.
let expected_eq = |left: Expr| Expr::BinaryOp {
left: Box::new(arrow_k(left)),
op: BinaryOperator::Eq,
right: Box::new(Identifier(Ident::new("b"))),
};

// Dialects with lambda functions read a bare `a ->` as the start of a lambda.
assert_eq!(
expected_eq(Identifier(Ident::new("a"))),
all_dialects_where(|d| !d.supports_lambda_functions()).verified_expr("a -> 'k' = b")
);
assert_eq!(
expected_eq(t_a()),
all_dialects().verified_expr("t.a -> 'k' = b")
);

// `LIKE` sits below `=` in the precedence table, so cover that boundary too.
assert_eq!(
Expr::Like {
negated: false,
any: false,
expr: Box::new(arrow_k(t_a())),
pattern: Box::new(Expr::Value(
Value::SingleQuotedString("x".into()).with_empty_span(),
)),
escape_char: None,
},
all_dialects().verified_expr("t.a -> 'k' LIKE 'x'")
);
}

#[test]
fn parse_not_precedence() {
// NOT has higher precedence than OR/AND, so the following must parse as (NOT true) OR true
Expand Down
33 changes: 33 additions & 0 deletions tests/sqlparser_mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4946,3 +4946,36 @@ 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_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'")
);
}

@adriangb adriangb Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

#[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.)

@zvonimir-dd zvonimir-dd Aug 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. parse_json_arrow_comparison_precedence is gone; parse_pg_other_operator_precedence in sqlparser_common.rs replaces it. t.a -> 'k' = b passes on all 15 dialects — a compound left operand is never read as a lambda — so only the bare a -> 'k' = b form needs all_dialects_where(|d| !d.supports_lambda_functions()). No dialect had to be excluded.

One deviation: I kept a LIKE case rather than dropping it. Like is 19 and Eq is 20, so the = assertion alone leaves the 19/21 boundary untested — a future change landing PgOther anywhere in 17..=19 would still pass = and silently break LIKE. Let me know if you'd prefer it gone.

Loading