andygrove opened a new issue, #6423:
URL: https://github.com/apache/datafusion-comet/issues/6423
### Describe the bug
`regr_slope`, `regr_intercept`, `regr_r2`, `regr_sxx`, `regr_syy` and
`regr_sxy` run natively by default since #4775. The bug needs two conditions:
- a group's variable is constant at a value that binary floating point can't
represent exactly, such as 0.1
- the group's rows are merged from two or more partial aggregates
When both hold, Comet returns plausible-looking wrong values where Spark
returns NULL or 0.0. In 1.0.0 these aggregates ran in Spark, so this is a
regression in 1.1.0. It reproduces on 1.1.0-rc1 and on `main`.
### Steps to reproduce
```scala
// In a suite extending CometTestBase, default configs
spark.range(0, 6, 1, 2)
.selectExpr("CAST(id AS DOUBLE) AS y", "0.1D AS x")
.write
.parquet(path) // two files, so two scan partitions
withParquetTable(path, "t") {
checkSparkAnswer(
"SELECT regr_slope(y, x), regr_intercept(y, x), regr_r2(y, x),
regr_sxx(y, x), " +
"regr_sxy(y, x), regr_r2(x, y), regr_syy(x, y) FROM t")
}
```
### Expected behavior
Spark and Comet 1.0.0 return `NULL, NULL, NULL, 0.0, 0.0, 1.0, 0.0`.
Comet 1.1.0-rc1, with `CometHashAggregate` in both the partial and the final
stage, returns `2.16e17, -2.16e16, 0.7714, 2.89e-34, 6.2e-17, 0.7714, 2.89e-34`
(Spark 4.1 profile).
### Additional context
The merge in `native/spark-expr/src/agg_funcs/welford.rs` (`variance_merge`,
`covariance_merge`) uses a different floating-point operation order from
Spark's `CentralMomentAgg` and `Covariance`. The first merge into the
zero-initialized final buffer leaves a one-ULP error on the mean. As a result
`m2` ends up around 1e-34 instead of exactly 0, and Spark's exact `m2 == 0`
checks for the degenerate cases never fire.
The same merge order already makes multi-partition `var_pop` and
`stddev_pop` drift on clustered values in 1.0.0. The tests in #4775 use
whole-number constants and a tolerance, which hide it.
For 1.1.0 the smallest fix is to mark the `regr_*` aggregates Incompatible,
so they fall back to Spark as they did in 1.0.0. Porting Spark's merge order is
the full fix, and it would also fix `var_pop` and `stddev_pop`.
Found by the 1.1.0 regression audit (#6399) and tracked in #6402.
--
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]
