From 1228c64d2e966e0bb12b8d01504ad75d4db5db51 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 01:35:11 +0000 Subject: [PATCH 1/3] fix: correct operator precedence for IS [NOT] DISTINCT FROM `sqlparser` parses the right operand of `IS [NOT] DISTINCT FROM` with `parse_expr()`, i.e. at the lowest possible precedence, so operators that bind less tightly than `IS` are swallowed into the right operand: a IS NOT DISTINCT FROM b AND c IS NOT DISTINCT FROM d parses as `a IS NOT DISTINCT FROM (b AND (c IS NOT DISTINCT FROM d))` instead of `(a IS NOT DISTINCT FROM b) AND (c IS NOT DISTINCT FROM d)`. Planning then fails with Cannot infer common argument type for logical boolean operation Int64 AND Boolean which makes multi-column `IS NOT DISTINCT FROM` joins unusable unless every condition is parenthesised. Restore the expected associativity in the SQL planner: before planning an expression, flatten its `AND`/`OR` spine, re-attach each `IS [NOT] DISTINCT FROM` (and each `NOT`, which binds more tightly as well) to only the first operand of its right hand side, and rebuild the expression with `AND` binding more tightly than `OR`. The rewrite is skipped unless the mis-parse is actually present, and its output is a fixed point, so it cannot loop. Closes #23692 --- datafusion/sql/src/expr/mod.rs | 161 +++++++++++++++++- datafusion/sql/tests/sql_integration.rs | 141 +++++++++++++++ .../test_files/join_is_not_distinct_from.slt | 100 +++++++++++ 3 files changed, 401 insertions(+), 1 deletion(-) diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index c2e4822f76b9..a4c6c347e3f7 100644 --- a/datafusion/sql/src/expr/mod.rs +++ b/datafusion/sql/src/expr/mod.rs @@ -25,7 +25,7 @@ use sqlparser::ast::{ AccessExpr, BinaryOperator, CastFormat, CastKind, CeilFloorKind, DataType as SQLDataType, DateTimeField, DictionaryField, Expr as SQLExpr, ExprWithAlias as SQLExprWithAlias, JsonPath, MapEntry, Spanned, StructField, - Subscript, TrimWhereField, TypedString, Value, ValueWithSpan, + Subscript, TrimWhereField, TypedString, UnaryOperator, Value, ValueWithSpan, }; use sqlparser::ast::{Query, Visit, Visitor}; @@ -69,6 +69,157 @@ fn null_value_span(expr: &SQLExpr) -> Option> { } } +/// Returns `true` if `op` binds less tightly than `IS [NOT] DISTINCT FROM`. +fn is_and_or(op: &BinaryOperator) -> bool { + matches!(op, BinaryOperator::And | BinaryOperator::Or) +} + +/// Builds an `IS [NOT] DISTINCT FROM` SQL AST node. +fn distinct_from_expr(left: SQLExpr, right: SQLExpr, negated: bool) -> SQLExpr { + let (left, right) = (Box::new(left), Box::new(right)); + if negated { + SQLExpr::IsNotDistinctFrom(left, right) + } else { + SQLExpr::IsDistinctFrom(left, right) + } +} + +/// Builds a binary SQL AST node. +fn binary_op(left: SQLExpr, op: BinaryOperator, right: SQLExpr) -> SQLExpr { + SQLExpr::BinaryOp { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +/// Returns `true` if `expr` contains an `IS [NOT] DISTINCT FROM` whose right +/// operand swallowed a following `AND` / `OR`. +/// +/// See [`fix_distinct_from_precedence`]. +#[cfg_attr(feature = "recursive_protection", recursive::recursive)] +fn has_greedy_distinct_from(expr: &SQLExpr) -> bool { + match expr { + SQLExpr::BinaryOp { left, op, right } if is_and_or(op) => { + has_greedy_distinct_from(left) || has_greedy_distinct_from(right) + } + SQLExpr::IsDistinctFrom(_, right) | SQLExpr::IsNotDistinctFrom(_, right) => { + matches!(right.as_ref(), SQLExpr::BinaryOp { op, .. } if is_and_or(op)) + || has_greedy_distinct_from(right) + } + SQLExpr::UnaryOp { + op: UnaryOperator::Not, + expr, + } => has_greedy_distinct_from(expr), + _ => false, + } +} + +/// Restores the expected operator precedence around `IS [NOT] DISTINCT FROM`. +/// +/// `sqlparser` parses the right operand of `IS [NOT] DISTINCT FROM` as a +/// complete expression instead of stopping at the first operator that binds +/// less tightly than `IS`, so a following `AND` / `OR` gets swallowed into the +/// right operand. For example +/// +/// ```sql +/// a IS NOT DISTINCT FROM b AND c IS NOT DISTINCT FROM d +/// ``` +/// +/// is parsed as +/// +/// ```sql +/// a IS NOT DISTINCT FROM (b AND (c IS NOT DISTINCT FROM d)) +/// ``` +/// +/// which contradicts PostgreSQL, where `AND` binds less tightly than `IS`, and +/// fails to plan because `AND` requires boolean arguments. +/// +/// This function flattens the `AND` / `OR` spine of `expr`, re-attaching every +/// `IS [NOT] DISTINCT FROM` (and every `NOT`, which binds more tightly as well) +/// to just the first operand of its right hand side, and rebuilds the expression +/// with `AND` binding more tightly than `OR`, yielding +/// +/// ```sql +/// (a IS NOT DISTINCT FROM b) AND (c IS NOT DISTINCT FROM d) +/// ``` +/// +/// The result never contains an `IS [NOT] DISTINCT FROM` whose right operand is +/// an `AND` / `OR`, so applying this function to its own output is a no-op. +/// +/// Operands are not descended into; a parenthesised sub-expression is fixed when +/// the planner recurses into it. +fn fix_distinct_from_precedence(expr: SQLExpr) -> SQLExpr { + let (first, rest) = flatten_and_or(expr); + rebuild_and_or(first, rest) +} + +/// Flattens the `AND` / `OR` spine of `expr` into its first operand followed by +/// the remaining `(operator, operand)` pairs in source order, moving whatever an +/// `IS [NOT] DISTINCT FROM` greedily absorbed back onto the spine. +#[cfg_attr(feature = "recursive_protection", recursive::recursive)] +fn flatten_and_or(expr: SQLExpr) -> (SQLExpr, Vec<(BinaryOperator, SQLExpr)>) { + match expr { + SQLExpr::BinaryOp { left, op, right } if is_and_or(&op) => { + let (first, mut rest) = flatten_and_or(*left); + let (right_first, right_rest) = flatten_and_or(*right); + rest.push((op, right_first)); + rest.extend(right_rest); + (first, rest) + } + // Only the first operand of the right hand side belongs to the + // comparison, the rest stays on the spine. + SQLExpr::IsDistinctFrom(left, right) => { + let (right_first, rest) = flatten_and_or(*right); + (distinct_from_expr(*left, right_first, false), rest) + } + SQLExpr::IsNotDistinctFrom(left, right) => { + let (right_first, rest) = flatten_and_or(*right); + (distinct_from_expr(*left, right_first, true), rest) + } + // `NOT` binds more tightly than `AND` / `OR` too, so it only negates the + // first operand of its operand's spine. + SQLExpr::UnaryOp { + op: op @ UnaryOperator::Not, + expr, + } => { + let (first, rest) = flatten_and_or(*expr); + ( + SQLExpr::UnaryOp { + op, + expr: Box::new(first), + }, + rest, + ) + } + other => (other, vec![]), + } +} + +/// Rebuilds the flattened spine produced by [`flatten_and_or`] with `AND` +/// binding more tightly than `OR`, both left associative. +fn rebuild_and_or(first: SQLExpr, rest: Vec<(BinaryOperator, SQLExpr)>) -> SQLExpr { + // `AND` binds more tightly, so fold consecutive `AND`s into a group and + // combine the completed groups with `OR` as they are closed. + let mut and_group = first; + let mut or_expr: Option = None; + for (op, right) in rest { + if matches!(op, BinaryOperator::Or) { + let completed = std::mem::replace(&mut and_group, right); + or_expr = Some(match or_expr.take() { + Some(previous) => binary_op(previous, op, completed), + None => completed, + }); + } else { + and_group = binary_op(and_group, op, right); + } + } + match or_expr { + Some(previous) => binary_op(previous, BinaryOperator::Or, and_group), + None => and_group, + } +} + fn null_equality_warning(expr: &SQLExpr) -> Option { let SQLExpr::BinaryOp { left, op, right } = expr else { return None; @@ -156,6 +307,14 @@ impl SqlToRel<'_, S> { schema: &DFSchema, planner_context: &mut PlannerContext, ) -> Result { + // Work around the greedy parsing of `IS [NOT] DISTINCT FROM`'s right + // operand, see `fix_distinct_from_precedence` + let sql = if has_greedy_distinct_from(&sql) { + fix_distinct_from_precedence(sql) + } else { + sql + }; + enum StackEntry { SQLExpr(Box), Operator(BinaryOperator), diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs index a4bf0db91077..70eff6fad515 100644 --- a/datafusion/sql/tests/sql_integration.rs +++ b/datafusion/sql/tests/sql_integration.rs @@ -4206,6 +4206,147 @@ fn join_on_complex_condition() { ); } +#[test] +fn join_on_multiple_is_not_distinct_from_conditions() { + // `IS [NOT] DISTINCT FROM` binds more tightly than `AND`, so the right hand + // side of each operator must not swallow the following `AND`. + // See https://github.com/apache/datafusion/issues/23692 + let sql = "SELECT id, order_id \ + FROM person \ + JOIN orders ON id IS NOT DISTINCT FROM customer_id AND person.age IS NOT DISTINCT FROM orders.qty"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id, orders.order_id + Inner Join: Filter: person.id IS NOT DISTINCT FROM orders.customer_id AND person.age IS NOT DISTINCT FROM orders.qty + TableScan: person + TableScan: orders + " + ); +} + +#[test] +fn join_on_multiple_is_distinct_from_conditions() { + let sql = "SELECT id, order_id \ + FROM person \ + JOIN orders ON id IS DISTINCT FROM customer_id AND person.age IS DISTINCT FROM orders.qty"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id, orders.order_id + Inner Join: Filter: person.id IS DISTINCT FROM orders.customer_id AND person.age IS DISTINCT FROM orders.qty + TableScan: person + TableScan: orders + " + ); +} + +#[test] +fn where_is_not_distinct_from_with_and() { + let sql = "SELECT id FROM person WHERE id IS NOT DISTINCT FROM 1 AND age > 30"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id + Filter: person.id IS NOT DISTINCT FROM Int64(1) AND person.age > Int64(30) + TableScan: person + " + ); +} + +#[test] +fn where_is_not_distinct_from_with_or() { + let sql = "SELECT id FROM person WHERE id IS NOT DISTINCT FROM 1 OR age > 30"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id + Filter: person.id IS NOT DISTINCT FROM Int64(1) OR person.age > Int64(30) + TableScan: person + " + ); +} + +#[test] +fn where_is_not_distinct_from_chained_conditions() { + let sql = "SELECT id FROM person \ + WHERE id IS NOT DISTINCT FROM 1 AND age IS NOT DISTINCT FROM 2 OR salary IS NOT DISTINCT FROM 3"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id + Filter: person.id IS NOT DISTINCT FROM Int64(1) AND person.age IS NOT DISTINCT FROM Int64(2) OR person.salary IS NOT DISTINCT FROM Int64(3) + TableScan: person + " + ); +} + +#[test] +fn where_is_not_distinct_from_mixed_with_other_predicates() { + // `OR` must be lifted above the enclosing `AND` + let sql = "SELECT id FROM person \ + WHERE age > 30 AND id IS NOT DISTINCT FROM 1 OR salary IS NOT DISTINCT FROM 2"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id + Filter: person.age > Int64(30) AND person.id IS NOT DISTINCT FROM Int64(1) OR person.salary IS NOT DISTINCT FROM Int64(2) + TableScan: person + " + ); +} + +#[test] +fn where_is_not_distinct_from_parenthesized_is_unchanged() { + let sql = "SELECT id FROM person \ + WHERE (id IS NOT DISTINCT FROM 1) AND (age IS NOT DISTINCT FROM 2)"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id + Filter: person.id IS NOT DISTINCT FROM Int64(1) AND person.age IS NOT DISTINCT FROM Int64(2) + TableScan: person + " + ); +} + +#[test] +fn where_is_not_distinct_from_explicit_grouping_is_preserved() { + // Parentheses still win over the implicit precedence + let sql = "SELECT id FROM person \ + WHERE id IS NOT DISTINCT FROM 1 AND (age IS NOT DISTINCT FROM 2 OR salary IS NOT DISTINCT FROM 3)"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id + Filter: person.id IS NOT DISTINCT FROM Int64(1) AND (person.age IS NOT DISTINCT FROM Int64(2) OR person.salary IS NOT DISTINCT FROM Int64(3)) + TableScan: person + " + ); +} + +#[test] +fn is_not_distinct_from_binds_tighter_than_and_in_projection() { + // The right hand side keeps operators that bind more tightly than `IS` + let sql = "SELECT id IS NOT DISTINCT FROM age + 1 AND first_name IS NOT DISTINCT FROM last_name FROM person"; + let plan = logical_plan(sql).unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.id IS NOT DISTINCT FROM person.age + Int64(1) AND person.first_name IS NOT DISTINCT FROM person.last_name + TableScan: person + " + ); +} + #[test] fn hive_aggregate_with_filter() -> Result<()> { let dialect = &HiveDialect {}; diff --git a/datafusion/sqllogictest/test_files/join_is_not_distinct_from.slt b/datafusion/sqllogictest/test_files/join_is_not_distinct_from.slt index 1b6f2e4c8638..573fe01c102e 100644 --- a/datafusion/sqllogictest/test_files/join_is_not_distinct_from.slt +++ b/datafusion/sqllogictest/test_files/join_is_not_distinct_from.slt @@ -321,6 +321,106 @@ JOIN t4 ON (t3.id = t4.id) AND (t3.val1 IS NOT DISTINCT FROM t4.val1) AND (t3.va 2 2 NULL NULL 200 200 3 3 30 30 NULL NULL +# Multiple `IS NOT DISTINCT FROM` conditions without parentheses. +# `IS [NOT] DISTINCT FROM` binds more tightly than `AND`, so the right operand of +# the first condition must not swallow the rest of the ON clause. +# https://github.com/apache/datafusion/issues/23692 +query IIIIII rowsort +SELECT t3.id AS t3_id, t4.id AS t4_id, t3.val1, t4.val1, t3.val2, t4.val2 +FROM t3 +JOIN t4 ON t3.val1 IS NOT DISTINCT FROM t4.val1 AND t3.val2 IS NOT DISTINCT FROM t4.val2 +---- +1 1 10 10 100 100 +2 2 NULL NULL 200 200 +3 3 30 30 NULL NULL + +# The unparenthesized form plans exactly like the parenthesized one +query TT +EXPLAIN SELECT t3.id AS t3_id, t4.id AS t4_id +FROM t3 +JOIN t4 ON t3.val1 IS NOT DISTINCT FROM t4.val1 AND t3.val2 IS NOT DISTINCT FROM t4.val2 +---- +logical_plan +01)Projection: t3.id AS t3_id, t4.id AS t4_id +02)--Inner Join: t3.val1 = t4.val1, t3.val2 = t4.val2 +03)----TableScan: t3 projection=[id, val1, val2] +04)----TableScan: t4 projection=[id, val1, val2] +physical_plan +01)ProjectionExec: expr=[id@0 as t3_id, id@1 as t4_id] +02)--HashJoinExec: mode=CollectLeft, join_type=Inner, on=[(val1@1, val1@1), (val2@2, val2@2)], projection=[id@0, id@3], NullsEqual: true +03)----DataSourceExec: partitions=1, partition_sizes=[1] +04)----DataSourceExec: partitions=1, partition_sizes=[1] + +# LEFT ANTI JOIN, as reported in the issue +query III rowsort +SELECT t3.id, t3.val1, t3.val2 +FROM t3 +LEFT ANTI JOIN t4 ON t3.val1 IS NOT DISTINCT FROM t4.val1 AND t3.val2 IS NOT DISTINCT FROM t4.val2 +---- + +# Three conditions +query III rowsort +SELECT t3.id, t3.val1, t3.val2 +FROM t3 +JOIN t4 ON t3.id IS NOT DISTINCT FROM t4.id + AND t3.val1 IS NOT DISTINCT FROM t4.val1 + AND t3.val2 IS NOT DISTINCT FROM t4.val2 +---- +1 10 100 +2 NULL 200 +3 30 NULL + +# `IS DISTINCT FROM` is affected the same way +query IIII rowsort +SELECT t3.id, t4.id, t3.val1, t4.val1 +FROM t3 +JOIN t4 ON t3.id IS NOT DISTINCT FROM t4.id AND t3.val1 IS DISTINCT FROM t4.val2 +---- +1 1 10 10 +2 2 NULL NULL +3 3 30 30 + +# `AND` binds more tightly than `OR` +query III rowsort +SELECT t3.id, t3.val1, t3.val2 +FROM t3 +WHERE t3.val1 IS NOT DISTINCT FROM 10 AND t3.val2 IS NOT DISTINCT FROM 100 + OR t3.val2 IS NOT DISTINCT FROM NULL +---- +1 10 100 +3 30 NULL + +query TT +EXPLAIN SELECT t3.id +FROM t3 +WHERE t3.val1 IS NOT DISTINCT FROM 10 AND t3.val2 IS NOT DISTINCT FROM 100 + OR t3.val2 IS NOT DISTINCT FROM NULL +---- +logical_plan +01)Projection: t3.id +02)--Filter: t3.val1 IS NOT DISTINCT FROM Int32(10) AND t3.val2 IS NOT DISTINCT FROM Int32(100) OR t3.val2 IS NOT DISTINCT FROM Int32(NULL) +03)----TableScan: t3 projection=[id, val1, val2] +physical_plan +01)FilterExec: val1@1 IS NOT DISTINCT FROM 10 AND val2@2 IS NOT DISTINCT FROM 100 OR val2@2 IS NOT DISTINCT FROM NULL, projection=[id@0] +02)--DataSourceExec: partitions=1, partition_sizes=[1] + +# `NOT` binds more tightly than `AND`, so it only negates the first condition +query III rowsort +SELECT t3.id, t3.val1, t3.val2 +FROM t3 +WHERE NOT t3.val1 IS NOT DISTINCT FROM 10 AND t3.val2 IS NOT DISTINCT FROM 200 +---- +2 NULL 200 + +# Explicit parentheses keep their grouping +query III rowsort +SELECT t3.id, t3.val1, t3.val2 +FROM t3 +WHERE t3.val1 IS NOT DISTINCT FROM 10 + AND (t3.val2 IS NOT DISTINCT FROM 999 OR t3.val2 IS NOT DISTINCT FROM 100) +---- +1 10 100 + statement ok drop table t0; From 9cc5eb3c0dfe936fe65f70fb32f8cfad66538a9c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 03:43:33 +0000 Subject: [PATCH 2/3] fix: make IS [NOT] DISTINCT FROM precedence fixup iterative `has_greedy_distinct_from` runs for every expression the planner sees, and walked the AND/OR spine recursively, putting chain depth back on the call stack in front of the stack machine that exists to keep it off (#1444). `recursive_protection` is not a default feature, so the attribute those helpers carried was not enough. Walk the spine with explicit work stacks in both helpers instead, and box the large variants of the two new local enums to match the neighbouring `StackEntry`. Adds test_stack_overflow_distinct_from_{1024,8192}, covering the fixup at the same spine depths the neighbouring test_stack_overflow tests use. Like those, it is a scale check rather than a proof: these frames are small enough that a recursive walk survives these depths too. The chain in that test is built from `=` terms after a single `IS NOT DISTINCT FROM` rather than from more `IS NOT DISTINCT FROM`: a chain of the latter nests in the AST instead of looping, so sqlparser overflows while parsing it, before any of this crate's code runs. --- datafusion/sql/src/expr/mod.rs | 223 ++++++++++++++++++++++++--------- 1 file changed, 167 insertions(+), 56 deletions(-) diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index a4c6c347e3f7..f4fa19285831 100644 --- a/datafusion/sql/src/expr/mod.rs +++ b/datafusion/sql/src/expr/mod.rs @@ -96,23 +96,36 @@ fn binary_op(left: SQLExpr, op: BinaryOperator, right: SQLExpr) -> SQLExpr { /// Returns `true` if `expr` contains an `IS [NOT] DISTINCT FROM` whose right /// operand swallowed a following `AND` / `OR`. /// +/// Walks the `AND` / `OR` spine iteratively. This runs for every expression the +/// planner sees, so its traversal belongs on the heap for the same reason the +/// stack machine in [`SqlToRel::sql_expr_to_logical_expr`] does: deep +/// `AND` / `OR` chains are common, and nothing here should put their depth back +/// on the call stack. +/// /// See [`fix_distinct_from_precedence`]. -#[cfg_attr(feature = "recursive_protection", recursive::recursive)] fn has_greedy_distinct_from(expr: &SQLExpr) -> bool { - match expr { - SQLExpr::BinaryOp { left, op, right } if is_and_or(op) => { - has_greedy_distinct_from(left) || has_greedy_distinct_from(right) - } - SQLExpr::IsDistinctFrom(_, right) | SQLExpr::IsNotDistinctFrom(_, right) => { - matches!(right.as_ref(), SQLExpr::BinaryOp { op, .. } if is_and_or(op)) - || has_greedy_distinct_from(right) + let mut stack = vec![expr]; + while let Some(expr) = stack.pop() { + match expr { + SQLExpr::BinaryOp { left, op, right } if is_and_or(op) => { + stack.push(left); + stack.push(right); + } + SQLExpr::IsDistinctFrom(_, right) | SQLExpr::IsNotDistinctFrom(_, right) => { + if matches!(right.as_ref(), SQLExpr::BinaryOp { op, .. } if is_and_or(op)) + { + return true; + } + stack.push(right); + } + SQLExpr::UnaryOp { + op: UnaryOperator::Not, + expr, + } => stack.push(expr), + _ => {} } - SQLExpr::UnaryOp { - op: UnaryOperator::Not, - expr, - } => has_greedy_distinct_from(expr), - _ => false, } + false } /// Restores the expected operator precedence around `IS [NOT] DISTINCT FROM`. @@ -150,60 +163,111 @@ fn has_greedy_distinct_from(expr: &SQLExpr) -> bool { /// Operands are not descended into; a parenthesised sub-expression is fixed when /// the planner recurses into it. fn fix_distinct_from_precedence(expr: SQLExpr) -> SQLExpr { - let (first, rest) = flatten_and_or(expr); - rebuild_and_or(first, rest) + let (operands, ops) = flatten_and_or(expr); + rebuild_and_or(operands, ops) } -/// Flattens the `AND` / `OR` spine of `expr` into its first operand followed by -/// the remaining `(operator, operand)` pairs in source order, moving whatever an -/// `IS [NOT] DISTINCT FROM` greedily absorbed back onto the spine. -#[cfg_attr(feature = "recursive_protection", recursive::recursive)] -fn flatten_and_or(expr: SQLExpr) -> (SQLExpr, Vec<(BinaryOperator, SQLExpr)>) { - match expr { - SQLExpr::BinaryOp { left, op, right } if is_and_or(&op) => { - let (first, mut rest) = flatten_and_or(*left); - let (right_first, right_rest) = flatten_and_or(*right); - rest.push((op, right_first)); - rest.extend(right_rest); - (first, rest) - } - // Only the first operand of the right hand side belongs to the - // comparison, the rest stays on the spine. - SQLExpr::IsDistinctFrom(left, right) => { - let (right_first, rest) = flatten_and_or(*right); - (distinct_from_expr(*left, right_first, false), rest) - } - SQLExpr::IsNotDistinctFrom(left, right) => { - let (right_first, rest) = flatten_and_or(*right); - (distinct_from_expr(*left, right_first, true), rest) - } - // `NOT` binds more tightly than `AND` / `OR` too, so it only negates the - // first operand of its operand's spine. - SQLExpr::UnaryOp { - op: op @ UnaryOperator::Not, - expr, - } => { - let (first, rest) = flatten_and_or(*expr); - ( - SQLExpr::UnaryOp { - op, - expr: Box::new(first), - }, - rest, - ) +/// An operator that the parser attached to the wrong operand, waiting to be +/// re-applied to the next operand emitted by [`flatten_and_or`]. +enum Postponed { + /// The left operand of an `IS [NOT] DISTINCT FROM` + DistinctFrom { left: Box, negated: bool }, + /// A prefix `NOT` + Not, +} + +/// Flattens the `AND` / `OR` spine of `expr` into its operands and the operators +/// separating them, both in source order, moving whatever an +/// `IS [NOT] DISTINCT FROM` or a `NOT` greedily absorbed back onto the spine. +/// +/// Always emits at least one operand, and exactly one more operand than +/// operators. Iterative for the same reason as [`has_greedy_distinct_from`]. +fn flatten_and_or(expr: SQLExpr) -> (Vec, Vec) { + enum Work { + Expr(Box), + Op(BinaryOperator), + } + + let mut work = vec![Work::Expr(Box::new(expr))]; + // Operators whose operand has not been reached yet. The last one pushed is + // the innermost, so they are applied in reverse. + let mut postponed: Vec = vec![]; + let mut operands = vec![]; + let mut ops = vec![]; + + while let Some(item) = work.pop() { + let expr = match item { + Work::Op(op) => { + ops.push(op); + continue; + } + Work::Expr(expr) => *expr, + }; + + match expr { + SQLExpr::BinaryOp { left, op, right } if is_and_or(&op) => { + // Pushed in reverse so that the left operand is visited first + work.push(Work::Expr(right)); + work.push(Work::Op(op)); + work.push(Work::Expr(left)); + } + // Only the first operand of the right hand side belongs to the + // comparison, the rest stays on the spine. + SQLExpr::IsDistinctFrom(left, right) => { + postponed.push(Postponed::DistinctFrom { + left, + negated: false, + }); + work.push(Work::Expr(right)); + } + SQLExpr::IsNotDistinctFrom(left, right) => { + postponed.push(Postponed::DistinctFrom { + left, + negated: true, + }); + work.push(Work::Expr(right)); + } + // `NOT` binds more tightly than `AND` / `OR` too, so it only negates + // the first operand of its operand's spine. + SQLExpr::UnaryOp { + op: UnaryOperator::Not, + expr, + } => { + postponed.push(Postponed::Not); + work.push(Work::Expr(expr)); + } + mut operand => { + for op in postponed.drain(..).rev() { + operand = match op { + Postponed::DistinctFrom { left, negated } => { + distinct_from_expr(*left, operand, negated) + } + Postponed::Not => SQLExpr::UnaryOp { + op: UnaryOperator::Not, + expr: Box::new(operand), + }, + }; + } + operands.push(operand); + } } - other => (other, vec![]), } + + (operands, ops) } /// Rebuilds the flattened spine produced by [`flatten_and_or`] with `AND` /// binding more tightly than `OR`, both left associative. -fn rebuild_and_or(first: SQLExpr, rest: Vec<(BinaryOperator, SQLExpr)>) -> SQLExpr { +fn rebuild_and_or(operands: Vec, ops: Vec) -> SQLExpr { + debug_assert_eq!(operands.len(), ops.len() + 1); + let mut operands = operands.into_iter(); // `AND` binds more tightly, so fold consecutive `AND`s into a group and // combine the completed groups with `OR` as they are closed. - let mut and_group = first; + let mut and_group = operands + .next() + .expect("flatten_and_or always emits at least one operand"); let mut or_expr: Option = None; - for (op, right) in rest { + for (op, right) in ops.into_iter().zip(operands) { if matches!(op, BinaryOperator::Or) { let completed = std::mem::replace(&mut and_group, right); or_expr = Some(match or_expr.take() { @@ -1731,6 +1795,53 @@ mod tests { test_stack_overflow!(test_stack_overflow_2048, 2048); test_stack_overflow!(test_stack_overflow_4096, 4096); test_stack_overflow!(test_stack_overflow_8192, 8192); + + /// A single `IS NOT DISTINCT FROM` followed by a long `OR` chain. + /// + /// The greedy parse pulls the whole chain into the right operand of the + /// comparison, so this covers the precedence fixup in + /// `sql_expr_to_logical_expr` at the same spine depths as + /// `test_stack_overflow` covers the stack machine it runs in front of. Like + /// those tests it is a scale check rather than a proof: the fixup walks the + /// spine iteratively so that its cost is heap rather than stack, but its + /// frames are small enough that a recursive walk would survive these depths + /// too. + /// + /// The chain deliberately uses `=` rather than more + /// `IS NOT DISTINCT FROM`: a chain of the latter nests in the AST instead of + /// looping, so `sqlparser` itself overflows while parsing it, well before + /// any of this crate's code runs. + macro_rules! test_stack_overflow_distinct_from { + ($name:ident, $num_expr:expr) => { + #[test] + fn $name() { + let schema = DFSchema::empty(); + let mut planner_context = PlannerContext::default(); + + let mut expr_str = "column1 IS NOT DISTINCT FROM 'value'".to_string(); + for i in 0..$num_expr { + expr_str.push_str(&format!(" OR column1 = 'value{:?}'", i)); + } + + let dialect = GenericDialect {}; + let mut parser = Parser::new(&dialect) + .try_with_sql(expr_str.as_str()) + .unwrap(); + let sql_expr = parser.parse_expr().unwrap(); + + let context_provider = TestContextProvider::new(); + let sql_to_rel = SqlToRel::new(&context_provider); + + // Should not stack overflow + sql_to_rel + .sql_expr_to_logical_expr(sql_expr, &schema, &mut planner_context) + .unwrap(); + } + }; + } + + test_stack_overflow_distinct_from!(test_stack_overflow_distinct_from_1024, 1024); + test_stack_overflow_distinct_from!(test_stack_overflow_distinct_from_8192, 8192); #[test] fn test_sql_to_expr_with_alias() { let schema = DFSchema::empty(); From 672dfc1584d9032d2d8d000e67fe539c8b9bfe00 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 04:01:37 +0000 Subject: [PATCH 3/3] test: cover the minimal IS NOT DISTINCT FROM precedence repro The issue's reproducer used a LEFT ANTI JOIN with two conditions, but neither the join nor the second condition is needed: a single `IS [NOT] DISTINCT FROM` followed by anything that binds less tightly is enough, so `SELECT 1 IS NOT DISTINCT FROM 1 AND true` fails the same way. Add that case next to the existing `IS DISTINCT FROM` tests in select.slt, along with the `OR` and `NOT` variants. The `NOT` and mixed `AND`/`OR` cases use values where a wrong grouping produces a different answer, since the plan display alone does not distinguish `NOT (A AND B)` from `(NOT A) AND B`. --- datafusion/sqllogictest/test_files/select.slt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/datafusion/sqllogictest/test_files/select.slt b/datafusion/sqllogictest/test_files/select.slt index 4107921d2fda..e7cc8b9dfb99 100644 --- a/datafusion/sqllogictest/test_files/select.slt +++ b/datafusion/sqllogictest/test_files/select.slt @@ -919,6 +919,22 @@ NULL is NOT DISTINCT FROM 1 as d, ---- false true true false true false false true +# `IS [NOT] DISTINCT FROM` binds more tightly than `AND` / `OR` / `NOT`, so the +# operators after it must not be absorbed into its right operand. +# https://github.com/apache/datafusion/issues/23692 +# +# `c` and `d` are chosen so that a wrong grouping gives a different answer: +# `NOT ((1 IS NOT DISTINCT FROM 2) AND false)` would be true, and +# `(1 IS NOT DISTINCT FROM 2) AND (true OR (3 IS NOT DISTINCT FROM 3))` false. +query BBBB +select +1 IS NOT DISTINCT FROM 1 AND true as a, +1 IS NOT DISTINCT FROM 2 OR true as b, +NOT 1 IS NOT DISTINCT FROM 2 AND false as c, +1 IS NOT DISTINCT FROM 2 AND true OR 3 IS NOT DISTINCT FROM 3 as d +---- +true true false true + # select distinct from utf8 query BBBB select