From 48e0f330e74047fc2e7c65cff156252557ce255b Mon Sep 17 00:00:00 2001 From: Amogh Ramesh Date: Sun, 30 Aug 2026 09:30:24 +0530 Subject: [PATCH] fix: stop treating arithmetic negation as bitwise NOT Expr::Negative represents unary minus, but these rules used bitwise-complement identities. Remove the unsound rewrites while preserving the existing exact double-negation simplification. Signed-off-by: Amogh Ramesh --- .../simplify_expressions/expr_simplifier.rs | 149 +++--------------- .../src/simplify_expressions/utils.rs | 46 +----- datafusion/sqllogictest/test_files/scalar.slt | 15 ++ 3 files changed, 39 insertions(+), 171 deletions(-) diff --git a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs index ee0997d9aeeb0..6d75eb4352bb4 100644 --- a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs +++ b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs @@ -1267,30 +1267,6 @@ impl TreeNodeRewriter for Simplifier<'_> { right, }) if !info.nullable(&right)? && is_zero(&left) => Transformed::yes(*left), - // !A & A -> 0 (if A not nullable) - Expr::BinaryExpr(BinaryExpr { - left, - op: BitwiseAnd, - right, - }) if is_negative_of(&left, &right) && !info.nullable(&right)? => { - Transformed::yes(Expr::Literal( - ScalarValue::new_zero(&info.get_data_type(&left)?)?, - None, - )) - } - - // A & !A -> 0 (if A not nullable) - Expr::BinaryExpr(BinaryExpr { - left, - op: BitwiseAnd, - right, - }) if is_negative_of(&right, &left) && !info.nullable(&left)? => { - Transformed::yes(Expr::Literal( - ScalarValue::new_zero(&info.get_data_type(&left)?)?, - None, - )) - } - // (..A..) & A --> (..A..) Expr::BinaryExpr(BinaryExpr { left, @@ -1341,30 +1317,6 @@ impl TreeNodeRewriter for Simplifier<'_> { right, }) if is_zero(&left) => Transformed::yes(*right), - // !A | A -> -1 (if A not nullable) - Expr::BinaryExpr(BinaryExpr { - left, - op: BitwiseOr, - right, - }) if is_negative_of(&left, &right) && !info.nullable(&right)? => { - Transformed::yes(Expr::Literal( - ScalarValue::new_negative_one(&info.get_data_type(&left)?)?, - None, - )) - } - - // A | !A -> -1 (if A not nullable) - Expr::BinaryExpr(BinaryExpr { - left, - op: BitwiseOr, - right, - }) if is_negative_of(&right, &left) && !info.nullable(&left)? => { - Transformed::yes(Expr::Literal( - ScalarValue::new_negative_one(&info.get_data_type(&left)?)?, - None, - )) - } - // (..A..) | A --> (..A..) Expr::BinaryExpr(BinaryExpr { left, @@ -1415,30 +1367,6 @@ impl TreeNodeRewriter for Simplifier<'_> { right, }) if !info.nullable(&right)? && is_zero(&left) => Transformed::yes(*right), - // !A ^ A -> -1 (if A not nullable) - Expr::BinaryExpr(BinaryExpr { - left, - op: BitwiseXor, - right, - }) if is_negative_of(&left, &right) && !info.nullable(&right)? => { - Transformed::yes(Expr::Literal( - ScalarValue::new_negative_one(&info.get_data_type(&left)?)?, - None, - )) - } - - // A ^ !A -> -1 (if A not nullable) - Expr::BinaryExpr(BinaryExpr { - left, - op: BitwiseXor, - right, - }) if is_negative_of(&right, &left) && !info.nullable(&left)? => { - Transformed::yes(Expr::Literal( - ScalarValue::new_negative_one(&info.get_data_type(&left)?)?, - None, - )) - } - // (..A..) ^ A --> (the expression without A, if number of A is odd, otherwise one A) Expr::BinaryExpr(BinaryExpr { left, @@ -1503,7 +1431,12 @@ impl TreeNodeRewriter for Simplifier<'_> { // // Rules for Negative // - Expr::Negative(inner) => Transformed::yes(distribute_negation(*inner)), + // Preserve the existing double-negation simplification without + // applying bitwise-complement identities to arithmetic negation. + Expr::Negative(inner) => match *inner { + Expr::Negative(inner) => Transformed::yes(*inner), + inner => Transformed::no(Expr::Negative(Box::new(inner))), + }, // // Rules for Case @@ -3449,47 +3382,25 @@ mod tests { } #[test] - fn test_simplify_negated_bitwise_and() { - // !c3 & c3 --> 0 - let expr = (-col("c3_non_null")) & col("c3_non_null"); - let expected = lit(0i64); - - assert_eq!(simplify(expr), expected); - // c3 & !c3 --> 0 - let expr = col("c3_non_null") & (-col("c3_non_null")); - let expected = lit(0i64); - - assert_eq!(simplify(expr), expected); - } - - #[test] - fn test_simplify_negated_bitwise_or() { - // !c3 | c3 --> -1 - let expr = (-col("c3_non_null")) | col("c3_non_null"); - let expected = lit(-1i64); - - assert_eq!(simplify(expr), expected); - - // c3 | !c3 --> -1 - let expr = col("c3_non_null") | (-col("c3_non_null")); - let expected = lit(-1i64); - - assert_eq!(simplify(expr), expected); - } - - #[test] - fn test_simplify_negated_bitwise_xor() { - // !c3 ^ c3 --> -1 - let expr = (-col("c3_non_null")) ^ col("c3_non_null"); - let expected = lit(-1i64); - - assert_eq!(simplify(expr), expected); + fn test_preserve_arithmetic_negation() { + let c3 = col("c3_non_null"); + let expressions = [ + (-c3.clone()) & c3.clone(), + c3.clone() & (-c3.clone()), + (-c3.clone()) | c3.clone(), + c3.clone() | (-c3.clone()), + (-c3.clone()) ^ c3.clone(), + c3.clone() ^ (-c3.clone()), + -bitwise_and(col("c3"), c3.clone()), + -bitwise_or(col("c3"), c3.clone()), + ]; - // c3 ^ !c3 --> -1 - let expr = col("c3_non_null") ^ (-col("c3_non_null")); - let expected = lit(-1i64); + for expr in expressions { + assert_eq!(simplify(expr.clone()), expr); + } - assert_eq!(simplify(expr), expected); + // Keep the pre-existing arithmetic double-negation simplification. + assert_eq!(simplify(-(-c3.clone())), c3); } #[test] @@ -3674,20 +3585,6 @@ mod tests { let expr = col("c3").not().not(); let expected = col("c3"); assert_eq!(simplify(expr), expected); - - // Laws with bitwise operations - // !(c3 & c4) --> !c3 | !c4 - let expr = -bitwise_and(col("c3"), col("c4")); - let expected = bitwise_or(-col("c3"), -col("c4")); - assert_eq!(simplify(expr), expected); - // !(c3 | c4) --> !c3 & !c4 - let expr = -bitwise_or(col("c3"), col("c4")); - let expected = bitwise_and(-col("c3"), -col("c4")); - assert_eq!(simplify(expr), expected); - // !(!c3) --> c3 - let expr = -(-col("c3")); - let expected = col("c3"); - assert_eq!(simplify(expr), expected); } #[test] diff --git a/datafusion/optimizer/src/simplify_expressions/utils.rs b/datafusion/optimizer/src/simplify_expressions/utils.rs index 3f29e8c5214c3..34ead9a69deac 100644 --- a/datafusion/optimizer/src/simplify_expressions/utils.rs +++ b/datafusion/optimizer/src/simplify_expressions/utils.rs @@ -23,7 +23,7 @@ use datafusion_common::{Result, ScalarValue, internal_err}; use datafusion_expr::{ Case, Expr, Like, Operator, expr::{Between, BinaryExpr, InList}, - expr_fn::{and, bitwise_and, bitwise_or, or}, + expr_fn::{and, or}, }; /// returns true if `needle` is found in a chain of search_op @@ -194,11 +194,6 @@ pub fn is_not_of(not_expr: &Expr, expr: &Expr) -> bool { matches!(not_expr, Expr::Not(inner) if expr == inner.as_ref()) } -/// returns true if `not_expr` is !`expr` (bitwise not) -pub fn is_negative_of(not_expr: &Expr, expr: &Expr) -> bool { - matches!(not_expr, Expr::Negative(inner) if expr == inner.as_ref()) -} - /// returns the contained boolean value in `expr` as /// `Expr::Literal(ScalarValue::Boolean(v))`. pub fn as_bool_lit(expr: &Expr) -> Result> { @@ -370,45 +365,6 @@ pub fn negate_clause(expr: Expr) -> Expr { } } -/// bitwise negate a Negative clause -/// input is the clause to be bitwise negated.(args for Negative clause) -/// For BinaryExpr: -/// ~(A & B) ===> ~A | ~B -/// ~(A | B) ===> ~A & ~B -/// For Negative: -/// ~(~A) ===> A -/// For others, use Negative clause -pub fn distribute_negation(expr: Expr) -> Expr { - match expr { - Expr::BinaryExpr(BinaryExpr { left, op, right }) => { - match op { - // ~(A & B) ===> ~A | ~B - Operator::BitwiseAnd => { - let left = distribute_negation(*left); - let right = distribute_negation(*right); - - bitwise_or(left, right) - } - // ~(A | B) ===> ~A & ~B - Operator::BitwiseOr => { - let left = distribute_negation(*left); - let right = distribute_negation(*right); - - bitwise_and(left, right) - } - // use negative clause - _ => Expr::Negative(Box::new(Expr::BinaryExpr(BinaryExpr::new( - left, op, right, - )))), - } - } - // ~(~A) ===> A - Expr::Negative(expr) => *expr, - // use negative clause - _ => Expr::Negative(Box::new(expr)), - } -} - #[cfg(test)] mod tests { use super::{is_one, is_zero}; diff --git a/datafusion/sqllogictest/test_files/scalar.slt b/datafusion/sqllogictest/test_files/scalar.slt index 35705a12a9622..b5a5553db1cb7 100644 --- a/datafusion/sqllogictest/test_files/scalar.slt +++ b/datafusion/sqllogictest/test_files/scalar.slt @@ -1479,6 +1479,21 @@ from (values (NULL::INT, 7), (3, 7)) as t(a, b); 7 7 0 NULL NULL NULL +# arithmetic negation is not bitwise NOT +query IIIIII +select + i, + (-i) & i, + i | (-i), + (-i) ^ i, + -(i & j), + -(i | j) +from (values (5, 3), (6, 3)) as t(i, j) +order by i; +---- +5 1 -1 -2 -1 -7 +6 2 -2 -4 -2 -7 + # bitwise xor with other operators query II rowsort select 2 * c - 1 ^ 856 + d + 3, d ^ 7 >> 4 from signed_integers;