jayzhan211 commented on PR #25207:
URL: https://github.com/apache/datafusion/pull/25207#issuecomment-5679980853
@wudidapaopao , there is one issue I think is better to fix this before merge
`ScalarValue::try_cmp` orders floats with `total_cmp`, where `-0.0 < 0.0`,
but the executor normalizes `-0.0` to `0.0` before comparing
(`normalize_float_zero_scalar` in
`datafusion_physical_expr_common::datum::apply_cmp`). So the new checks decide
satisfiability with a different ordering than the filter is evaluated with, and
signed zeros give wrong results.
With a Float64 column holding `0.0`, `-0.0` and `NaN` (baseline verified
with `SET datafusion.optimizer.max_passes = 0`):
| Query | Unoptimized | This PR |
| --- | --- | --- |
| `WHERE f = -0.0 AND f >= 0.0` | the two zero rows | `EmptyRelation`, no
rows |
| `WHERE f = 0.0 AND f != -0.0` | no rows (`f != -0.0` is false for both
zeros) | `f = 0.0`, both zero rows |
| `WHERE f > -0.0 AND f >= 0.0` | only the NaN row | `f >= 0.0`, all three
rows |
The first two are new in this PR. The third is pre-existing in
`find_most_restrictive_predicate` (as is `f = 0.0 AND f = -0.0` folding to
`false`, since `ScalarValue::eq` is bitwise), but the same fix covers it. NaN
is fine: the executor treats NaN as the largest value and `NaN = NaN` as true,
which matches `total_cmp`.
Since this changes returned rows, I think it needs to be fixed here rather
than tracked in a follow-up issue. The fix is small: normalize the literal once
at the grouping site, so `find_most_restrictive_predicate`, `is_empty_range`
and `satisfies_all` all see the value the executor compares. Rewriting the
emitted literal to `+0.0` is semantically identical at runtime.
```rust
use datafusion_common::utils::normalize_float_zero_scalar;
/// The executor folds `-0.0` into `0.0` before comparing floats (see
/// `datafusion_physical_expr_common::datum::apply_cmp`), while
/// `ScalarValue::try_cmp` orders `-0.0` below `0.0`. Fold it here too so the
/// decisions in this module match what the filter will evaluate.
fn normalize_literal(expr: Box<Expr>) -> Box<Expr> {
match *expr {
Expr::Literal(value, metadata) => {
Box::new(Expr::Literal(normalize_float_zero_scalar(value),
metadata))
}
other => Box::new(other),
}
}
```
```diff
column_predicates
.entry(col)
.or_default()
- .push(Expr::BinaryExpr(BinaryExpr { left, op, right
}));
+ .push(Expr::BinaryExpr(BinaryExpr {
+ left,
+ op,
+ right: normalize_literal(right),
+ }));
@@
.push(Expr::BinaryExpr(BinaryExpr {
left: right,
op: swapped_op,
- right: left,
+ right: normalize_literal(left),
}));
```
Please also add a unit test (`col("f").eq(lit(-0.0))` with
`col("f").gt_eq(lit(0.0))` must not become `false`) and an slt case that checks
returned rows, not only the plan, for the three queries above.
--
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]
