pepijnve commented on code in PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#discussion_r2761116502
##########
datafusion/physical-expr/src/intervals/cp_solver.rs:
##########
@@ -364,7 +364,7 @@ pub fn propagate_comparison(
match op {
Operator::Eq => {
// TODO: Propagation is not possible until we support interval
sets.
- Ok(None)
+ Ok(Some((left_child.clone(), right_child.clone())))
Review Comment:
I think this is correct, but only if the left and right child intervals do
not intersect. So maybe this should be
```
match left_child.intersect(right_child)? {
None => Ok(Some((left_child.clone(), right_child.clone()))),
// TODO: Propagation is not possible until we support interval sets.
Some(_) => Ok(None)
}
```
instead?
In the test case
```
#[test]
fn test_propagate_eq_false() -> Result<()> {
let left = Interval::make(Some(-10_i64), Some(10_i64))?;
let right = Interval::make(Some(0_i64), Some(0_i64))?;
let outcome = propagate_comparison(&Operator::Eq, &Interval::FALSE,
&left, &right)?;
assert_eq!(
None,
outcome
);
Ok(())
}
```
for instance, the result for the left child should be the intervals `[-10,
0[` and `]0, 10]`, but as the TODO suggests that can't currently be
represented. The proposed fix would return `[-10, 10]` for the left child, but
that is not correct since a value of `0` for the left child would result in
true, not false.
--
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]
