Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Aggregation without aggregation expressions should use correct result expressions #175

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

viirya
Copy link
Member

@viirya viirya commented Mar 9, 2024

Which issue does this PR close?

Closes #176.

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Comment on lines +1786 to +1791
val attributes = groupingExpressions.map(_.toAttribute) ++ aggregateAttributes
val resultExprs = resultExpressions.map(exprToProto(_, attributes))
if (resultExprs.exists(_.isEmpty)) {
emitWarning(s"Unsupported result expressions found in: ${resultExpressions}")
return None
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Even aggregateExpressions is empty, we still need to assign result expressions. This is the latest bug I found during fixing TPCDS query failures in SortMergeJoin work.

@viirya viirya changed the title fix: Aggregation without aggregation functions should use correct result expressions fix: Aggregation without aggregation expressions should use correct result expressions Mar 9, 2024
@viirya
Copy link
Member Author

viirya commented Mar 9, 2024

cc @sunchao @huaxingao

Copy link
Member

@sunchao sunchao left a comment

Choose a reason for hiding this comment

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

LGTM (pending CI)

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 33.29%. Comparing base (e83635a) to head (8bed4b3).
Report is 2 commits behind head on main.

Files Patch % Lines
.../scala/org/apache/comet/serde/QueryPlanSerde.scala 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main     #175   +/-   ##
=========================================
  Coverage     33.29%   33.29%           
  Complexity      766      766           
=========================================
  Files           107      107           
  Lines         35379    35385    +6     
  Branches       7657     7658    +1     
=========================================
+ Hits          11778    11781    +3     
- Misses        21155    21157    +2     
- Partials       2446     2447    +1     

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

@viirya viirya merged commit 488c523 into apache:main Mar 9, 2024
20 checks passed
@viirya
Copy link
Member Author

viirya commented Mar 9, 2024

Merged. Thanks.

@viirya viirya deleted the fix_empty_agg branch March 9, 2024 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect output when aggregation without aggregation functions
3 participants