Smallfu666 commented on code in PR #24464:
URL: https://github.com/apache/datafusion/pull/24464#discussion_r3838217021
##########
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:
Good catch, and confirmed. `Interval::try_new` orders endpoints with
`total_cmp`, so `[-0.0, +0.0]` is a valid interval whose endpoints differ
bit-wise. The old `lower() == upper()` singleton check therefore missed it and
`NOT (a = 0.0)` came back feasible over a domain where it is not.
Fixed in 0a762d6: `singleton_values_equal` now drives both the per-child
singleton check and the cross-child comparison, so the whole guard uses `Eq`
comparison semantics rather than `ScalarValue`'s bit-wise equality.
Added `[-0.0, +0.0]` coverage for Float32 and Float64, both directly on
`propagate_comparison` and through the public `analyze` path. Both new tests
fail on the previous commit and pass on this one.
##########
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:
Agreed, the indirect `BETWEEN` coverage was too far from the contract. Added
in 0a762d6: `test_propagate_comparison_uncertain_parent_preserves_operands`
asserts that a `TRUE_OR_FALSE` parent returns both operands unchanged for `Eq`,
`Gt`, `GtEq`, `Lt` and `LtEq`, over ranged as well as singleton/unbounded
operands.
--
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]
