kosiew commented on code in PR #24464:
URL: https://github.com/apache/datafusion/pull/24464#discussion_r3836042433
##########
datafusion/physical-expr/src/intervals/cp_solver.rs:
##########
@@ -378,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())))
Review Comment:
Could we add direct regression coverage for `Interval::TRUE_OR_FALSE` with
`Eq`, `Gt`, `GtEq`, `Lt`, and `LtEq`, asserting that both operands remain
unchanged?
The `BETWEEN` integration tests cover this indirectly, but a focused test
here would pin down the new shared catch-all contract and make future
regressions easier to diagnose.
##########
datafusion/physical-expr/src/intervals/cp_solver.rs:
##########
@@ -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()
Review Comment:
`ScalarValue::PartialEq` is also being used here to decide whether each
interval is a singleton. For floats, that comparison is bitwise, while
`Interval::try_new(Float32(-0.0), Float32(+0.0))` accepts those endpoints
through `total_cmp`.
That means this interval contains only SQL-equal zero values, so `NOT (a =
0.0)` should be infeasible. With the current guard, though, it is treated as
non-singleton and the input interval is returned.
Could we use `singleton_values_equal(lower, upper)` for each child's
singleton check as well, then compare the normalized child values? It would
also be good to add `[-0.0, +0.0]` coverage for both Float32 and Float64.
Without that, this can reintroduce a false-feasibility result through the
public `analyze` path for valid signed-zero bounds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
