andygrove commented on code in PR #6451:
URL: https://github.com/apache/datafusion-comet/pull/6451#discussion_r4149594695
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -929,7 +929,27 @@ private[comet] object RegrSparkVersions {
* variable (y) and `child2` is the independent variable (x), matching the
native accumulator's
* `regr_*(y, x)` convention.
*/
-trait CometRegrBase {
+trait CometRegrBase[T <: AggregateFunction] extends
CometAggregateExpressionSerde[T] {
+
+ /** The SQL function or functions this serde implements, named in the
incompatibility note. */
+ protected def sqlFunctions: String
+
+ // The native merge (`variance_merge` and `covariance_merge` in welford.rs)
orders its
+ // floating-point operations differently from Spark's CentralMomentAgg and
Covariance. Merging
+ // the first partial buffer into the zero-initialized final buffer can leave
a one-ULP error on
+ // the mean, so a constant variable ends up with a tiny non-zero m2 and
Spark's exact `m2 == 0`
+ // degenerate-case checks never fire. Porting Spark's merge order would make
these Compatible.
Review Comment:
Yes, that's the plan. This is the stopgap for 1.1.0, and #6076 is the real
fix. If this lands first, #6076 can drop the `Incompatible` override along with
its merge change, since its new tests are the #6423 reproducer. If #6076 lands
first, I'll retarget this to branch-1.1 only.
On `corr` and `covar_*`: you're right that they share the merge. I ran the
#6423 data through them on 1.0.0 and 1.1.0-rc1. `covar_pop`, `covar_samp`,
`var_*` and `stddev_*` return values around 1e-17 or 1e-35 where Spark returns
0.0, and `corr` returns 0.878 where Spark returns NULL, or fails with
DIVIDE_BY_ZERO under ANSI. The two releases give identical values, because
these functions already ran natively with the same `welford.rs` merge in 1.0.0.
So they aren't 1.1.0 regressions, and making them fall back now would slow down
every such query to fix an edge case that already shipped. #6076 ports Spark's
merge order, which should fix them on main along with `regr_*`. I filed #6481
to track them.
--
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]
