Skip to content

Commit 04acb8d

Browse files
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 apache#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 apache#20138, which was closed as stale after review. The singleton guard was agreed on during apache#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 6eaca8b 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 apache#19264. Co-authored-by: evangelisilva <[email protected]>
1 parent 19f2e85 commit 04acb8d

2 files changed

Lines changed: 265 additions & 3 deletions

File tree

‎datafusion/physical-expr/src/analysis.rs‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,7 @@ fn calculate_selectivity(
344344

345345
#[cfg(test)]
346346
mod tests {
347+
use std::ops::Not;
347348
use std::sync::Arc;
348349

349350
use arrow::datatypes::{DataType, Field, Schema};
@@ -589,4 +590,136 @@ mod tests {
589590

590591
let _ = selectivity; // silence unused warning
591592
}
593+
594+
// Regression tests for #19264.
595+
//
596+
// `analyze` propagates constraints through `NotExpr::propagate_constraints`,
597+
// which hands the underlying `Eq` node a `FALSE` parent interval. When the
598+
// input domain contains `0.0` but is not equal to it, `NOT (a = 0.0)` is
599+
// satisfiable, so `analyze` reports the input domain. `None` is reserved
600+
// for an empty set, as documented on `ExprBoundaries::interval`.
601+
fn analyze_not_eq_helper(
602+
lower: f32,
603+
upper: f32,
604+
) -> datafusion_common::Result<Option<Interval>> {
605+
let schema = Arc::new(Schema::new(vec![make_field("a", DataType::Float32)]));
606+
let df_schema = DFSchema::try_from(Arc::clone(&schema)).unwrap();
607+
608+
// Input column bounds: [lower, upper]
609+
let boundaries = vec![ExprBoundaries {
610+
column: Column::new("a", 0),
611+
interval: Some(Interval::try_new(
612+
ScalarValue::Float32(Some(lower)),
613+
ScalarValue::Float32(Some(upper)),
614+
)?),
615+
distinct_count: Precision::Inexact(100),
616+
}];
617+
let ctx = AnalysisContext::new(boundaries);
618+
619+
// Predicate: NOT (a = 0.0)
620+
let pred_not_eq = col("a").eq(lit(0.0f32)).not();
621+
let phys_not = create_physical_expr(
622+
&pred_not_eq,
623+
&df_schema,
624+
&ExecutionProps::new(),
625+
&PhysicalPlanningContext::default(),
626+
)?;
627+
let out = analyze(&phys_not, ctx, df_schema.as_ref())?;
628+
Ok(out.boundaries[0].interval.clone())
629+
}
630+
631+
#[test]
632+
fn test_analyze_not_eq_around_zero() -> datafusion_common::Result<()> {
633+
// Input domain [-1, 1] contains 0.0.
634+
// `NOT (a = 0.0)` can be true everywhere except at 0.0, so the output
635+
// interval should be the full input domain [-1, 1], NOT None.
636+
let out = analyze_not_eq_helper(-1.0, 1.0)?;
637+
let expected = Some(Interval::try_new(
638+
ScalarValue::Float32(Some(-1.0)),
639+
ScalarValue::Float32(Some(1.0)),
640+
)?);
641+
assert_eq!(
642+
out, expected,
643+
"NOT (a = 0.0) over [-1, 1] should yield [-1, 1], got {out:?}"
644+
);
645+
Ok(())
646+
}
647+
648+
#[test]
649+
fn test_analyze_not_eq_clamped_at_zero() -> datafusion_common::Result<()> {
650+
// Input domain [0, 0] is exactly the point where a = 0.0 is true.
651+
// `NOT (a = 0.0)` is always false here, so the output interval should be None.
652+
let out = analyze_not_eq_helper(0.0, 0.0)?;
653+
assert_eq!(
654+
out, None,
655+
"NOT (a = 0.0) over [0, 0] should yield None (infeasible), got {out:?}"
656+
);
657+
Ok(())
658+
}
659+
660+
// Regression test for the nested signed-zero case.
661+
//
662+
// `NOT(a = +0.0) AND b` with `a ∈ [-0.0,-0.0]`, `b ∈ [false,true]`:
663+
// `a = +0.0` is TRUE (IEEE 754), so the whole predicate is infeasible.
664+
//
665+
// This defeats the case-(1) short-circuit in `update_ranges`: bottom-up
666+
// evaluates `a = +0.0` over `[-0.0,-0.0]` as FALSE (structural equality),
667+
// so `NOT` becomes TRUE, the root becomes `TRUE_OR_FALSE`, and top-down
668+
// propagation forces `a = +0.0` to FALSE, reaching
669+
// `propagate_comparison(Eq, FALSE, [-0.0,-0.0], [+0.0,+0.0])`. The guard
670+
// must normalize signed zero to report infeasible here.
671+
#[test]
672+
fn test_analyze_not_eq_nested_signed_zero_infeasible() -> datafusion_common::Result<()>
673+
{
674+
let schema = Arc::new(Schema::new(vec![
675+
make_field("a", DataType::Float32),
676+
make_field("b", DataType::Boolean),
677+
]));
678+
let df_schema = DFSchema::try_from(Arc::clone(&schema)).unwrap();
679+
680+
let boundaries = vec![
681+
ExprBoundaries {
682+
column: Column::new("a", 0),
683+
interval: Some(Interval::try_new(
684+
ScalarValue::Float32(Some(-0.0f32)),
685+
ScalarValue::Float32(Some(-0.0f32)),
686+
)?),
687+
distinct_count: Precision::Inexact(1),
688+
},
689+
ExprBoundaries {
690+
column: Column::new("b", 1),
691+
interval: Some(Interval::try_new(
692+
ScalarValue::Boolean(Some(false)),
693+
ScalarValue::Boolean(Some(true)),
694+
)?),
695+
distinct_count: Precision::Inexact(2),
696+
},
697+
];
698+
let ctx = AnalysisContext::new(boundaries);
699+
700+
// NOT(a = +0.0) AND b
701+
let pred = col("a").eq(lit(0.0f32)).not().and(col("b"));
702+
let phys = create_physical_expr(
703+
&pred,
704+
&df_schema,
705+
&ExecutionProps::new(),
706+
&PhysicalPlanningContext::default(),
707+
)?;
708+
let out = analyze(&phys, ctx, df_schema.as_ref())?;
709+
710+
// Infeasible: a = +0.0 is TRUE (signed zero), NOT is FALSE,
711+
// FALSE AND b = FALSE. All column intervals must be None.
712+
assert!(
713+
out.boundaries.iter().all(|b| b.interval.is_none()),
714+
"NOT(a = +0.0) AND b over a=[-0.0,-0.0] should be infeasible (all boundaries None), got {:?}",
715+
out.boundaries
716+
);
717+
assert_eq!(
718+
out.selectivity,
719+
Some(0.0),
720+
"selectivity should be 0.0 for an infeasible predicate, got {:?}",
721+
out.selectivity
722+
);
723+
Ok(())
724+
}
592725
}

‎datafusion/physical-expr/src/intervals/cp_solver.rs‎

Lines changed: 132 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ use crate::expressions::{BinaryExpr, Literal};
153153
use crate::utils::{ExprTreeNode, build_dag};
154154

155155
use arrow::datatypes::{DataType, Schema};
156-
use datafusion_common::{Result, internal_err, not_impl_err};
156+
use datafusion_common::{Result, ScalarValue, internal_err, not_impl_err};
157157
use datafusion_expr::Operator;
158158
use datafusion_expr::interval_arithmetic::{Interval, apply_operator, satisfy_greater};
159159

@@ -296,6 +296,17 @@ pub fn propagate_arithmetic(
296296
}
297297
}
298298

