andygrove opened a new pull request, #6272: URL: https://github.com/apache/datafusion-comet/pull/6272
Backport of #5470 to `branch-1.0`. Cherry-picked from `c8ee6aef50dcb4d4f8592dec4d264f6a81a4a0c9`. The fix itself is unchanged. Two import lines needed adapting; see "What changes are included" below. ## Which issue does this PR close? None. The original doesn't close an issue either. Listed in #6201. ## Rationale for this change The bug ships in 1.0.0. `CometHashAggregateExec.equals` and `hashCode` on `branch-1.0` are the same as on `main` before #5470: they compare the grouping and aggregate expressions but not `resultExpressions`. Canonicalization erases attribute names and expression IDs and drops the serialized native plan. So two final aggregates over the same input that differ only in their result expressions, such as `COUNT(*) + 1` and `COUNT(*) - 1`, compare equal. Spark's exchange reuse then serves both from one shuffle, and the query silently returns the first aggregate's rows for both. That is `ReuseExchangeAndSubquery` without AQE and the stage cache with AQE; on `branch-1.0` both see the Comet plan. This happens with default configs on every Spark version. ## What changes are included in this PR? The fix is the original one; see #5470 for the details: - `CometHashAggregateExec` gains the aggregate's `aggregateAttributes`. - `equals` and `hashCode` include `resultExpressions` and `aggregateAttributes`. - `allAttributes` and `producedAttributes` follow Spark's aggregate canonicalization, so aggregates that differ only in expression IDs or output aliases still compare equal and still share a shuffle. The adaptations: - The `operators.scala` import from `catalyst.expressions.aggregate` keeps `First` and `Last`, which `branch-1.0` still uses. On `main`, #5041 removed them before #5470. - `CometAggregateSuite` imports `Final`, which the new test uses. On `main` an earlier commit had already imported it. ## How are these changes tested? The new test in `CometAggregateSuite`, run locally on `branch-1.0` with the default Spark 4.1 profile and JDK 17: - All 5 cases pass. - The bug is present on `branch-1.0`, and the test catches it. With `operators.scala` reverted and the test kept, all 5 cases fail. For `COUNT(*)`, Spark returns `[1,0]` and `[3,0]`, and Comet returns `[3,0]` twice. - The full `CometAggregateSuite` passes, 93 tests. - `CometTPCDSV1_4_PlanStabilitySuite` and `CometTPCDSV2_7_PlanStabilitySuite` pass, 129 tests, so no TPC-DS golden plan changes. - Scalastyle, through `test-compile` on the default profile, Spotless, and scalafix in CHECK mode on Spark 3.5 pass. I ran them once with all six `branch-1.0` backports from this round applied together. ## Are there any user-facing changes? Queries that combine aggregates differing only in their result expressions now return correct results. Equivalent aggregates still share one shuffle. There are no config or API changes. -- 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]
