Skip to content

fix: correct missingInput for CometHashAggregateExec#2409

Merged
mbutrovich merged 1 commit intoapache:mainfrom
comphead:dev2
Sep 17, 2025
Merged

fix: correct missingInput for CometHashAggregateExec#2409
mbutrovich merged 1 commit intoapache:mainfrom
comphead:dev2

Conversation

@comphead
Copy link
Contributor

@comphead comphead commented Sep 17, 2025

Which issue does this PR close?

Closes #2214 .

I'm not sure if unit test is needed but here is a proof

== Parsed Logical Plan ==
'Aggregate ['id], [unresolvedalias('count('id), None)]
+- 'UnresolvedRelation [t2], [], false

== Analyzed Logical Plan ==
count(id): bigint
Aggregate [id#43], [count(id#43) AS count(id)#48L]
+- SubqueryAlias t2
   +- View (`t2`, [id#43,value#44])
      +- Relation [id#43,value#44] parquet

== Optimized Logical Plan ==
Aggregate [id#43], [count(id#43) AS count(id)#48L]
+- Project [id#43]
   +- Relation [id#43,value#44] parquet

== Physical Plan ==
*(1) CometColumnarToRow
+- CometHashAggregate [id#43, count#51L], Final, [id#43], [count(id#43)]
   +- CometExchange hashpartitioning(id#43, 10), ENSURE_REQUIREMENTS, CometNativeShuffle, [plan_id=140]
      +- CometHashAggregate [id#43], Partial, [id#43], [partial_count(id#43)]
         +- CometScan [native_iceberg_compat] parquet [id#43] Batched: true, DataFilters: [], Format: CometParquet, Location: InMemoryFileIndex(1 paths)[file:/tmp/test_11], PartitionFilters: [], PushedFilters: [], ReadSchema: struct<id:int>

Before the fix it was

== Physical Plan ==
*(1) CometColumnarToRow
+- !CometHashAggregate [id#43, count#51L], Final, [id#43], [count(id#43)]
   +- CometExchange hashpartitioning(id#43, 10), ENSURE_REQUIREMENTS, CometNativeShuffle, [plan_id=140]
      +- !CometHashAggregate [id#43], Partial, [id#43], [partial_count(id#43)]
         +- CometScan [native_iceberg_compat] parquet [id#43] Batched: true, DataFilters: [], Format: CometParquet, Location: InMemoryFileIndex(1 paths)[file:/tmp/test_11], PartitionFilters: [], PushedFilters: [], ReadSchema: struct<id:int>

Rationale for this change

What changes are included in this PR?

How are these changes tested?

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 57.52%. Comparing base (f09f8af) to head (e2b0253).
⚠️ Report is 512 commits behind head on main.

Files with missing lines Patch % Lines
...n/scala/org/apache/spark/sql/comet/operators.scala 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #2409      +/-   ##
============================================
+ Coverage     56.12%   57.52%   +1.39%     
- Complexity      976     1295     +319     
============================================
  Files           119      147      +28     
  Lines         11743    13470    +1727     
  Branches       2251     2352     +101     
============================================
+ Hits           6591     7748    +1157     
- Misses         4012     4457     +445     
- Partials       1140     1265     +125

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mbutrovich
mbutrovich approved these changes Sep 17, 2025
View reviewed changes
Copy link
Contributor

@mbutrovich mbutrovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @comphead! I'm curious if we have any other node types that require this change. Actually, I'm surprised this didn't require new golden plans, assuming there were some hash aggregates in there.

@mbutrovich mbutrovich merged commit 9f95e8b into apache:main Sep 17, 2025
96 checks passed
mbutrovich added a commit to mbutrovich/datafusion-comet that referenced this pull request Sep 20, 2025
@mbutrovich
fix: new plans after apache#2409.
a9738c0
@mbutrovich mbutrovich mentioned this pull request Sep 20, 2025
Merged
mbutrovich added a commit that referenced this pull request Sep 20, 2025
@mbutrovich
chore: new TPC-DS golden plans after #2409 (#2426)
a4c437c
coderfender pushed a commit to coderfender/datafusion-comet that referenced this pull request Dec 13, 2025
@comphead
fix: correct missingInput for CometHashAggregateExec (apache#2409)
7d5903f
coderfender pushed a commit to coderfender/datafusion-comet that referenced this pull request Dec 13, 2025
@mbutrovich
chore: new TPC-DS golden plans after apache#2409 (apache#2426)
d9c38e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbutrovich mbutrovich mbutrovich approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CometHashAggregate prefixed with ! in explain plan

3 participants

@comphead @codecov-commenter @mbutrovich