ulysses-you commented on code in PR #57460:
URL: https://github.com/apache/spark/pull/57460#discussion_r3642367977
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala:
##########
@@ -589,6 +589,59 @@ class DataFrameAggregateSuite extends SharedSparkSession
Row(null, null, null, null, null))
}
+ test("SPARK-58291: empty-buffer merge must not overflow to NaN for
statistical aggregates") {
+ // A single-partition group with two equal, very large finite values has
zero variance, so
+ // var_pop / covar_pop / regr_sxy must be 0.0. Previously, when adjacent
Partial/Final
+ // aggregates were NOT combined (the old default), the Final merge of the
non-empty Partial
+ // buffer into the empty Final buffer computed `delta * deltaN * n1 * n2`
where `n1 == 0`;
+ // `delta * deltaN` overflowed to Infinity and `Infinity * 0 = NaN`,
corrupting the moments.
+ // CombineAdjacentAggregation (Complete mode) sidesteps the merge and
returned 0.0, so the two
+ // configurations disagreed. The merge fix makes both paths return 0.0.
+ // This must hold with and without AQE, and with combining on and off.
+ Seq(true, false).foreach { aqe =>
+ withSQLConf(SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> aqe.toString) {
+ val df = Seq(1e155, 1e155).toDF("a").repartition(1)
Review Comment:
Thanks, good call. Added `MergeEmptyBufferSuite` (Catalyst) that drives the
merger projection directly with the empty buffer on each side --
`merger(joiner(empty, populated))` and `merger(joiner(populated, empty))` --
for `var_pop`, `covar_pop`, and `corr`, under both `CODEGEN_ONLY` and
`NO_CODEGEN` via `TestWithAndWithoutCodegen`. Merging an empty buffer is an
identity, so it asserts the populated buffer is returned unchanged, which also
proves nothing overflowed to `Infinity`/`NaN`.
Mutation-tested both ways: reverting the guard fails all six cases;
isolating just the `isEmptyRight` assertion (the branch `repartition(1)` never
reaches) still fails on the reverted code (`xMk`/`m2` come back `NaN`),
confirming that branch is now independently covered.
I kept the fix in a separate commit and added the tests in a follow-up
commit so the diff is easy to re-review.
--
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]
