Smallfu666 opened a new pull request, #24464:
URL: https://github.com/apache/datafusion/pull/24464
## Which issue does this PR close?
- Closes #19264.
## Rationale for this change
`physical_expr::analyze` is a public API. As reported in #19264, an external
user calling `analyze()` to infer bounds for pushdown into another library
receives `None` (infeasible) for the column interval when the expression is
`NOT (a = 0.0)` and the input domain of `a` contains `0.0` but is not equal
to it (e.g. `a ∈ [-1, 1]`).
The `ExprBoundaries::interval` doc states that `None` means "evaluating the
given column results in an empty set" — i.e. the predicate is always false
over the input interval. But `NOT (a = 0.0)` is true for every `a ≠ 0`, so
over `[-1, 1]` the predicate is satisfiable and the correct result is the
full input domain `[-1, 1]`. The current `None` is a contract violation.
This revives the fix from #20138, which was closed as stale after review. It
preserves the reviewed identical-singleton infeasibility case (identified by
@berkaysynnada and @pepijnve during #20138 review) and adds direct unit
coverage that isolates the ordinary identical-singleton case. The `[0,0]`
`analyze()` test cannot reach that arm because `update_ranges`
short-circuits;
the nested signed-zero `analyze()` test does reach it through the
pre-existing
interval/runtime equality divergence (see "Are these changes tested?" below).
Scope: the `FilterExec` statistics path does not currently reach `analyze()`
for a predicate containing a `NotExpr`. `check_support`
(`intervals/utils.rs`)
accepts only `BinaryExpr`, `Column`, `Literal`, `CastExpr` and
`NegativeExpr`,
so such a predicate falls through to `false` and
`FilterExec::statistics_helper`
takes its default-selectivity branch instead. This PR is about the public
`analyze()` contract, which #19264 reports against directly.
## What changes are included in this PR?
- `datafusion/physical-expr/src/intervals/cp_solver.rs`:
- `propagate_comparison` for `Operator::Eq` with `parent ==
Interval::FALSE`:
the arm previously returned `Ok(None)` for all cases, which the caller
(`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's interval.
A single interval cannot represent that excluded point, so returning the
children unchanged is a safe over-approximation — except when equality is
provably true for the two singleton operands under the comparison
semantics, in which case `a = b` is certainly true and `NOT(a = b)` is
genuinely infeasible.
- Added `singleton_values_equal` helper: compares two singleton
`ScalarValue`s using SQL comparison semantics. `ScalarValue::PartialEq`
is bit-wise for floats (`to_bits`), under which `-0.0 != +0.0`; but
SQL/IEEE-754 comparison treats them as equal. The helper uses
`normalize_float_zero_scalar` (the same normalization DataFusion applies
in `physical-expr-common/src/datum.rs` for runtime comparison) before
structural equality, so the guard correctly identifies `[-0.0,-0.0]` and
`[+0.0,+0.0]` as certainly-equal singletons.
- Added 4 direct unit tests that call `propagate_comparison` directly,
forcing the `Eq + FALSE` branch:
- `test_propagate_eq_false_identical_singletons`: `[0,0]` vs `[0,0]` →
`None` (nearest-invalid boundary).
- `test_propagate_eq_false_signed_zero_singletons`: `[-0.0,-0.0]` vs
`[+0.0,+0.0]` → `None` (Float32 and Float64). This case is reachable
in nested boolean contexts (see E2E test below).
- `test_propagate_eq_false_distinct_singletons`: `[0,0]` vs `[1,1]` →
`Some` (nearest-valid boundary).
- `test_propagate_eq_false_overlapping_intervals`: `[-1,1]` vs `[0,0]` →
`Some` (the #19264 case), `[-1,1]` vs `[0,2]` → `Some`.
- `datafusion/physical-expr/src/analysis.rs`:
- Added 3 E2E regression tests via `analyze()`:
`test_analyze_not_eq_around_zero` (input `[-1, 1]`, expects `Some([-1,
1])`),
`test_analyze_not_eq_clamped_at_zero` (input `[0, 0]`, expects `None`),
and `test_analyze_not_eq_nested_signed_zero_infeasible` (nested
`NOT(a = +0.0) AND b` with `a ∈ [-0.0,-0.0]`, `b ∈ [false,true]` →
expects infeasible). The nested test proves that the case-(1)
short-circuit in `update_ranges` does not protect the production arm:
the bottom-up pass evaluates `a = +0.0` over `[-0.0,-0.0]` as `FALSE`
(because `Interval::equal` uses structural equality), so `NOT` becomes
`TRUE`, the root becomes `TRUE_OR_FALSE`, and top-down propagation
forces `a = +0.0` to `FALSE`, reaching
`propagate_comparison(Eq, FALSE, [-0.0,-0.0], [+0.0,+0.0])`.
The guarded production logic:
```rust
Operator::Eq => {
if !left_child.is_unbounded()
&& !right_child.is_unbounded()
&& left_child.lower() == left_child.upper()
&& right_child.lower() == right_child.upper()
&& singleton_values_equal(left_child.lower(), right_child.lower())
{
Ok(None)
} else {
Ok(Some((left_child.clone(), right_child.clone())))
}
}
```
## Are these changes tested?
Yes, at two levels.
`analysis.rs` covers the user-visible `analyze()` behavior. The `[-1, 1]`
case
from the issue does reach the fixed branch: its bounds evaluate to
`TRUE_OR_FALSE`, so `ExprIntervalGraph::update_ranges` runs propagation and
`NotExpr` hands `Eq` a `FALSE` parent. Its `[0, 0]` counterpart does not —
those bounds evaluate to `FALSE`, so `update_ranges` short-circuits to
`Infeasible` before `propagate_constraints` is called.
`cp_solver.rs` therefore adds unit tests calling `propagate_comparison`
directly. These isolate the ordinary identical-singleton guard that the
`[0,0]` `analyze()` test cannot reach, while the nested signed-zero E2E test
separately proves that the production arm is reachable from `analyze()`.
Negative control, run on the pinned code baseline (`6eaca8bfe`) with the
production arm temporarily reverted to `Ok(None)`. The two modified files'
pre-patch production blobs are identical to the final parent (`19f2e85fa`);
intervening upstream changes do not touch them. Each test is split so that
its
baseline verdict is unambiguous — 3 of the 7 fail:
| test | on baseline | role |
| --- | --- | --- |
| `test_analyze_not_eq_around_zero` | **FAILS** — returns `None`, expected
`[-1, 1]` | regression evidence (#19264) |
| `test_analyze_not_eq_nested_signed_zero_infeasible` | passes
(accidentally) | nested reachability + signed-zero guard |
| `test_propagate_eq_false_overlapping_intervals` | **FAILS** — `[-1,1]` vs
`[0,0]` returns `None`, expected `Some` | regression evidence |
| `test_propagate_eq_false_distinct_singletons` | **FAILS** — `[0,0]` vs
`[1,1]` returns `None`, expected `Some` | regression evidence |
| `test_propagate_eq_false_signed_zero_singletons` | passes (accidentally) |
signed-zero guard |
| `test_propagate_eq_false_identical_singletons` | passes | boundary guard |
| `test_analyze_not_eq_clamped_at_zero` | passes | boundary guard |
All 7 pass on this branch.
`test_analyze_not_eq_around_zero` failing on baseline is the direct evidence
that this PR fixes the reported behavior.
The four tests that pass on baseline are boundary guards, not evidence: the
baseline returns `None` for every `Eq + FALSE` input, so it agrees with them
by accident rather than by identifying the singleton-equal case. They are
included to pin the cases that must stay infeasible. The mutation control
below is what gives the two signed-zero guards their teeth.
`test_analyze_not_eq_nested_signed_zero_infeasible` deserves special note:
it passes on baseline (which reports `None` for all `Eq + FALSE`), but a
guard using structural `ScalarValue::PartialEq` instead of
`singleton_values_equal` would make it **fail** — the nested `AND` defeats
the case-(1) short-circuit in `update_ranges`, so propagation reaches the
`Eq + FALSE` arm with `[-0.0,-0.0]` vs `[+0.0,+0.0]`, and structural equality
treats them as distinct, producing a false feasible result. This was verified
by temporarily replacing `singleton_values_equal` with `==` and confirming
both this test and `test_propagate_eq_false_signed_zero_singletons` fail.
Note on signed zero: `Interval::equal`/`Interval::intersect` use structural
`ScalarValue::PartialEq`, which is bit-wise for floats (`to_bits`), so
`[-0.0,-0.0]` and `[+0.0,+0.0]` are distinct at the interval-algebra level
even though SQL comparison treats them as equal. This divergence is
pre-existing and not fixed by this PR. The guard added here uses
`singleton_values_equal` (which normalizes signed zero via
`normalize_float_zero_scalar`, the same function used in
`physical-expr-common/src/datum.rs` for runtime comparison) so that the
`Eq + FALSE` infeasibility check is consistent with the `Eq` operator's
actual comparison semantics. The standalone `NOT(a = 0.0)` over
`a ∈ [-0.0,-0.0]` case remains unchanged before and after this PR (the
bottom-up pass short-circuits before reaching this arm); only the nested
case, where top-down propagation forces `Eq` to `FALSE`, is affected.
Full local verification:
- `cargo fmt --all -- --check`: clean
- `cargo clippy --all-targets --all-features -- -D warnings`: clean
- `./dev/rust_lint.sh`: exit 0
- extended workspace test suite required by `AGENTS.md` (with `ulimit -n
8192`): pass
- `cargo test -p datafusion-physical-expr --lib`: 1602 passed, 0 failed, 2
ignored
- `cargo test -p datafusion-physical-optimizer --lib`: 33 passed, 0 failed
- `cargo test -p datafusion-physical-plan --lib pruning`: 120 passed, 0
failed
- `cargo test -p datafusion-sqllogictest --test sqllogictests --
simplify_predicates`: pass
All listed lint and test gates, including the extended workspace suite, were
rerun on SHA `03e6d0ad0`. A subsequent upstream rebase added only the dfbench
statistics command and changed neither modified production blob; fmt and the
full physical-expr suite were rerun on final SHA `04acb8d40`.
## Are there any user-facing changes?
No public API changes. Behavior fix: `analyze()` no longer reports `None`
(infeasible) for a satisfiable `NOT(a = b)` predicate over an interval that
contains `b` but is not equal to it. The only case that remains infeasible
under `Eq + FALSE` is when both children are singletons equal under the
comparison semantics (including `-0.0` and `+0.0`).
--
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]
