andygrove opened a new issue, #6519:
URL: https://github.com/apache/datafusion-comet/issues/6519
### Describe the bug
Native `percentile_approx` over input that holds a NaN with the sign bit set
returns different percentiles from Spark, at every percentage, once a
partition's summary has merged more than one head buffer (Spark's
`QuantileSummaries`, which Comet ports, buffers 50,000 values at a time).
`CometApproxPercentile` reports `Compatible` for `FLOAT` and `DOUBLE`, so the
wrong answer is silent.
On x86-64 every NaN that arithmetic produces has the sign bit set, and
negating any NaN sets it on every platform. Spark-written Parquet only holds
the canonical NaN, so the NaN has to come from an expression.
### Steps to reproduce
```scala
spark.range(0, 300000, 1, 1)
.selectExpr("IF(id = 0, double('NaN'), cast(id as double)) as d")
.write.parquet(path)
spark.read.parquet(path).createOrReplaceTempView("t")
sql("SELECT percentile_approx(IF(isnan(d), -d, d), array(0.0D, 0.25D, 0.5D,
0.75D, 1.0D)) FROM t")
```
| | Result |
| --- | --- |
| Spark | `[1.0, 75001.0, 150000.0, 225013.0, NaN]` |
| Comet (native `CometHashAggregate`) | `[9.0, 74997.0, 149999.0, 224982.0,
299999.0]` |
With the canonical NaN, `percentile_approx(d, ...)`, Comet returns Spark's
result.
### Expected behavior
The same result as Spark.
### Additional context
`QuantileSummaries::with_head_buffer_inserted` in
`native/spark-expr/src/agg_funcs/quantile_summaries.rs` sorts the head buffer
with `f64::total_cmp`, which puts a NaN with the sign bit set before every
other value. Spark sorts it with `headSampled.toArray.sorted`, which for a
`double[]` is `java.util.Arrays.sort` in `java.lang.Double.compare` order, with
every NaN last. The merge that follows compares samples with `<=`, which is
false against NaN, so with the NaN at the front the next head buffer's samples
are merged out of order.
Sorting the head buffer in `Double.compare` order instead makes the kernel
return Spark's result for these 300,000 values, checked by inserting them into
a `QuantileSummaries` in a unit test. #6518 adds that order to
`float_semantics` as `compare_floats_java`. The other sorts in the file order
the requested percentages or test data, not input values.
Part of #6385.
--
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]