299+
/// Match runtime `Eq`'s signed-zero semantics before using `ScalarValue`
300+
/// equality for singleton operands. `ScalarValue::PartialEq` is bit-wise for
301+
/// floats (`to_bits`), under which `-0.0 != +0.0`; runtime `Eq` normalizes
302+
/// them via `normalize_float_zero_scalar` (see
303+
/// `physical-expr-common/src/datum.rs`).
304+
fn singleton_values_equal(left: &ScalarValue, right: &ScalarValue) -> bool {
305+
use datafusion_common::utils::normalize_float_zero_scalar;
306+
normalize_float_zero_scalar(left.clone())
307+
== normalize_float_zero_scalar(right.clone())
308+
}
309+
299310
/// This function refines intervals `left_child` and `right_child` by applying
300311
/// comparison propagation through `parent` via operation. The main idea is
301312
/// that we can shrink ranges of variables x and y using parent interval p.
@@ -363,8 +374,29 @@ pub fn propagate_comparison(
363374
} else if parent == &Interval::FALSE {
364375
match op {
365376
Operator::Eq => {
366-
// TODO: Propagation is not possible until we support interval sets.
367-
Ok(None)
377+
// `a = b` being certainly false means `a != b`, which excludes
378+
// at most a single point from each child. A single interval
379+
// cannot represent that excluded point, so returning the
380+
// children unchanged is a safe over-approximation. Returning
381+
// `None` is not: the caller reads it as infeasible, which
382+
// discards satisfiable ranges (see issue #19264).
383+
//
384+
// The exception is when both children are singletons that are
385+
// equal under the `Eq` operator's comparison semantics: then
386+
// `a = b` is certainly true, so `NOT(a = b)` is infeasible.
387+
// `ScalarValue::PartialEq` is bit-wise for floats, so
388+
// `singleton_values_equal` normalizes signed zero to match
389+
// runtime `Eq` behavior before comparing.
390+
if !left_child.is_unbounded()
391+
&& !right_child.is_unbounded()
392+
&& left_child.lower() == left_child.upper()
393+
&& right_child.lower() == right_child.upper()
394+
&& singleton_values_equal(left_child.lower(), right_child.lower())
395+
{
396+
Ok(None)
397+
} else {
398+
Ok(Some((left_child.clone(), right_child.clone())))
399+
}
368400
}
369401
Operator::Gt => satisfy_greater(right_child, left_child, false),
370402
Operator::GtEq => satisfy_greater(right_child, left_child, true),
@@ -1621,6 +1653,103 @@ mod tests {
16211653
Ok(())
16221654
}
16231655

1656+
#[test]
1657+
fn test_propagate_eq_false_identical_singletons() -> Result<()> {
1658+
// When both children are the same single-point interval, `a = b` is
1659+
// certainly true, so `NOT(a = b)` (parent = FALSE) is infeasible.
1660+
// This is the only case the `Eq + FALSE` arm reports as infeasible;
1661+
// see issue #19264 and PR #20138.
1662+
let left = Interval::make(Some(0_i64), Some(0_i64))?;
1663+
let right = Interval::make(Some(0_i64), Some(0_i64))?;
1664+
assert_eq!(
1665+
None,
1666+
propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)?
1667+
);
1668+
1669+
Ok(())
1670+
}
1671+
1672+
#[test]
1673+
fn test_propagate_eq_false_signed_zero_singletons() -> Result<()> {
1674+
// `-0.0` and `+0.0` are equal under SQL/IEEE-754 comparison
1675+
// semantics, even though `ScalarValue::PartialEq` is bit-wise and
1676+
// treats them as distinct. When both children are singletons that
1677+
// are equal under comparison semantics, `a = b` is certainly true
1678+
// and `NOT(a = b)` is infeasible.
1679+
//
1680+
// This case is reachable in nested boolean contexts (e.g.
1681+
// `NOT(a = +0.0) AND b`) where the bottom-up pass does not
1682+
// short-circuit before reaching `propagate_comparison`.
1683+
let left = Interval::try_new(
1684+
ScalarValue::Float32(Some(-0.0f32)),
1685+
ScalarValue::Float32(Some(-0.0f32)),
1686+
)?;
1687+
let right = Interval::try_new(
1688+
ScalarValue::Float32(Some(0.0f32)),
1689+
ScalarValue::Float32(Some(0.0f32)),
1690+
)?;
1691+
assert_eq!(
1692+
None,
1693+
propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)?
1694+
);
1695+
1696+
// Float64 variant.
1697+
let left = Interval::try_new(
1698+
ScalarValue::Float64(Some(-0.0f64)),
1699+
ScalarValue::Float64(Some(-0.0f64)),
1700+
)?;
1701+
let right = Interval::try_new(
1702+
ScalarValue::Float64(Some(0.0f64)),
1703+
ScalarValue::Float64(Some(0.0f64)),
1704+
)?;
1705+
assert_eq!(
1706+
None,
1707+
propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)?
1708+
);
1709+
1710+
Ok(())
1711+
}
1712+
1713+
#[test]
1714+
fn test_propagate_eq_false_distinct_singletons() -> Result<()> {
1715+
// Different single points: `a = b` is certainly false, so `NOT(a = b)`
1716+
// is feasible. The children admit no further refinement, but must be
1717+
// preserved rather than reported as infeasible.
1718+
let left = Interval::make(Some(0_i64), Some(0_i64))?;
1719+
let right = Interval::make(Some(1_i64), Some(1_i64))?;
1720+
assert_eq!(
1721+
Some((left.clone(), right.clone())),
1722+
propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)?
1723+
);
1724+
1725+
Ok(())
1726+
}
1727+
1728+
#[test]
1729+
fn test_propagate_eq_false_overlapping_intervals() -> Result<()> {
1730+
// The case from issue #19264: `a ∈ [-1, 1]`, `b = 0`. `a = b` is
1731+
// possibly true (at a = 0) and possibly false, so `NOT(a = b)` is
1732+
// feasible. Interval arithmetic cannot exclude the single point 0,
1733+
// so the children are returned unchanged rather than as infeasible.
1734+
let left = Interval::make(Some(-1_i64), Some(1_i64))?;
1735+
let right = Interval::make(Some(0_i64), Some(0_i64))?;
1736+
assert_eq!(
1737+
Some((left.clone(), right.clone())),
1738+
propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)?
1739+
);
1740+
1741+
// Overlapping non-singleton intervals: same reasoning, feasible but
1742+
// not refinable.
1743+
let left = Interval::make(Some(-1_i64), Some(1_i64))?;
1744+
let right = Interval::make(Some(0_i64), Some(2_i64))?;
1745+
assert_eq!(
1746+
Some((left.clone(), right.clone())),
1747+
propagate_comparison(&Operator::Eq, &Interval::FALSE, &left, &right)?
1748+
);
1749+
1750+
Ok(())
1751+
}
1752+
16241753
#[test]
16251754
fn test_propagate_or() -> Result<()> {
16261755
let expr = Arc::new(BinaryExpr::new(

0 commit comments

Comments
 (0)