Skip to content

fix: stop treating arithmetic negation as bitwise NOT #24668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Amogh-2404 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: stop treating arithmetic negation as bitwise NOT #24668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cfcd150
fix: stop treating arithmetic negation as bitwise NOT
Amogh-2404 Aug 25, 2026
c37e666
test: cover arithmetic negation in bitwise expressions
Amogh-2404 Aug 25, 2026
4e9d57f
fix: handle preserved arithmetic negation
Amogh-2404 Aug 25, 2026
1b7d5f9
fix: keep negation bounds conservative
Amogh-2404 Aug 26, 2026
00b519c
fix: align scalar and array integer negation
Amogh-2404 Aug 26, 2026
69af506
fix: preserve wrapping negation semantics
Amogh-2404 Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions datafusion/core/tests/sql/unparser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,34 @@ use datafusion_sql::unparser::dialect::{DefaultDialect, DuckDBDialect};
use itertools::Itertools;
use recursive::{set_minimum_stack_size, set_stack_allocation_size};

#[tokio::test]
async fn optimized_unparse_parenthesizes_nested_negation() -> Result<()> {
let ctx = SessionContext::new();
ctx.register_table(
"t",
Arc::new(EmptyTable::new(Arc::new(Schema::new(vec![Field::new(
"ts",
DataType::Timestamp(arrow::datatypes::TimeUnit::Nanosecond, None),
true,
)])))),
)?;
let plan = ctx
.sql("SELECT -(-ts) AS x FROM t")
.await?
.into_optimized_plan()?;

for pretty in [false, true] {
let sql = Unparser::default()
.with_pretty(pretty)
.plan_to_sql(&plan)?
.to_string();
assert_eq!(sql, "SELECT -(-t.ts) AS x FROM t");
ctx.sql(&sql).await?.into_optimized_plan()?;
}

Ok(())
}

/// Paths to benchmark query files (supports running from repo root or different working directories).
const BENCHMARK_PATHS: &[&str] = &["../../benchmarks/", "./benchmarks/"];

Expand Down
222 changes: 98 additions & 124 deletions datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ use datafusion_expr::physical_planning_context::PhysicalPlanningContext;
use datafusion_expr::{
BinaryExpr, Case, ColumnarValue, Expr, ExprSchemable, Like, Operator, Volatility,
and, binary::BinaryTypeCoercer, lit, or, preimage::PreimageResult,
type_coercion::is_signed_numeric,
};
use datafusion_expr::{Cast, TryCast, simplify::ExprSimplifyResult};
use datafusion_expr::{expr::ScalarFunction, interval_arithmetic::NullableInterval};
Expand Down Expand Up @@ -760,6 +761,10 @@ struct Simplifier<'a> {
info: &'a SimplifyContext,
}

fn can_cancel_double_negation(data_type: &DataType) -> bool {
data_type.is_null() || is_signed_numeric(data_type)
}

