From cfcd150fbe7ad4fab43500f525bd2b151241590f Mon Sep 17 00:00:00 2001 From: Amogh Ramesh Date: Tue, 25 Aug 2026 19:04:19 +0530 Subject: [PATCH 1/2] fix: stop treating arithmetic negation as bitwise NOT `Expr::Negative` represents unary minus, but these rules used bitwise-NOT identities. Remove the unsound rewrites and leave arithmetic negation unchanged. Closes #24665 Signed-off-by: Amogh Ramesh --- .../simplify_expressions/expr_simplifier.rs | 148 ++---------------- .../src/simplify_expressions/utils.rs | 46 +----- 2 files changed, 18 insertions(+), 176 deletions(-) diff --git a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs index dadea4784802a..88658abe16a9c 100644 --- a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs +++ b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs @@ -1178,30 +1178,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, @@ -1252,30 +1228,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, @@ -1326,30 +1278,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, @@ -1411,11 +1339,6 @@ impl TreeNodeRewriter for Simplifier<'_> { // Expr::Not(inner) => Transformed::yes(negate_clause(*inner)), - // - // Rules for Negative - // - Expr::Negative(inner) => Transformed::yes(distribute_negation(*inner)), - // // Rules for Case // @@ -3115,47 +3038,24 @@ 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); - - // c3 ^ !c3 --> -1 - let expr = col("c3_non_null") ^ (-col("c3_non_null")); - let expected = lit(-1i64); + 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()), + // The inner negation can overflow for the signed minimum. + -(-c3), + ]; - assert_eq!(simplify(expr), expected); + for expr in expressions { + assert_eq!(simplify(expr.clone()), expr); + } } #[test] @@ -3340,20 +3240,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 89bb762d59ce2..dec0cfa8c1772 100644 --- a/datafusion/optimizer/src/simplify_expressions/utils.rs +++ b/datafusion/optimizer/src/simplify_expressions/utils.rs @@ -21,7 +21,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 @@ -181,11 +181,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> { @@ -343,45 +338,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}; From c37e66669e12c2d9907f5305173d50c1d9072888 Mon Sep 17 00:00:00 2001 From: Amogh Ramesh Date: Tue, 25 Aug 2026 19:04:56 +0530 Subject: [PATCH 2/2] test: cover arithmetic negation in bitwise expressions Signed-off-by: Amogh Ramesh --- datafusion/sqllogictest/test_files/scalar.slt | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/datafusion/sqllogictest/test_files/scalar.slt b/datafusion/sqllogictest/test_files/scalar.slt index 175607b62089f..fb8f8399aa849 100644 --- a/datafusion/sqllogictest/test_files/scalar.slt +++ b/datafusion/sqllogictest/test_files/scalar.slt @@ -1455,6 +1455,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;