fix(physical-expr): stop false infeasibility report from Eq+FALSE in cp_solver - #24464
fix(physical-expr): stop false infeasibility report from Eq+FALSE in cp_solver#24464Smallfu666 wants to merge 3 commits into
Conversation
…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 <silvaevangeli@gmail.com>
Address review follow-ups: the catch-all uncertainty arm returned the infeasible sentinel instead of the unchanged input intervals, and the new is_unbounded() guard had no pinning tests. Signed-off-by: Han-Yin Chang <nick20350@gmail.com>
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The overall direction looks good, especially preserving satisfiable Eq + FALSE operands and keeping the proven singleton infeasibility behavior.
I found one signed-zero edge case that still affects correctness, so I think this needs one more change before merging. I also left a small regression-test suggestion to make the new catch-all behavior more explicit.
| // runtime `Eq` behavior before comparing. | ||
| if !left_child.is_unbounded() | ||
| && !right_child.is_unbounded() | ||
| && left_child.lower() == left_child.upper() |
There was a problem hiding this 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.
There was a problem hiding this 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.
| } else { | ||
| // Uncertainty cannot change any end-point of the intervals. | ||
| Ok(None) | ||
| Ok(Some((left_child.clone(), right_child.clone()))) |
There was a problem hiding this 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.
There was a problem hiding this 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.
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24464 +/- ##
==========================================
+ Coverage 81.22% 81.27% +0.05%
==========================================
Files 1113 1116 +3
Lines 392250 395216 +2966
Branches 392250 395216 +2966
==========================================
+ Hits 318596 321213 +2617
- Misses 54910 55169 +259
- Partials 18744 18834 +90 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review follow-up. The Eq + FALSE guard used ScalarValue equality to decide whether each child is a singleton. That comparison is bit-wise for floats, while Interval::try_new orders endpoints with total_cmp and therefore accepts [-0.0, +0.0]. Such an interval denotes a single Eq-comparable value, so NOT(a = 0.0) over it is infeasible, but the bit-wise check treated it as non-singleton and returned the input intervals -- a false-feasibility result on the public analyze path. Use singleton_values_equal for both the per-child singleton check and the cross-child comparison, and add Float32/Float64 [-0.0, +0.0] coverage in cp_solver and through analyze. Also pin the shared uncertainty catch-all: propagate_comparison with a TRUE_OR_FALSE parent must return both operands unchanged for Eq, Gt, GtEq, Lt and LtEq. Signed-off-by: Han-Yin Chang <nick20350@gmail.com>
3 participants
Which issue does this PR close?
physical_expr::analyze#19264.Rationale for this change
physical_expr::analyzeis a public API. For a satisfiable predicate such asNOT (a = 0.0)overa ∈ [-1, 1], it currently returnsNone, whichExprBoundaries::intervaldocuments as an empty set. The safe result is theunchanged input interval because one interval cannot represent the hole at zero.
The same infeasible-sentinel mistake exists in
propagate_comparison's catch-alluncertainty arm. That arm is shared by every supported comparison operator that
reaches it with an uncertain Boolean parent. Returning the child intervals unchanged
preserves their enclosure for all operand shapes, including shapes beyond the three
issue test functions; it does not claim a refinement that interval arithmetic cannot
prove.
This carries forward the original patch by @evangelisilva in #20138 and preserves
its reviewed equal-singleton infeasibility guard. The original bug-fix commit in this PR contains a valid
Co-authored-bytrailer for the original author.What changes are included in this PR?
Eq + FALSE, preserve both child intervals unless both are bounded,equal singletons. Only the latter case is provably infeasible.
instead of returning the infeasible sentinel.
preventing
[NULL, NULL]from being mistaken for a known singleton.NOT (a = 0.0),not_between(...), and explicitbetween(...).not()over the issue's input domains.Are these changes tested?
Yes. The three issue test functions execute independently:
test_analyze_not_eq_around_zeropasses with the repair.test_analyze_not_between_clampedis a baseline-passing negative control: itsinfeasible result remains unchanged.
test_analyze_not_between_containingpasses with the repair for both NOT spellings.The complete
datafusion-physical-exprlibrary suite passes: 1607 passed,0 failed, 2 ignored. Formatting and clippy with all targets and all features
also pass.
Are there any user-facing changes?
No API signature change.
analyze()no longer reports a satisfiable negatedcomparison as infeasible when interval arithmetic cannot refine its operands.