impl<'a> Simplifier<'a> {
pub fn new(info: &'a SimplifyContext) -> Self {
Self { info }
Expand Down Expand Up @@ -1178,30 +1183,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,
Expand Down Expand Up @@ -1252,30 +1233,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,
Expand Down Expand Up @@ -1326,30 +1283,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,
Expand Down Expand Up @@ -1411,10 +1344,21 @@ impl TreeNodeRewriter for Simplifier<'_> {
//
Expr::Not(inner) => Transformed::yes(negate_clause(*inner)),

//
// Rules for Negative
//
Expr::Negative(inner) => Transformed::yes(distribute_negation(*inner)),

@sunchao sunchao Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve timestamp-column support when retaining double negation

Removing this arm also stops cancelling -(-timestamp_column). I reproduced SELECT -(-ts) AS x FROM t using a registered TimestampNanosecondArray containing 2020-01-01 and 2020-01-02: base 63f5b55f returns both timestamps, while head c37e6666 fails with Invalid arithmetic operation: !Timestamp(ns). SQL analysis and physical planning explicitly accept timestamp negation, but NegativeExpr::evaluate passes arrays to Arrow's neg_wrapping, whose fallback does not support timestamp arrays. Timestamp literals still work through the separate scalar implementation.

The single-negation kernel gap already existed, but retaining both nodes newly breaks these previously working double-negation queries. Could we add timestamp-array negation support and a column-based execution regression test alongside this change, while preserving the intended overflow behavior?

@Amogh-2404 Amogh-2404 Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4e9d57f3c. NegativeExpr now uses checked negation for timestamp arrays in seconds, milliseconds, microseconds, and nanoseconds. It preserves timezone metadata and nulls, and reports overflow for i64::MIN. I added unit coverage for each case and an end-to-end timestamp-column regression.

@sunchao sunchao Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve integer double-negation ordering equivalence

Removing this arm preserves -(-i), whose properties no longer establish the full ordering of an input sorted by (i, j). On an unbounded stream with constant i = 0 and nondecreasing j, I reproduced:

SELECT i, j FROM fixed_prefix_stream
ORDER BY -(-i) ASC NULLS LAST, j ASC NULLS LAST
LIMIT 1;

Base 63f5b55f uses StreamingTableExec with fetch=1 and returns (0, 0) immediately. Head 00b519c3a inserts PartialSortExec: TopK(fetch=1), common_prefix_length=[1] and times out. The partial sort waits for i to change, so a permanently fixed prefix never emits its first row. Both direct ORDER BY i, j LIMIT 1 and one-key ORDER BY -(-i) LIMIT 1 still return immediately on head.

Could we restore cancellation specifically for signed integers, where wrapping makes it safe even at MIN, or preserve equivalent ordering metadata? Extending the new streaming regression to two sort keys would cover this without changing checked timestamp/interval behavior.

@Amogh-2404 Amogh-2404 Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 69af50622. Double-negation cancellation is now type-aware: signed integers, floating-point values, valid decimals, and NULL cancel, while timestamp and interval negation remains explicit. This restores the full ordering equivalence for integer -(-i). The unbounded regression now includes the reported two-key fixed-prefix plan and keeps fetch=1 on StreamingTableExec without a partial sort.

// -(-A) -> A for types where negation is total and involutive.
// Signed integer negation wraps, floating-point negation is total,
// and valid decimal precision excludes the native signed minimum.
// Timestamp and interval negation remains explicit because it can
// overflow.
Expr::Negative(inner) => match *inner {
Expr::Negative(inner)
if can_cancel_double_negation(
&info.get_data_type(inner.as_ref())?,
) =>
{
Transformed::yes(*inner)
}
inner => Transformed::no(Expr::Negative(Box::new(inner))),
},

//
// Rules for Case
Expand Down Expand Up @@ -3115,47 +3059,91 @@ 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);
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()),
];

assert_eq!(simplify(expr), expected);
for expr in expressions {
assert_eq!(simplify(expr.clone()), expr);
}

// c3 | !c3 --> -1
let expr = col("c3_non_null") | (-col("c3_non_null"));
let expected = lit(-1i64);
// Timestamp and interval negation are checked, so the inner expression
// can overflow and must remain visible.
let checked_types = [
(
"ts",
DataType::Timestamp(arrow::datatypes::TimeUnit::Nanosecond, None),
),
(
"year_month",
DataType::Interval(arrow::datatypes::IntervalUnit::YearMonth),
),
(
"day_time",
DataType::Interval(arrow::datatypes::IntervalUnit::DayTime),
),
(
"month_day_nano",
DataType::Interval(arrow::datatypes::IntervalUnit::MonthDayNano),
),
];
let schema = Schema::new(
checked_types
.iter()
.map(|(name, data_type)| Field::new(*name, data_type.clone(), false))
.collect::<Vec<_>>(),
)
.to_dfschema_ref()
.unwrap();
let simplifier =
ExprSimplifier::new(SimplifyContext::builder().with_schema(schema).build());

assert_eq!(simplify(expr), expected);
for (name, _) in checked_types {
let expr = -(-col(name));
assert_eq!(simplifier.simplify(expr.clone()).unwrap(), expr);
}
}

#[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_cancel_safe_double_negation() {
let safe_types = [
("i8", DataType::Int8),
("i16", DataType::Int16),
("i32", DataType::Int32),
("i64", DataType::Int64),
("f16", DataType::Float16),
("f32", DataType::Float32),
("f64", DataType::Float64),
("d32", DataType::Decimal32(9, 0)),
("d64", DataType::Decimal64(18, 0)),
("d128", DataType::Decimal128(38, 0)),
("d256", DataType::Decimal256(76, 0)),
("null", DataType::Null),
];
let schema = Schema::new(
safe_types
.iter()
.map(|(name, data_type)| Field::new(*name, data_type.clone(), true))
.collect::<Vec<_>>(),
)
.to_dfschema_ref()
.unwrap();
let simplifier =
ExprSimplifier::new(SimplifyContext::builder().with_schema(schema).build());

assert_eq!(simplify(expr), expected);
for (name, _) in safe_types {
assert_eq!(simplifier.simplify(-(-col(name))).unwrap(), col(name));
assert_eq!(simplifier.simplify(-(-(-col(name)))).unwrap(), -col(name));
}
}

#[test]
Expand Down Expand Up @@ -3340,20 +3328,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]
Expand Down
46 changes: 1 addition & 45 deletions datafusion/optimizer/src/simplify_expressions/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Option<bool>> {
Expand Down Expand Up @@ -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};
Expand Down
Loading