You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@datafusion.apache.org by "sunchao (via GitHub)" <gi...@apache.org> on 2026/08/26 05:18:02 UTC

[PR] fix: preserve aggregate result identity during exchange reuse [datafusion-comet]

sunchao opened a new pull request, #5470:
URL: https://github.com/apache/datafusion-comet/pull/5470

   ## Which issue does this PR close?
   
   No issue is automatically closed. This is a standalone aggregate identity fix with a regression added below.
   
   ## Rationale for this change
   
   Different aggregate result projections can currently have the same Comet plan identity. For example, an exchange above `COUNT(*) + 1` can be considered reusable for `COUNT(*) - 1`, even though the outputs differ. Distinct aggregates also need Spark's original result attributes to canonicalize equivalent plans correctly.
   
   ## What changes are included in this PR?
   
   - Preserve Spark's original `aggregateAttributes` in Comet hash and object-hash aggregate conversion.
   - Include those attributes in canonicalization and produced attributes, and include aggregate attributes and result expressions in equality and hashing.
   - Add four cases to `CometAggregateSuite`: count, distinct count, distinct count plus sum, and collect-set size. They distinguish different result projections while requiring equivalent projections with fresh aliases/IDs to remain reusable.
   
   Only the aggregate implementation and its existing test suite change. No join feature, native operator, or dependency changes are included.
   
   ## How are these changes tested?
   
   **Draft: JVM validation is blocked, so no passing JVM test result is claimed.**
   
   - Built the native library from public main `5baa6b03`; a subsequent locked build also passed.
   - Spotless checks on the final public patch and `git diff --check` passed.
   - Attempted the focused public Spark 4.0 test run below. Maven dependency resolution stalled on a configured transitive repository before Spark compilation or these tests ran. Repository configuration was left unchanged.
   
   ```sh
   ./mvnw test -Pspark-4.0 -Dtest=none \
     -Dsuites=org.apache.comet.exec.CometAggregateSuite \
     '-Dtests=aggregate canonicalization preserves result expressions and equivalent reuse'
   ```
   
   The new regressions and affected aggregate tests still need runtime validation before this draft is ready to merge.
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: preserve aggregate result identity during exchange reuse [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.

andygrove commented on PR #5470:
URL: https://github.com/apache/datafusion-comet/pull/5470#issuecomment-5610598437

Apologies for not reviewing before this PR was merged, but it was only marked as ready for review yesterday.

I went through it after the fact and the fix itself looks right to me. I checked out the merge commit and reverted `operators.scala` back to its parent while keeping the new tests, and exactly the five new tests fail, all of them on wrong answers rather than on plan assertions, so the regressions really are targeting the bug. I also tried removing just the `allAttributes` override while keeping the equals change. The same five fail, but this time on `finalAggregate(plus).sameResult(finalAggregate(same))` rather than on results, which confirms both halves are pulling their weight. The equals change buys correctness and the `allAttributes` override buys back the legitimate reuse. That override is also a character for character match of `HashAggregateExec.allAttributes` in Spark 4.1, so no concerns there either.

What did come out of the review is that the same defect is still live in four other operators, and I have filed #5824 for it. `CometHashJoinExec`, `CometBroadcastHashJoinExec` and `CometSortMergeJoinExec` all carry `joinType` as a constructor field and all leave it out of `equals` and `hashCode`. `CometBroadcastNestedLoopJoinExec` does include it, which makes me think the other three were simply missed. `LeftSemi` and `LeftAnti` have identical output, keys, condition and build side, so without `joinType` they canonicalize to the same plan. Usually the semi join's inferred `isnotnull(key)` makes the left subtrees differ and hides it, but writing the null check explicitly in both branches is enough to remove that protection. At that point an `EXISTS` / `NOT EXISTS` union returns the `EXISTS` rows twice under Comet and the correct rows under Spark, and it reproduces with all three join strategies.

`CometExplodeExec` has the same shape of problem. It writes `op.outer` into the protobuf but never stores it on the case class, so `equals` cannot see it, and `output` does not disambiguate because plain `explode` over an array with `containsNull = true` is already nullable. `InferFiltersFromGenerate` hides this one, but only when the generator input is a bare attribute, so `explode(s.arr)` against `explode_outer(s.arr)` collides on stock configuration.

Since you already have the context loaded, would you be up for picking up #5824 as a follow-up? The fixes look like about a line each. The other thing I would like to get out of it is a guard so we stop rediscovering this. A test that reflects over every `CometNativeExec` subclass and asserts that each constructor parameter is either referenced in `equals` or named on an explicit exclusion list would have caught all three of these, including this one, before they shipped. Does that seem worth doing to you?

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: preserve aggregate result identity during exchange reuse [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao commented on PR #5470:
URL: https://github.com/apache/datafusion-comet/pull/5470#issuecomment-5594785287

Thanks @viirya. Addressed the test and validation feedback in `2ea7ed82c`:

- Added one AQE-enabled COUNT case to the existing regressions. It checks different final projections against Spark and explicit expected rows, requires an adaptive plan, and checks that equivalent aliases reuse an exchange above the native Final aggregate. The plan checks now traverse adaptive/query-stage wrappers.
- Refreshed the validation section to distinguish the current commit's local results from historical CI on `b448894b`. All five committed regressions pass on Spark 3.4.3 / JDK 17 and Spark 4.1.3 / JDK 21, for 10 passing tests. Hosted CI for the new commit is pending.

I kept `override lazy val allAttributes` in the shared implementation for Spark 3 compatibility. Both [Spark 3.4.3](https://github.com/apache/spark/blob/v3.4.3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala#L614) and [Spark 3.5.9](https://github.com/apache/spark/blob/v3.5.9/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala#L579) declare the inherited member as a `lazy val`; Scala rejects overriding it with a `def` (confirmed with Scala 2.12 and 2.13 compiler probes). I agree with the memory-retention rationale in newer Spark. Adopting `def` only on Spark 4 would require a version-specific implementation, so I left that outside this narrow correctness fix.

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: preserve aggregate result identity during exchange reuse [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao commented on PR #5470:
URL: https://github.com/apache/datafusion-comet/pull/5470#issuecomment-5594332580

   cc @andygrove @viirya @comphead 


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: preserve aggregate result identity during exchange reuse [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao commented on PR #5470:
URL: https://github.com/apache/datafusion-comet/pull/5470#issuecomment-5609139650

   Merged, thanks @viirya !


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] fix: preserve aggregate result identity during exchange reuse [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.

sunchao merged PR #5470:
URL: https://github.com/apache/datafusion-comet/pull/5470


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org