You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "evangelisilva (via GitHub)" <gi...@apache.org> on 2026/02/03 17:04:01 UTC
[PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
evangelisilva opened a new pull request, #20138:
URL: https://github.com/apache/datafusion/pull/20138
## Which issue does this PR close?
Closes #19264.
## Rationale for this change
The `analyze` function in `physical-expr` was incorrectly returning `None` (implying an empty set/infeasibility) when analyzing `NOT(Eq)` expressions (e.g., `NOT(a = 0.0)`).
This was caused by `propagate_comparison` in `cp_solver.rs` returning `Ok(None)` for `Operator::Eq` when the parent interval is `FALSE`. `Ok(None)` in `cp_solver` is interpreted as "Infeasible", but `NOT(a=b)` is a feasible condition (it effectively creates two disjoint intervals, which the current `Interval` implementation cannot represent as a single interval, but it is certainly not empty).
## What changes are included in this PR?
- Modified `propagate_comparison` in `cp_solver.rs`. When handling `Operator::Eq` with a `FALSE` parent (meaning we are looking for "Not Equal"), it now returns the original intervals (identity) instead of `None`. This treats the constraint as valid/feasible but properly reflects that we cannot refine the bounds further with single-interval arithmetic.
- Added a new regression test `analyze_not_eq_around` in `datafusion/physical-expr/src/analysis.rs` to verify that `NOT(a = 0.0)` now returns a valid interval encompassing the domain instead of `None`.
## Are these changes tested?
Yes, tested locally.
- Run command: `cargo test -p datafusion-physical-expr --lib analysis::tests`
- Verification: The new test `analyze_not_eq_around` passes, and existing tests in `analysis.rs` also pass.
## Are there any user-facing changes?
No, this is a bug fix in the physical expression analysis logic.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#issuecomment-3843984257
The fix seems correct to me. To elaborate, assume we have 2 child intervals; A and B. Propagation of TRUE over Eq is done like that:
We prune A such: "choose a point in A. If that point cannot be satisfied by any point from B, then that point is pruned."
and for B, similarly: "choose a point in B. If that point cannot be satisfied by any point from A, then that point is pruned."
So, what I think now is, we don't actually require interval sets for this problem, only except this case:
A is a set which strictly contains B, and B is a single-point value. In that case, we need to split the interval A into 2 parts at the intersection point (or vice versa for B and A). However, leaving the fix as is is not a big problem. We will only not be capable of a tiny optimization.
But to not introduce another bug, the only case which should keep as infeasible is, having 2 same single-point values. In that case, having an equality as false is impossible.
cc @ozankabak if I missed anything
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#issuecomment-3868274676
Thanks for the effort. Let's fix the fmt, remove the redundant diffs, and we'll merge this. It would be also better if you add a test for that edge case
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "evangelisilva (via GitHub)" <gi...@apache.org>.
evangelisilva commented on PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#issuecomment-3868213640
@pepijnve @berkaysynnada I've updated the PR to handle the `Not(Eq)` logic more precisely.
Specifically:
1. If `left == right` and both are valid singleton intervals (identical single points), it now returns `Ok(None)` (Infeasible).
2. For all other cases (different or overlapping intervals), it falls back to the safe over-approximation of returning the original intervals, as discussed regarding the current limitations with interval sets.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#discussion_r2762630846
##########
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:
Based on @berkaysynnada's answer I see I misinterpreted the contract of `propagate_comparison`. I had assumed it needed to provide a correct stricter constraint if it could or `None` if it couldn't. If not restricting the child constraints is valid, which makes perfect sense, then indeed there's just the situation where we can know for sure that `(left == right) == true`, i.e. two equal point intervals, that we should return `None`. In all other cases we can simply retain the existing constraints.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "evangelisilva (via GitHub)" <gi...@apache.org>.
evangelisilva commented on PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#issuecomment-3844033770
@pepijnve @berkaysynnada Thanks for the review! I agree, that makes sense.
I will update the PR to handle the specific case where both sides are single points (constants). In that case, if `left == right`, we know `Not(Eq)` is impossible, so we can correctly return `Infeasible` (`None`).
For other interval cases, I'll keep the safe over-approximation (returning the original intervals) until interval sets are supported.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#issuecomment-4317539550
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #20138: fix: incorrect results when using NOT physical expression in physical…
URL: https://github.com/apache/datafusion/pull/20138
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "evangelisilva (via GitHub)" <gi...@apache.org>.
evangelisilva commented on PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#issuecomment-3905070967
@berkaysynnada I have updated the PR based on your feedback:
* **Fixed Formatting**: Ran `cargo fmt` to ensure everything aligns with the project's style.
* **Cleaned Diffs**: Removed redundant diffs and kept the changes focused on the fix.
* **Added Edge Case Test**: I’ve added a new test case, [analyze_not_eq_infeasible](cci:1://file:///Users/evangelisilva/Documents/datafusion/datafusion/physical-expr/src/analysis.rs:475:4-506:5), in [analysis.rs](cci:7://file:///Users/evangelisilva/Documents/datafusion/datafusion/physical-expr/src/analysis.rs:0:0-0:0). This test specifically verifies that `NOT (a = 0.0)` for a column `a` already fixed at `0.0` correctly results in an `Infeasible` outcome (0.0 selectivity and `None` intervals).
Ready for another look. Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: incorrect results when using NOT physical expression in physical… [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #20138:
URL: https://github.com/apache/datafusion/pull/20138#issuecomment-3843784971
@alamb I'm not super familiar with this code, but looking at the review I think I can see a counter example that produces an incorrect response with this change. I will try to write that to confirm my hypothesis.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org