jayzhan211 commented on code in PR #26718:
URL: https://github.com/apache/datafusion/pull/26718#discussion_r4454465668
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -2782,6 +2782,22 @@ impl ScalarValue {
}
}
+ /// Whether this is a float `0.0` or `-0.0`, including one inside a
+ /// dictionary. The two zeros are equal but sort apart (see
+ /// [`compare_floats`]), so a sort boundary at a zero has to account for
+ /// both.
+ ///
+ /// [`compare_floats`]: crate::float_semantics::compare_floats
+ pub fn is_float_zero(&self) -> bool {
+ match self {
+ ScalarValue::Float16(Some(v)) => f32::from(*v) == 0.0,
+ ScalarValue::Float32(Some(v)) => *v == 0.0,
+ ScalarValue::Float64(Some(v)) => *v == 0.0,
+ ScalarValue::Dictionary(_, inner) => inner.is_float_zero(),
Review Comment:
A run-end-encoded float key still hits the bug, because this match doesn't
look inside `RunEndEncoded`. With two batches this returns `7, 1` on the
current head instead of `5, 7`:
```sql
set datafusion.execution.target_partitions = 1;
create table t as select arrow_cast(column1, 'RunEndEncoded("run_ends":
non-null Int32, "values": Float64)') x, column2 y from (values (0.0, 1), (-0.0,
7));
insert into t select arrow_cast(-0.0, 'RunEndEncoded("run_ends": non-null
Int32, "values": Float64)'), 5;
select y from t order by x, y limit 2;
```
```suggestion
ScalarValue::Dictionary(_, inner)
| ScalarValue::RunEndEncoded(_, _, inner) =>
inner.is_float_zero(),
```
With that change the query returns `5, 7`. Could you add this query to
`topk.slt` and this case to `is_float_zero_matches_both_zeros`?
```rs
ScalarValue::RunEndEncoded(
Field::new("run_ends", DataType::Int32, false).into(),
Field::new("values", DataType::Float64, true).into(),
Box::new(ScalarValue::Float64(Some(-0.0))),
),
```
##########
datafusion/physical-plan/src/topk/mod.rs:
##########
@@ -630,13 +630,21 @@ impl TopK {
break;
}
+ // The heap orders `-0.0` before `0.0`, but comparisons treat them
as
+ // equal (#22835), so `col < 0.0 OR (col = 0.0 AND ...)` would drop
+ // `-0.0` rows that belong in the TopK. For a zero threshold, use a
+ // non-strict bound that keeps both zeros and do not filter on any
+ // later key.
Review Comment:
Optional cleanup: as @mkleen mentioned in the approval, could this comment
mark the special case as temporary so it gets removed along with #26138?
```suggestion
// The heap orders `-0.0` before `0.0`, but comparisons treat
them as
// equal (#22835), so `col < 0.0 OR (col = 0.0 AND ...)` would
drop
// `-0.0` rows that belong in the TopK. For a zero threshold,
use a
// non-strict bound that keeps both zeros and do not filter on
any
// later key. Temporary: remove once -0.0 and 0.0 sort as equal,
// see https://github.com/apache/datafusion/issues/26138
```
##########
datafusion/sqllogictest/test_files/topk.slt:
##########
@@ -811,3 +811,54 @@ drop table nested_progress;
statement ok
set datafusion.execution.target_partitions = 4;
+
+# The TopK dynamic filter must keep `-0.0` rows when the threshold is `0.0`
+# (and vice versa): the heap orders `-0.0` before `0.0`, but comparisons treat
+# them as equal. https://github.com/apache/datafusion/issues/26618
+#
+# The expected rows assume -0.0 sorts before 0.0 (the current order).
+# They will change when -0.0 and 0.0 are treated as equal when sorting,
+# see https://github.com/apache/datafusion/issues/26138
+statement ok
+set datafusion.execution.target_partitions = 1;
+
+statement ok
+create table float_zero (x double, y int) as values (0.0, 1), (-0.0, 7);
+
+statement ok
+insert into float_zero values (-0.0, 5);
+
+# `-0.0` is displayed as `0`; `y` tells the rows apart.
+query RI
+select x, y from float_zero order by x, y;
+----
+0 5
+0 7
+0 1
+
+query RI
+select x, y from float_zero order by x, y limit 2;
+----
+0 5
+0 7
+
+statement ok
+create table float_zero_desc (x double, y int) as values (-0.0, 1), (0.0, 7);
+
+statement ok
+insert into float_zero_desc values (0.0, 5);
+
+query RI
+select x, y from float_zero_desc order by x desc, y limit 2;
Review Comment:
Optional: the scan also receives this filter for file and row-group pruning.
A parquet case with the `-0.0` row in its own file fails on main and passes
with this PR, both with `pushdown_filters` on and off:
```sql
copy (select * from (values (0.0, 1), (-0.0, 7)) t(x, y)) to
'test_files/scratch/topk/float_zero/a.parquet';
copy (select * from (values (-0.0, 5)) t(x, y)) to
'test_files/scratch/topk/float_zero/b.parquet';
create external table float_zero_pq stored as parquet location
'test_files/scratch/topk/float_zero/';
select y from float_zero_pq order by x, y limit 2;
-- expected: 5, 7
```
--
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]
