[SPARK-58291][SQL] Guard statistical-aggregate merge against empty-buffer overflow to NaN#57460
[SPARK-58291][SQL] Guard statistical-aggregate merge against empty-buffer overflow to NaN#57460ulysses-you wants to merge 1 commit into
Conversation
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 1 nit.
The numerical fix is consistent across the affected aggregates; only the regression-test issue references need correction.
Nits: 1 minor item (see inline comments).
Verification
Traced each changed merge expression through its empty-left, empty-right, and non-empty paths. The guards preserve the existing formula for two non-empty buffers and reduce an empty-side merge to the non-empty buffer. Scanner coverage verified all eight changed behavioral claims.
| Row(null, null, null, null, null)) | ||
| } | ||
|
|
||
| test("SPARK-58210: empty-buffer merge must not overflow to NaN for statistical aggregates") { |
There was a problem hiding this comment.
Please use SPARK-58291 in both new test names. SPARK-58210 points searches and test-failure reports at the wrong issue.
…ffer overflow The Pearson correlation, covariance, and central-moment merge expressions overflow to Infinity and then NaN via `Infinity * 0` when one side is an empty buffer with a large average: `delta * deltaN` (and the `dx`/`dy` variants) overflow before being multiplied by the zero count. Force the delta terms to 0.0 and set the merged average directly to the non-empty side when either buffer is empty, mirroring the guard each other aggregate already applies. Extracted from apache#57363 as a standalone correctness fix, independent of the replaceHashWithSortAgg default flip, so it can be backported on its own. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9b60bf8 to
8d5263e
Compare
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Reviewed commit 8d5263ec82f0a0f84428afa29987d8dd4dae637c across numerical correctness, distributed partial/final aggregation, regression coverage, generated-expression overhead, and aggregate-buffer/checkpoint compatibility.
The empty-buffer guards consistently prevent Infinity * 0 from contaminating central moments, covariance, and Pearson correlation; both empty-buffer directions preserve the populated buffer, while two populated buffers retain the existing merge formulas. The regression cases cover the affected variance, covariance, correlation, and regression functions with AQE and adjacent-aggregation enabled and disabled. Aggregate buffer schemas and public APIs are unchanged.
No blocking findings. I left one optional [P3] suggestion to explicitly exercise both empty-buffer merge orderings.
CI note: the exact-head Build was still in progress at approval time; this approval reflects the reviewed code and does not represent a completed CI run.
| // 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) |
There was a problem hiding this comment.
[P3][non-blocking] Consider directly exercising both empty-buffer merge directions.
These end-to-end cases reproduce the empty-left final-merge failure, but repartition(1) does not independently exercise the newly added isEmptyRight branch. A focused Catalyst regression using DeclarativeAggregateEvaluator.merge with a populated buffer and an empty buffer in both orders, ideally under both interpreted and generated execution, would protect both branches without changing the production fix.
3 participants
What changes were proposed in this pull request?
The
mergeexpressions ofPearsonCorrelation(Corr),Covariance, andCentralMomentAggoverflow toInfinityand then toNaNviaInfinity * 0when one side is an empty aggregation buffer whose counterpart holds a large average.Concretely, the merge computes terms like
delta * deltaN * n1 * n2(and thedx * dxN,dy * dyNvariants). When one buffer is empty (n1 == 0orn2 == 0), that term is mathematically zero, butdelta * deltaNcan overflow toInfinitybefore being multiplied by the zero count, andInfinity * 0 = NaNcorrupts the merged moments.This PR forces the
delta/dx/dyterms to0.0when either side is empty, and sets the merged average directly to the non-empty side's average, so the empty-side terms vanish cleanly. This mirrors the guard the other aggregates already apply.This is extracted from #57363 as a standalone correctness fix, independent of the
spark.sql.execution.replaceHashWithSortAggdefault flip, so it can be reviewed, merged, and backported on its own.Why are the changes needed?
var_pop,covar_pop,regr_sxy,corr, andregr_r2can returnNaNfor finite inputs when a partial/final aggregate merge encounters an empty buffer against a large-magnitude average. The correct results are finite (e.g. zero variance for equal values). The bug is only masked when adjacent aggregates are combined intoCompletemode (which skips the merge), so the same query returns different results depending on whether the merge path runs.Does this PR introduce any user-facing change?
Yes. Queries using
corr,covar_pop/covar_samp,var_pop/var_samp,stddev_*,skewness,kurtosis, and theregr_*family that previously returnedNaNfor large finite inputs now return the correct finite result.How was this patch tested?
Two new regression tests in
DataFrameAggregateSuite:SPARK-58291-coveredvar_pop/covar_pop/regr_sxyon two equal large values (expect0.0).corr/regr_r2on two large-magnitude finite points (expect a finite result, notNaN).Both assert results are stable across AQE on/off and
combineAdjacentAggregationon/off, and both fail without the fix on the merge path.catalyst/compileand the new tests pass.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)