comphead commented on code in PR #6451:
URL: https://github.com/apache/datafusion-comet/pull/6451#discussion_r4146866275
##########
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:
Question on sequencing: #6076 already ports Spark's merge order in
`variance_merge` and `covariance_merge`. Its new `CometAggregateSuite` tests
run `regr_*` natively with default configs, so whichever PR lands second needs
either the five opt-in keys in those tests or a revert of this `Incompatible`.
Is the plan to undo this on `main` once #6076 merges? I'd also expect `corr`
and `covar_*` to share the exposure through the same accumulators, though I
haven't run that.
##########
spark/src/test/scala/org/apache/comet/exec/CometAggregateSuite.scala:
##########
@@ -1815,6 +1815,38 @@ class CometAggregateSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
}
}
+ test("regression aggregates fall back to Spark by default") {
Review Comment:
Nit: would a `query expect_fallback(issues/6423)` in a new SQL file next to
`regr.sql` (without the opt-in header) cover this? The fallback check does not
depend on the two-partition layout, and two `INSERT` statements with `x = 0.1`
would still mirror the reproducer data. It would also drop the five imports
added to this suite.
--
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]
