Honour operator precedence in IS [NOT] DISTINCT FROM

[zvonimir-dd]

Parser::parse_infix parsed the right operand of IS DISTINCT FROM and IS NOT DISTINCT FROM with
parse_expr, which is parse_subexpr(0), so the operand swallowed every following operator —
including AND and OR.

For instance, a IS DISTINCT FROM 1 AND b = 2 parsed as a IS DISTINCT FROM (1 AND b = 2) instead
of (a IS DISTINCT FROM 1) AND (b = 2):

IsDistinctFrom(
    Identifier("a"),
    BinaryOp { left: Number("1"), op: And, right: BinaryOp { left: b, op: Eq, right: 2 } },
)

PostgreSQL's operator precedence table places the IS family above NOT, AND and OR, so
AND cannot be part of the right operand. Here is an expression that is well-typed under both
readings and distinguishes them:

SELECT true IS DISTINCT FROM true AND false;
-- (true IS DISTINCT FROM true) AND false  =  false AND false             =  false  <- correct
--  true IS DISTINCT FROM (true AND false) =  true IS DISTINCT FROM false =  true   <- before this PR

DuckDB, which follows PostgreSQL precedence here, returns false.

These two branches were ignoring the precedence their caller passed; using it fixes both. That value
is prec_value(Precedence::Is) for an IS token — 17 in the default Dialect impl, IS_PREC in
the PostgreSQL dialect. Since parse_subexpr breaks on precedence >= next_precedence, the operand
now stops at AND (10) and OR (5), and at a following IS — which is what makes the family
associate left — while still absorbing tighter operators such as + (30). The IS arm is not
dialect-gated, so this applies to every dialect; that looks intended, since MySQL's <=> and
MSSQL's IS [NOT] DISTINCT FROM bind the same way.

This is the same root cause as #2419, in a different hook. As there, nothing errored before, and
Display adds no parentheses, so the wrong tree reprinted as the original text — which is why a
round trip never caught it and the new test asserts on the tree instead.

The new parse_is_distinct_from_precedence covers:

a IS DISTINCT FROM 1 AND b = 2       ->  (a IS DISTINCT FROM 1) AND (b = 2)
a IS NOT DISTINCT FROM 1 OR b = 2    ->  (a IS NOT DISTINCT FROM 1) OR (b = 2)
a IS DISTINCT FROM 1 AND b OR c      ->  ((a IS DISTINCT FROM 1) AND b) OR c
a IS DISTINCT FROM 1 OR b AND c      ->  (a IS DISTINCT FROM 1) OR (b AND c)
a IS DISTINCT FROM (1 AND b)         ->  unchanged (explicit parens)
a IS DISTINCT FROM b IS NULL         ->  (a IS DISTINCT FROM b) IS NULL
a IS DISTINCT FROM b + 1             ->  a IS DISTINCT FROM (b + 1)

Each of the first six produces an IsDistinctFrom at the root before this change; the last two
guard against over-tightening. The IS NULL case is the IS-family left-associativity symptom of
the same precedence-0 call.

cargo test, cargo fmt and cargo clippy all pass.

[LucaCappelletti94]

While reading through the PR, I noticed that also DIV has the identical defect. 7 DIV 2 + 1 → 7 DIV (2 + 1) = 2, while MySQL gives 4. It is only vaguely associated to the current PR in terms of precedence errors, so it should likely be a different subsequent PR, I just wanted to jot it down so as to not forget it.

