From 04acb8d40b2007ddcf33ae33db8ff714d601b60a Mon Sep 17 00:00:00 2001 From: Smallfu666 Date: Mon, 10 Aug 2026 23:28:16 +0800 Subject: [PATCH 1/3] fix(physical-expr): stop false infeasibility report from Eq+FALSE in cp_solver propagate_comparison for Operator::Eq with parent == Interval::FALSE returned Ok(None), which ExprIntervalGraph::propagate_constraints interprets as infeasible. The correct semantics is that a = b being certainly false means a != b, which excludes at most a single point from each operand. Interval arithmetic cannot represent a hole, so the children cannot be refined further, but the expression is not infeasible. This caused analyze() to return None for the column interval when the predicate was NOT (a = 0.0) and the input domain contained 0.0 but was not equal to it (e.g. a in [-1, 1]). The None is documented in ExprBoundaries::interval to mean "evaluating the given column results in an empty set", but NOT (a = 0.0) is true for every a != 0, so the correct result is the full input domain. This is a contract violation of the public analyze() API, as reported in #19264 by an external user calling analyze() to infer bounds for pushdown into another library. Eq + FALSE is infeasible when equality is provably true for the two singleton operands under the comparison semantics. This must use comparison-semantics equality, not structural ScalarValue::PartialEq: the latter is bit-wise for floats (to_bits), under which -0.0 != +0.0 even though SQL/IEEE-754 comparison treats them as equal. The guard uses singleton_values_equal, which normalizes signed zero via normalize_float_zero_scalar (the same normalization DataFusion applies in physical-expr-common/src/datum.rs for runtime comparison) before structural equality. This matters in nested boolean contexts (e.g. NOT(a = +0.0) AND b) where the bottom-up pass does not short-circuit and top-down propagation forces Eq to FALSE, reaching this arm with [-0.0,-0.0] vs [+0.0,+0.0]. This revives the fix from #20138, which was closed as stale after review. The singleton guard was agreed on during #20138 review by berkaysynnada and pepijnve; this version strengthens it to use comparison-semantics equality. Direct unit tests isolate the ordinary identical-singleton case that the [0,0] analyze() test cannot reach because update_ranges short-circuits. The nested signed-zero E2E test separately proves that analyze() can reach the production arm through the pre-existing interval/runtime equality divergence. Tests: - analysis.rs: E2E tests via analyze(). Input [-1,1] is the reported case and does reach the fixed branch; input [0,0] is the nearest-invalid counterpart and must stay infeasible; the nested NOT(a = +0.0) AND b case proves the case-(1) short-circuit does not protect the production arm and exercises the signed-zero guard. - cp_solver.rs: direct propagate_comparison unit tests for Eq + FALSE covering identical singletons (infeasible), signed-zero singletons (infeasible under comparison semantics), distinct singletons (feasible), and overlapping intervals including the [-1,1] vs [0,0] case from the issue. Negative control on baseline 6eaca8bfe with the production arm reverted to Ok(None): 3 of the 7 new tests fail. The 4 that pass on baseline are boundary guards (baseline returns None for every Eq + FALSE input, so it agrees by accident). A second, independent control covers those guards: replacing singleton_values_equal with structural == makes both signed-zero tests fail, confirming they exercise the comparison-semantics guard rather than passing vacuously. Closes #19264. Co-authored-by: evangelisilva --- datafusion/physical-expr/src/analysis.rs | 133 +++++++++++++++++ .../physical-expr/src/intervals/cp_solver.rs | 135 +++++++++++++++++- 2 files changed, 265 insertions(+), 3 deletions(-) diff --git a/datafusion/physical-expr/src/analysis.rs b/datafusion/physical-expr/src/analysis.rs index a00fc19ae9c02..8ca9ad7fee7a6 100644 --- a/datafusion/physical-expr/src/analysis.rs +++ b/datafusion/physical-expr/src/analysis.rs @@ -344,6 +344,7 @@ fn calculate_selectivity( #[cfg(test)] mod tests { + use std::ops::Not; use std::sync::Arc; use arrow::datatypes::{DataType, Field, Schema}; @@ -589,4 +590,136 @@ mod tests { let _ = selectivity; // silence unused warning } + + // Regression tests for #19264. + // + // `analyze` propagates constraints through `NotExpr::propagate_constraints`, + // which hands the underlying `Eq` node a `FALSE` parent interval. When the + // input domain contains `0.0` but is not equal to it, `NOT (a = 0.0)` is + // satisfiable, so `analyze` reports the input domain. `None` is reserved + // for an empty set, as documented on `ExprBoundaries::interval`. + fn analyze_not_eq_helper( + lower: f32, + upper: f32, + ) -> datafusion_common::Result> { + let schema = Arc::new(Schema::new(vec![make_field("a", DataType::Float32)])); + let df_schema = DFSchema::try_from(Arc::clone(&schema)).unwrap(); + + // Input column bounds: [lower, upper] + let boundaries = vec![ExprBoundaries { + column: Column::new("a", 0), + interval: Some(Interval::try_new( + ScalarValue::Float32(Some(lower)), + ScalarValue::Float32(Some(upper)), + )?), + distinct_count: Precision::Inexact(100), + }]; + let ctx = AnalysisContext::new(boundaries); + + // Predicate: NOT (a = 0.0) + let pred_not_eq = col("a").eq(lit(0.0f32)).not(); + let phys_not = create_physical_expr( + &pred_not_eq, + &df_schema, + &ExecutionProps::new(), + &PhysicalPlanningContext::default(), + )?; + let out = analyze(&phys_not, ctx, df_schema.as_ref())?; + Ok(out.boundaries[0].interval.clone()) + } + + #[test] + fn test_analyze_not_eq_around_zero() -> datafusion_common::Result<()> { + // Input domain [-1, 1] contains 0.0. + // `NOT (a = 0.0)` can be true everywhere except at 0.0, so the output + // interval should be the full input domain [-1, 1], NOT None. + let out = analyze_not_eq_helper(-1.0, 1.0)?; + let expected = Some(Interval::try_new( + ScalarValue::Float32(Some(-1.0)), + ScalarValue::Float32(Some(1.0)), + )?); + assert_eq!( + out, expected, + "NOT (a = 0.0) over [-1, 1] should yield [-1, 1], got {out:?}" + ); + Ok(()) + } + + #[test] + fn test_analyze_not_eq_clamped_at_zero() -> datafusion_common::Result<()> { + // Input domain [0, 0] is exactly the point where a = 0.0 is true. + // `NOT (a = 0.0)` is always false here, so the output interval should be None. + let out = analyze_not_eq_helper(0.0, 0.0)?; + assert_eq!( + out, None, + "NOT (a = 0.0) over [0, 0] should yield None (infeasible), got {out:?}" + ); + Ok(()) + } + + // Regression test for the nested signed-zero case. + // + // `NOT(a = +0.0) AND b` with `a ∈ [-0.0,-0.0]`, `b ∈ [false,true]`: + // `a = +0.0` is TRUE (IEEE 754), so the whole predicate is infeasible. + // + // This defeats the case-(1) short-circuit in `update_ranges`: bottom-up + // evaluates `a = +0.0` over `[-0.0,-0.0]` as FALSE (structural equality), + // so `NOT` becomes TRUE, the root becomes `TRUE_OR_FALSE`, and top-down + // propagation forces `a = +0.0` to FALSE, reaching + // `propagate_comparison(Eq, FALSE, [-0.0,-0.0], [+0.0,+0.0])`. The guard + // must normalize signed zero to report infeasible here. + #[test] + fn test_analyze_not_eq_nested_signed_zero_infeasible() -> datafusion_common::Result<()> + { + let schema = Arc::new(Schema::new(vec![ + make_field("a", DataType::Float32), + make_field("b", DataType::Boolean), + ])); + let df_schema = DFSchema::try_from(Arc::clone(&schema)).unwrap(); + + let boundaries = vec![ + ExprBoundaries { + column: Column::new("a", 0), + interval: Some(Interval::try_new( + ScalarValue::Float32(Some(-0.0f32)), + ScalarValue::Float32(Some(-0.0f32)), + )?), + distinct_count: Precision::Inexact(1), + }, + ExprBoundaries { + column: Column::new("b", 1), + interval: Some(Interval::try_new( + ScalarValue::Boolean(Some(false)), + ScalarValue::Boolean(Some(true)), + )?), + distinct_count: Precision::Inexact(2), + }, + ]; + let ctx = AnalysisContext::new(boundaries); + + // NOT(a = +0.0) AND b + let pred = col("a").eq(lit(0.0f32)).not().and(col("b")); + let phys = create_physical_expr( + &pred, + &df_schema, + &ExecutionProps::new(), + &PhysicalPlanningContext::default(), + )?; + let out = analyze(&phys, ctx, df_schema.as_ref())?; + + // Infeasible: a = +0.0 is TRUE (signed zero), NOT is FALSE, + // FALSE AND b = FALSE. All column intervals must be None. + assert!( + out.boundaries.iter().all(|b| b.interval.is_none()), + "NOT(a = +0.0) AND b over a=[-0.0,-0.0] should be infeasible (all boundaries None), got {:?}", + out.boundaries + ); + assert_eq!( + out.selectivity, + Some(0.0), + "selectivity should be 0.0 for an infeasible predicate, got {:?}", + out.selectivity + ); + Ok(()) + } } diff --git a/datafusion/physical-expr/src/intervals/cp_solver.rs b/datafusion/physical-expr/src/intervals/cp_solver.rs index aee65f35dc49c..3b1a5a042ea4d 100644 --- a/datafusion/physical-expr/src/intervals/cp_solver.rs +++ b/datafusion/physical-expr/src/intervals/cp_solver.rs @@ -153,7 +153,7 @@ use crate::expressions::{BinaryExpr, Literal}; use crate::utils::{ExprTreeNode, build_dag}; use arrow::datatypes::{DataType, Schema}; -use datafusion_common::{Result, internal_err, not_impl_err}; +use datafusion_common::{Result, ScalarValue, internal_err, not_impl_err}; use datafusion_expr::Operator; use datafusion_expr::interval_arithmetic::{Interval, apply_operator, satisfy_greater}; @@ -296,6 +296,17 @@ pub fn propagate_arithmetic( } } +/// Match runtime `Eq`'s signed-zero semantics before using `ScalarValue` +/// equality for singleton operands. `ScalarValue::PartialEq` is bit-wise for +/// floats (`to_bits`), under which `-0.0 != +0.0`; runtime `Eq` normalizes +/// them via `normalize_float_zero_scalar` (see +/// `physical-expr-common/src/datum.rs`). +fn singleton_values_equal(left: &ScalarValue, right: &ScalarValue) -> bool { + use datafusion_common::utils::normalize_float_zero_scalar; + normalize_float_zero_scalar(left.clone()) + == normalize_float_zero_scalar(right.clone()) +} + /// This function refines intervals `left_child` and `right_child` by applying /// comparison propagation through `parent` via operation. The main idea is /// that we can shrink ranges of variables x and y using parent interval p. @@ -363,8 +374,29 @@ pub fn propagate_comparison( } else if parent == &Interval::FALSE { match op { Operator::Eq => { - // TODO: Propagation is not possible until we support interval sets. - Ok(None) + // `a = b` being certainly false means `a != b`, which excludes + // at most a single point from each child. A single interval + // cannot represent that excluded point, so returning the + // children unchanged is a safe over-approximation. Returning + // `None` is not: the caller reads it as infeasible, which + // discards satisfiable ranges (see issue #19264). + // + // The exception is when both children are singletons that are + // equal under the `Eq` operator's comparison semantics: then + // `a = b` is certainly true, so `NOT(a = b)` is infeasible. + // `ScalarValue::PartialEq` is bit-wise for floats, so + // `singleton_values_equal` normalizes signed zero to match + // runtime `Eq` behavior before comparing. + if !left_child.is_unbounded() + && !right_child.is_unbounded() + && left_child.lower() == left_child.upper() + && right_child.lower() == right_child.upper() + && singleton_values_equal(left_child.lower(), right_child.lower()) + { + Ok(None) + } else { + Ok(Some((left_child.clone(), right_child.clone()))) + } } Operator::Gt => satisfy_greater(right_child, left_child, false), Operator::GtEq => satisfy_greater(right_child, left_child, true), @@ -1621,6 +1653,103 @@ mod tests { Ok(()) } + #[test] + fn test_propagate_eq_false_identical_singletons() -> Result<()> { + // When both children are the same single-point interval, `a = b` is + // certainly true, so `NOT(a = b)` (parent = FALSE) is infeasible. + // This is the only case the `Eq + FALSE` arm reports as infeasible; + // see issue #19264 and PR #20138. + let left = Interval::make(Some(0_i64), Some(0_i64))?; + let right = Interval::make(Some(0_i64), Some(0_i64))?; + assert_eq!( + None, + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + Ok(()) + } + + #[test] + fn test_propagate_eq_false_signed_zero_singletons() -> Result<()> { + // `-0.0` and `+0.0` are equal under SQL/IEEE-754 comparison + // semantics, even though `ScalarValue::PartialEq` is bit-wise and + // treats them as distinct. When both children are singletons that + // are equal under comparison semantics, `a = b` is certainly true + // and `NOT(a = b)` is infeasible. + // + // This case is reachable in nested boolean contexts (e.g. + // `NOT(a = +0.0) AND b`) where the bottom-up pass does not + // short-circuit before reaching `propagate_comparison`. + let left = Interval::try_new( + ScalarValue::Float32(Some(-0.0f32)), + ScalarValue::Float32(Some(-0.0f32)), + )?; + let right = Interval::try_new( + ScalarValue::Float32(Some(0.0f32)), + ScalarValue::Float32(Some(0.0f32)), + )?; + assert_eq!( + None, + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + // Float64 variant. + let left = Interval::try_new( + ScalarValue::Float64(Some(-0.0f64)), + ScalarValue::Float64(Some(-0.0f64)), + )?; + let right = Interval::try_new( + ScalarValue::Float64(Some(0.0f64)), + ScalarValue::Float64(Some(0.0f64)), + )?; + assert_eq!( + None, + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + Ok(()) + } + + #[test] + fn test_propagate_eq_false_distinct_singletons() -> Result<()> { + // Different single points: `a = b` is certainly false, so `NOT(a = b)` + // is feasible. The children admit no further refinement, but must be + // preserved rather than reported as infeasible. + let left = Interval::make(Some(0_i64), Some(0_i64))?; + let right = Interval::make(Some(1_i64), Some(1_i64))?; + assert_eq!( + Some((left.clone(), right.clone())), + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + Ok(()) + } + + #[test] + fn test_propagate_eq_false_overlapping_intervals() -> Result<()> { + // The case from issue #19264: `a ∈ [-1, 1]`, `b = 0`. `a = b` is + // possibly true (at a = 0) and possibly false, so `NOT(a = b)` is + // feasible. Interval arithmetic cannot exclude the single point 0, + // so the children are returned unchanged rather than as infeasible. + let left = Interval::make(Some(-1_i64), Some(1_i64))?; + let right = Interval::make(Some(0_i64), Some(0_i64))?; + assert_eq!( + Some((left.clone(), right.clone())), + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + // Overlapping non-singleton intervals: same reasoning, feasible but + // not refinable. + let left = Interval::make(Some(-1_i64), Some(1_i64))?; + let right = Interval::make(Some(0_i64), Some(2_i64))?; + assert_eq!( + Some((left.clone(), right.clone())), + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + Ok(()) + } + #[test] fn test_propagate_or() -> Result<()> { let expr = Arc::new(BinaryExpr::new( From 4ef75fdfaba1dc73399f74e0d0f06cf6e5d575bc Mon Sep 17 00:00:00 2001 From: Smallfu666 Date: Thu, 20 Aug 2026 14:40:53 +0800 Subject: [PATCH 2/3] fix: preserve input intervals in propagate_comparison uncertainty arm Address review follow-ups: the catch-all uncertainty arm returned the infeasible sentinel instead of the unchanged input intervals, and the new is_unbounded() guard had no pinning tests. Signed-off-by: Han-Yin Chang --- datafusion/physical-expr/src/analysis.rs | 67 +++++++++++++++++++ .../physical-expr/src/intervals/cp_solver.rs | 21 +++++- 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/datafusion/physical-expr/src/analysis.rs b/datafusion/physical-expr/src/analysis.rs index 8ca9ad7fee7a6..e65f5c7e44d5d 100644 --- a/datafusion/physical-expr/src/analysis.rs +++ b/datafusion/physical-expr/src/analysis.rs @@ -657,6 +657,73 @@ mod tests { Ok(()) } + fn analyze_between_helper( + lower: f32, + upper: f32, + negated: bool, + explicit_not: bool, + ) -> datafusion_common::Result> { + let schema = Arc::new(Schema::new(vec![make_field("a", DataType::Float32)])); + let df_schema = DFSchema::try_from(Arc::clone(&schema)).unwrap(); + let boundaries = vec![ExprBoundaries { + column: Column::new("a", 0), + interval: Some(Interval::try_new( + ScalarValue::Float32(Some(lower)), + ScalarValue::Float32(Some(upper)), + )?), + distinct_count: Precision::Inexact(100), + }]; + + let between = col("a").between(lit(-1.0f32), lit(1.0f32)); + let predicate = if explicit_not { + between.not() + } else if negated { + col("a").not_between(lit(-1.0f32), lit(1.0f32)) + } else { + between + }; + let physical_expr = create_physical_expr( + &predicate, + &df_schema, + &ExecutionProps::new(), + &PhysicalPlanningContext::default(), + )?; + let output = analyze( + &physical_expr, + AnalysisContext::new(boundaries), + df_schema.as_ref(), + )?; + Ok(output.boundaries[0].interval.clone()) + } + + #[test] + fn test_analyze_not_between_clamped() -> datafusion_common::Result<()> { + let expected = Some(Interval::try_new( + ScalarValue::Float32(Some(-1.0)), + ScalarValue::Float32(Some(1.0)), + )?); + assert_eq!(analyze_between_helper(-1.0, 1.0, false, false)?, expected); + assert_eq!(analyze_between_helper(-1.0, 1.0, true, false)?, None); + assert_eq!(analyze_between_helper(-1.0, 1.0, false, true)?, None); + Ok(()) + } + + #[test] + fn test_analyze_not_between_containing() -> datafusion_common::Result<()> { + let between = Some(Interval::try_new( + ScalarValue::Float32(Some(-1.0)), + ScalarValue::Float32(Some(1.0)), + )?); + let containing = Some(Interval::try_new( + ScalarValue::Float32(Some(-2.0)), + ScalarValue::Float32(Some(2.0)), + )?); + assert_eq!(analyze_between_helper(-2.0, 2.0, false, false)?, between); + assert_eq!(analyze_between_helper(-2.0, 2.0, true, false)?, containing); + assert_eq!(analyze_between_helper(-2.0, 2.0, false, true)?, containing); + Ok(()) + } + // Regression test for the nested signed-zero case. // // `NOT(a = +0.0) AND b` with `a ∈ [-0.0,-0.0]`, `b ∈ [false,true]`: diff --git a/datafusion/physical-expr/src/intervals/cp_solver.rs b/datafusion/physical-expr/src/intervals/cp_solver.rs index 3b1a5a042ea4d..a7fc1cb7031a5 100644 --- a/datafusion/physical-expr/src/intervals/cp_solver.rs +++ b/datafusion/physical-expr/src/intervals/cp_solver.rs @@ -410,7 +410,7 @@ pub fn propagate_comparison( } } else { // Uncertainty cannot change any end-point of the intervals. - Ok(None) + Ok(Some((left_child.clone(), right_child.clone()))) } } @@ -1747,6 +1747,25 @@ mod tests { propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? ); + // A structurally singleton-looking `[NULL, NULL]` interval is + // unbounded, not a known value, and must not be treated as equal to a + // bounded singleton. + let left = Interval::make(Some(0_i64), Some(0_i64))?; + let right = Interval::make::(None, None)?; + assert_eq!( + Some((left.clone(), right.clone())), + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + // Two fully unbounded intervals likewise provide no proof that the + // operands are equal singletons. + let left = Interval::make::(None, None)?; + let right = Interval::make::(None, None)?; + assert_eq!( + Some((left.clone(), right.clone())), + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + Ok(()) } From 0a762d66a8aadeadb7cc5b543271b40025c644b0 Mon Sep 17 00:00:00 2001 From: Han-Yin Chang Date: Sat, 22 Aug 2026 21:56:46 +0800 Subject: [PATCH 3/3] fix: use Eq comparison semantics for the singleton check in Eq+FALSE Review follow-up. The Eq + FALSE guard used ScalarValue equality to decide whether each child is a singleton. That comparison is bit-wise for floats, while Interval::try_new orders endpoints with total_cmp and therefore accepts [-0.0, +0.0]. Such an interval denotes a single Eq-comparable value, so NOT(a = 0.0) over it is infeasible, but the bit-wise check treated it as non-singleton and returned the input intervals -- a false-feasibility result on the public analyze path. Use singleton_values_equal for both the per-child singleton check and the cross-child comparison, and add Float32/Float64 [-0.0, +0.0] coverage in cp_solver and through analyze. Also pin the shared uncertainty catch-all: propagate_comparison with a TRUE_OR_FALSE parent must return both operands unchanged for Eq, Gt, GtEq, Lt and LtEq. Signed-off-by: Han-Yin Chang --- datafusion/physical-expr/src/analysis.rs | 54 +++++++++ .../physical-expr/src/intervals/cp_solver.rs | 114 +++++++++++++++++- 2 files changed, 163 insertions(+), 5 deletions(-) diff --git a/datafusion/physical-expr/src/analysis.rs b/datafusion/physical-expr/src/analysis.rs index e65f5c7e44d5d..8fbf1e0371c3a 100644 --- a/datafusion/physical-expr/src/analysis.rs +++ b/datafusion/physical-expr/src/analysis.rs @@ -657,6 +657,60 @@ mod tests { Ok(()) } + fn analyze_not_eq_f64_helper( + lower: f64, + upper: f64, + ) -> datafusion_common::Result> { + let schema = Arc::new(Schema::new(vec![make_field("a", DataType::Float64)])); + let df_schema = DFSchema::try_from(Arc::clone(&schema)).unwrap(); + + let boundaries = vec![ExprBoundaries { + column: Column::new("a", 0), + interval: Some(Interval::try_new( + ScalarValue::Float64(Some(lower)), + ScalarValue::Float64(Some(upper)), + )?), + distinct_count: Precision::Inexact(100), + }]; + let ctx = AnalysisContext::new(boundaries); + + let pred_not_eq = col("a").eq(lit(0.0f64)).not(); + let phys_not = create_physical_expr( + &pred_not_eq, + &df_schema, + &ExecutionProps::new(), + &PhysicalPlanningContext::default(), + )?; + let out = analyze(&phys_not, ctx, df_schema.as_ref())?; + Ok(out.boundaries[0].interval.clone()) + } + + // Regression test for the signed-zero singleton case. + // + // `Interval::try_new` orders endpoints with `total_cmp`, so + // `[-0.0, +0.0]` is a valid input domain. Its endpoints differ bit-wise + // but denote a single value under `Eq` comparison semantics, so + // `a = 0.0` is certainly true over it and `NOT (a = 0.0)` is infeasible. + // A bit-wise singleton check in `propagate_comparison` would instead + // report the input domain, i.e. a false-feasibility result on the public + // `analyze` path. + #[test] + fn test_analyze_not_eq_signed_zero_span_infeasible() -> datafusion_common::Result<()> + { + let out = analyze_not_eq_helper(-0.0, 0.0)?; + assert_eq!( + out, None, + "NOT (a = 0.0) over Float32 [-0.0, +0.0] should yield None (infeasible), got {out:?}" + ); + + let out = analyze_not_eq_f64_helper(-0.0, 0.0)?; + assert_eq!( + out, None, + "NOT (a = 0.0) over Float64 [-0.0, +0.0] should yield None (infeasible), got {out:?}" + ); + Ok(()) + } + fn analyze_between_helper( lower: f32, upper: f32, diff --git a/datafusion/physical-expr/src/intervals/cp_solver.rs b/datafusion/physical-expr/src/intervals/cp_solver.rs index a7fc1cb7031a5..f9d9561feeeef 100644 --- a/datafusion/physical-expr/src/intervals/cp_solver.rs +++ b/datafusion/physical-expr/src/intervals/cp_solver.rs @@ -296,8 +296,8 @@ pub fn propagate_arithmetic( } } -/// Match runtime `Eq`'s signed-zero semantics before using `ScalarValue` -/// equality for singleton operands. `ScalarValue::PartialEq` is bit-wise for +/// Compare two scalars the way runtime `Eq` does, instead of with +/// `ScalarValue`'s bit-wise equality. `ScalarValue::PartialEq` is bit-wise for /// floats (`to_bits`), under which `-0.0 != +0.0`; runtime `Eq` normalizes /// them via `normalize_float_zero_scalar` (see /// `physical-expr-common/src/datum.rs`). @@ -386,11 +386,15 @@ pub fn propagate_comparison( // `a = b` is certainly true, so `NOT(a = b)` is infeasible. // `ScalarValue::PartialEq` is bit-wise for floats, so // `singleton_values_equal` normalizes signed zero to match - // runtime `Eq` behavior before comparing. + // runtime `Eq` behavior -- both when deciding whether a child + // is a singleton and when comparing the two children. + // `Interval::try_new` orders endpoints with `total_cmp`, so + // `[-0.0, +0.0]` is a valid interval whose endpoints differ + // bit-wise but denote a single `Eq`-comparable value. if !left_child.is_unbounded() && !right_child.is_unbounded() - && left_child.lower() == left_child.upper() - && right_child.lower() == right_child.upper() + && singleton_values_equal(left_child.lower(), left_child.upper()) + && singleton_values_equal(right_child.lower(), right_child.upper()) && singleton_values_equal(left_child.lower(), right_child.lower()) { Ok(None) @@ -1769,6 +1773,106 @@ mod tests { Ok(()) } + #[test] + fn test_propagate_eq_false_signed_zero_interval_singleton() -> Result<()> { + // `Interval::try_new` orders endpoints with `total_cmp`, so + // `[-0.0, +0.0]` is a valid interval. Its endpoints differ bit-wise + // but denote a single value under `Eq` comparison semantics, so + // `a = 0.0` is certainly true over it and `NOT(a = 0.0)` is + // infeasible. A bit-wise `lower() == upper()` singleton check would + // miss this and report the input intervals as feasible. + let left = Interval::try_new( + ScalarValue::Float32(Some(-0.0f32)), + ScalarValue::Float32(Some(0.0f32)), + )?; + let right = Interval::try_new( + ScalarValue::Float32(Some(0.0f32)), + ScalarValue::Float32(Some(0.0f32)), + )?; + assert_eq!( + None, + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + // The signed-zero span may sit on either side. + assert_eq!( + None, + propagate_comparison(&Operator::Eq, &Interval::FALSE, &right, &left)? + ); + + // Float64 variant. + let left = Interval::try_new( + ScalarValue::Float64(Some(-0.0f64)), + ScalarValue::Float64(Some(0.0f64)), + )?; + let right = Interval::try_new( + ScalarValue::Float64(Some(-0.0f64)), + ScalarValue::Float64(Some(0.0f64)), + )?; + assert_eq!( + None, + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + // A genuinely wider float interval is still not a singleton. + let left = Interval::try_new( + ScalarValue::Float64(Some(-0.0f64)), + ScalarValue::Float64(Some(1.0f64)), + )?; + let right = Interval::try_new( + ScalarValue::Float64(Some(0.0f64)), + ScalarValue::Float64(Some(0.0f64)), + )?; + assert_eq!( + Some((left.clone(), right.clone())), + propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)? + ); + + Ok(()) + } + + #[test] + fn test_propagate_comparison_uncertain_parent_preserves_operands() -> Result<()> { + // A `TRUE_OR_FALSE` parent proves nothing about either operand, so the + // shared catch-all arm must return both children unchanged for every + // comparison operator. Returning `None` here would be read as + // infeasible and would discard satisfiable ranges (issue #19264). + let left = Interval::make(Some(-1_i64), Some(5_i64))?; + let right = Interval::make(Some(0_i64), Some(2_i64))?; + for op in [ + Operator::Eq, + Operator::Gt, + Operator::GtEq, + Operator::Lt, + Operator::LtEq, + ] { + assert_eq!( + Some((left.clone(), right.clone())), + propagate_comparison(&op, &Interval::TRUE_OR_FALSE, &left, &right)?, + "operands must be preserved for {op} under an uncertain parent" + ); + } + + // Also holds for singleton and unbounded operands. + let left = Interval::make(Some(0_i64), Some(0_i64))?; + let right = Interval::make::(None, None)?; + for op in [ + Operator::Eq, + Operator::Gt, + Operator::GtEq, + Operator::Lt, + Operator::LtEq, + ] { + assert_eq!( + Some((left.clone(), right.clone())), + propagate_comparison(&op, &Interval::TRUE_OR_FALSE, &left, &right)?, + "operands must be preserved for {op} under an uncertain parent" + ); + } + + Ok(()) + } + #[test] fn test_propagate_or() -> Result<()> { let expr = Arc::new(BinaryExpr::new(