diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index c2e4822f76b99..f4fa192858314 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,221 @@ 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`. +/// +/// 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`]. +fn has_greedy_distinct_from(expr: &SQLExpr) -> bool { + 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), + _ => {} + } + } + 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 (operands, ops) = flatten_and_or(expr); + rebuild_and_or(operands, ops) +} + +/// 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); + } + } + } + + (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(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 = operands + .next() + .expect("flatten_and_or always emits at least one operand"); + let mut or_expr: Option = None; + 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() { + 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 +371,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), @@ -1572,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(); diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs index a4bf0db910774..70eff6fad515b 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 1b6f2e4c86385..573fe01c102ee 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; diff --git a/datafusion/sqllogictest/test_files/select.slt b/datafusion/sqllogictest/test_files/select.slt index 4107921d2fda5..e7cc8b9dfb990 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