--- a/src/dialect/mysql.rs
+++ b/src/dialect/mysql.rs
@@ -99,10 +99,10 @@ impl Dialect for MySqlDialect {
-        _precedence: u8,
+        precedence: u8,
-            let right = Box::new(match parser.parse_expr() {
+            let right = Box::new(match parser.parse_subexpr(precedence) {

--- a/src/dialect/spark.rs
+++ b/src/dialect/spark.rs
@@ -138,9 +138,9 @@ impl Dialect for SparkSqlDialect {
-        _precedence: u8,
+        precedence: u8,
-            let right = Box::new(match parser.parse_expr() {
+            let right = Box::new(match parser.parse_subexpr(precedence) {

A red test for this could be:

#[test]
fn parse_div_precedence() {
    // `DIV` has the same precedence as `*` and `/`, so `+` must end up at the root.
    assert_eq!(
        Expr::BinaryOp {
            left: Box::new(Expr::BinaryOp {
                left: Box::new(Expr::value(number("7"))),
                op: BinaryOperator::MyIntegerDivide,
                right: Box::new(Expr::value(number("2"))),
            }),
            op: BinaryOperator::Plus,
            right: Box::new(Expr::value(number("1"))),
        },
        mysql().verified_expr("7 DIV 2 + 1")
    );
}

[zvonimir-dd]

@LucaCappelletti94 Sorry for bugging you, but what is the next step here?
I believe I addressed your feedback above. Is there anything else I should do on my end?
Thanks!

[LucaCappelletti94]

Hi @zvonimir-dd, I can only review the code and provide feedback so as to accelerate the work of maintainers, but I do not have write access to merge PRs.

[adriangb]

I arrived at this same fix independently while chasing apache/datafusion#23692, and ended up with a patch that is byte-identical to this one in both src/parser/mod.rs and src/dialect/mod.rs. Rather than open a duplicate PR, here is the supporting evidence I gathered, plus some concrete test suggestions. I think this should be merged — both hunks are correct.

Two things below may be useful to other reviewers: primary-source grounding for the PgOther hunk, which is the one that changes behaviour broadly, and a warning about DuckDB, which is easy to reach for here and will point the wrong way.

Why the PgOther hunk is required, not incidental

It is worth being explicit that this PR contains two changes, and the second is not optional cleanup — without it the first is a regression for every dialect except PostgreSQL.

Precedence::PgOther is the "any other operator" row: ->, ->>, @>, ?, CustomBinaryOperator. In the default table it sat at 16, below Is (17). So once the right operand of IS [NOT] DISTINCT FROM correctly stops at precedence, it also stops at ->, and a IS DISTINCT FROM b -> 'k' regroups to (a IS DISTINCT FROM b) -> 'k'. (This is the same defect the sibling PR #2443 still has, since it changes only the parser.)

PostgreSQL's gram.y settles it. Precedence increases downward:

%left     OR
%left     AND
%right    NOT
%nonassoc IS ISNULL NOTNULL     /* IS sets precedence for IS NULL, etc */
%nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
%nonassoc BETWEEN IN_P LIKE ILIKE SIMILAR NOT_LA
...
%left     Op OPERATOR RIGHT_ARROW '|'  /* multi-character ops and user-defined operators */
%left     '+' '-'

RIGHT_ARROW is literally ->, and the row sits above BETWEEN/LIKE/comparison/IS and below +/-. src/dialect/postgresql.rs already encodes exactly this (PG_OTHER_PREC 70, between PLUS_MINUS_PREC 80 and BETWEEN_LIKE_PREC 60) — only the default table was inverted. Moving it to 21 alongside Pipe and Colon is consistent, since the PostgreSQL dialect maps those to PG_OTHER_PREC too.

An MRE where the two readings are not merely different — PostgreSQL rejects the pre-fix one (verified on postgres:17):

-- reading A: what PostgreSQL does, and what this PR produces
SELECT '{"k": 1}'::jsonb IS DISTINCT FROM ('{"k": 1}'::jsonb -> 'k');  -- t

-- reading B: what the parser produced for Generic/MySQL before the PgOther hunk
SELECT ('{"k": 1}'::jsonb IS DISTINCT FROM '{"k": 1}'::jsonb) -> 'k';
-- ERROR:  operator does not exist: boolean -> unknown

-- unparenthesised agrees with reading A
SELECT '{"k": 1}'::jsonb IS DISTINCT FROM '{"k": 1}'::jsonb -> 'k';    -- t

A warning: do not cite DuckDB for the -> row

The PR description cites DuckDB for the AND case, which is correct — DuckDB and PostgreSQL both return false for SELECT true IS DISTINCT FROM true AND false. But DuckDB disagrees with PostgreSQL about -> specifically, so anyone reaching for it to check the PgOther hunk will get a misleading answer:

-- DuckDB returns type `json`, not `boolean` => it parses (a IS DISTINCT FROM b) -> 'k'
SELECT pg_typeof('{"k": 1}'::JSON IS DISTINCT FROM '{"k": 1}'::JSON -> 'k');

This is not DuckDB following a different-but-considered convention. It is a known limitation they intend to remove. DuckDB puts SINGLE_ARROW/DOUBLE_ARROW in their own row below OR — the loosest tier — and has no RIGHT_ARROW token at all. The cause is visible in their history: commit 80d723ad40 (2021-01-22) moved LAMBDA_ARROW down to that row "so lambdas such as x -> x > 10 AND z < 20 are correctly parsed" — before DuckDB had a JSON -> operator. JSON -> was later overloaded onto the same token and inherited the placement.

Their maintainers describe it as unwanted, not as a design choice:

• duckdb/duckdb#23405 (open): "an unfortunate side effect of our current precedence of the JSON arrow operators in the grammar ... We plan to fix this but due to this being conflicting with (now deprecated) lambda syntax this will take some more time."
• duckdb/duckdb#10786: "there is a precedence conflict here ... As such there is no good solution here - so parenthesis are just going to be required here."

The fix is gated on retiring -> lambdas (deprecated 1.3, off by default 2.0, flag removed 2.1). There is a long tail of user reports: #14889, #23405, #20366 still open.

Worth noting for this repo: sqlparser does not have DuckDB's problem, because lambdas are handled in prefix position (src/parser/mod.rs, the Token::Arrow if self.dialect.supports_lambda_functions() arm) with a greedy parse_expr() body, rather than as an infix operator with a precedence. A bison grammar must commit one token to one row; recursive descent can disambiguate by position. So this PR can give -> its PostgreSQL-correct precedence without touching lambda parsing — which is exactly the tradeoff DuckDB cannot make.

One consequence that is easy to miss, and that I think deserves a test (see inline): the lambda path only fires when -> directly follows the parameter list. A qualified or cast left operand reaches PgOther in every dialect, lambda-supporting ones included:

expression	DuckDB / ClickHouse	affected by this hunk
a -> 'k'	lambda	no
(a) -> 'k'	lambda	no
t.a -> 'k'	JSON arrow	yes
a::JSON -> 'k'	JSON arrow	yes
f(a) -> 'k'	JSON arrow	yes

End-to-end verification against DataFusion

I applied this change to sqlparser v0.62.0 and pointed apache/datafusion main (@ 9a96f6715f) at it via [patch.crates-io]:

• every case in apache/datafusion#23692 that previously failed to plan with Cannot infer common argument type for logical boolean operation Int64 AND Boolean now plans and returns the correct result, including the multi-column LEFT ANTI JOIN
• on AND/OR and NOT cases chosen so that an incorrect grouping changes the result rather than raising an error, DataFusion now matches PostgreSQL 17 exactly
• no regressions: datafusion-sql 88 + 572 + 12 passed; sqllogictest 502/502 files; datafusion-optimizer + datafusion-expr 248 + 760 + 26 + 55 + 5 passed

The sqllogictest run is the meaningful check on the PgOther hunk — it exercises ->/->>, LIKE, BETWEEN and comparisons broadly, and nothing moved. This let me close the DataFusion-side workaround PR (apache/datafusion#24479) in favour of this fix.

Suggestions

All inline. Summary: strengthen two assertions to pin the whole tree, add four uncovered cases (prefix NOT, comparison in the right operand, IS ... FROM on both sides of AND, ->>), and widen the arrow tests beyond mysql(). I verified every snippet compiles and passes on this branch at 4c67348.

[adriangb]

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:

Suggested change

	Precedence::PgOther => 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,

[adriangb]

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

[adriangb]

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:

Suggested change

	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,
	..
	}
	);
	// `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")
	);

[adriangb]

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.

Suggested change

	verified_expr("a IS DISTINCT FROM b + 1")
	);
	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")
	);

[adriangb]

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:

Suggested change

	mysql().verified_expr("a IS DISTINCT FROM b -> 'k'")
	);
	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]

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

[LucaCappelletti94]

Please guys, stop with this automated reviews. I hardly think that automated code generation with automated reviews is helping. This is ouroboric and adds even more work to the maintainers instead of reducing it, and will end up slowing down these PRs ever getting merged.