ulysses-you commented on code in PR #57363:
URL: https://github.com/apache/spark/pull/57363#discussion_r3618936658
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -3006,18 +3006,17 @@ object SQLConf {
.doc("Whether to replace hash aggregate node with sort aggregate based on
children's ordering")
.version("3.3.0")
.booleanConf
- .createWithDefault(false)
+ .createWithDefault(true)
val COMBINE_ADJACENT_AGGREGATION_ENABLED =
buildConf("spark.sql.execution.combineAdjacentAggregation")
.internal()
.doc("When true, combine adjacent aggregation with `Partial` and `Final`
to `Complete` " +
- "mode. This defaults to the value of
`spark.sql.execution.replaceHashWithSortAgg` since " +
- "combining adjacent aggregation subsumes the partial-and-final merge
that " +
- "`replaceHashWithSortAgg` used to perform on its own.")
+ "mode.")
.version("4.3.0")
.withBindingPolicy(ConfigBindingPolicy.NOT_APPLICABLE)
- .fallbackConf(REPLACE_HASH_WITH_SORT_AGG_ENABLED)
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
Great catch, and thanks for pinpointing the empty-buffer merge. Fixed as a
deliberate correctness change in the merge expressions rather than excluding
the aggregates.
Root cause: `CentralMomentAgg.merge` computes `delta * deltaN * n1 * n2` and
`Covariance.merge` computes `dx * dyN * n1 * n2`. When one side is an empty
buffer (`n1 == 0` or `n2 == 0`) and the other side has a large average, `delta
* deltaN` (or `dx * dyN`) overflows to `Infinity` before being multiplied by
the zero count, and `Infinity * 0 = NaN` corrupts the merged moments. The
existing `newN === 0` guard only covers both-empty. The old default (no
combining) hit this on a single-partition group of two large equal values,
returning `NaN`; the Complete-mode default sidestepped the merge and returned
the correct `0.0`, so the two configs disagreed.
Fix: force `delta`/`dx`/`dy` to `0.0` when either side is empty, and set
`newAvg`/`newXAvg`/`newYAvg` directly to the non-empty side's average. The
delta terms then vanish cleanly and the merged buffer equals the non-empty
side. Both the combined and the separate paths now return `0.0` for
`var_pop`/`covar_pop`/`regr_sxy`.
Added `SPARK-58210: empty-buffer merge must not overflow to NaN for
statistical aggregates` in `DataFrameAggregateSuite`, covering `var_pop`,
`covar_pop`, `regr_sxy` across AQE on/off x combine on/off. The `group-by`,
`linear-regression`, and `aggregates_part1` SQL golden files are unchanged
(regenerated to confirm).
--
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]
