andygrove opened a new pull request, #6451: URL: https://github.com/apache/datafusion-comet/pull/6451
## Which issue does this PR close? Closes #6423. Found by the 1.1.0 regression audit (#6399) and tracked in #6402. ## Rationale for this change #4775 made `regr_slope`, `regr_intercept`, `regr_r2`, `regr_sxx`, `regr_syy` and `regr_sxy` run natively by default. Their native merge of partial aggregates (`variance_merge` and `covariance_merge` in `native/spark-expr/src/agg_funcs/welford.rs`) orders its floating-point operations differently from Spark's `CentralMomentAgg` and `Covariance`. Suppose a group's variable is constant at a value binary floating point can't represent exactly, such as 0.1, and the group's rows come from two or more partial aggregates. Then the first merge into the zero-initialized final buffer leaves a one-ULP error on the mean. `m2` ends up around 1e-34 instead of 0, Spark's exact `m2 == 0` checks never fire, and Comet returns plausible-looking wrong values where Spark returns NULL, 0.0 or 1.0. These aggregates ran in Spark in 1.0.0, so this is a 1.1.0 regression. For 1.1.0 I'm taking the smallest fix: mark the regr aggregates Incompatible, so they fall back to Spark by default as they did in 1.0.0, and run natively only when the user opts in. I did not try to port Spark's merge order here. That port is the follow-up that would let these aggregates become Compatible again. It would also fix the multi-partition `var_pop` and `stddev_pop` drift on clustered values, which the same merge order already causes in 1.0.0. ## What changes are included in this PR? - `CometRegrBase` now extends `CometAggregateExpressionSerde` and returns `Incompatible` from `getSupportLevel`. The reason describes the merge-order problem and links #6423, so the fallback reason in the plan names it. `getIncompatibleReasons` returns the same text, so the generated compatibility guide lists it too. This covers all five serdes behind the six functions, because Spark plans both `regr_sxx` and `regr_syy` as `RegrReplacement`. Users can still opt into the native path per expression with `spark.comet.expression.<name>.allowIncompatible=true`, where `<name>` is `RegrSlope`, `RegrIntercept`, `RegrR2`, `RegrSXY` or `RegrReplacement`. - `regr.sql` opts in to all five keys, so its queries keep checking the native path. - A new `CometAggregateSuite` test built from the issue's reproducer. - The six rows in `expressions.md` now say these aggregates fall back by default, and give each one's opt-in key. ## How are these changes tested? The new test, "regression aggregates fall back to Spark by default", is the issue's reproducer. It writes two Parquet files with `x` constant at 0.1, runs the issue's seven expressions with default configs, and compares the answer with Spark's. It also checks that each of the five serdes records a fallback reason naming its opt-in key and #6423. Without the serde change the test fails the way the issue describes, with `CometHashAggregate` in both the partial and the final stage. Spark returns `[null,null,null,0.0,0.0,1.0,0.0]` and Comet returns `[2.16e17,-2.16e16,0.7714,2.89e-34,6.2e-17,0.7714,2.89e-34]`. With the change the test passes. I ran these locally on the default Spark 4.1 profile: - `./mvnw test -Dtest=none -Dsuites="org.apache.comet.exec.CometAggregateSuite"`: 128 passed, 2 ignored (existing ignores). - `./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite regr"`: passed. `regr.sql` still checks that its queries run natively, now through the opt-in. - `./mvnw spotless:check`, and the scalafix check on Spark 3.5 and Scala 2.12. I also ran `GenerateDocs` against a scratch copy of the user guide to check the new entries on the aggregate compatibility page. -- 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]
