fix: Prevent mixed Spark/Comet partial/final aggregates

[andygrove]

Which issue does this PR close?

Closes #1389

Rationale for this change

Prevent runtime errors. The issue is explained very well in #1389.

What changes are included in this PR?

Add new tagUnsupportedPartialAggregates method in CometExecRule that will look for unsupported final aggregates and then tag the corresponding partial aggregate with a fallback reason to prevent it from being converted to Comet.

The opposite scenario (supported partial, unsupported final) is already handled in CometHashAggregateExec. In a future PR I will move that logic into CometExecRule as well.

How are these changes tested?

New unit tests in CometExecRuleSuite.

There are tests for partial/final pair both in the same query stage and split across query stages.

[andygrove]

@EmilyMatt fyi

[EmilyMatt]

I still believe this is the wrong approach, but I do support having a fix rather than none^^

[codecov-commenter]

Codecov Report

❌ Patch coverage is 68.57143% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.78%. Comparing base (f09f8af) to head (d8ed6ee).
⚠️ Report is 759 commits behind head on main.

Files with missing lines	Patch %	Lines
...n/scala/org/apache/comet/rules/CometExecRule.scala	68.25%	7 Missing and 13 partials ⚠️
...n/scala/org/apache/spark/sql/comet/operators.scala	71.42%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2872      +/-   ##
============================================
+ Coverage     56.12%   60.78%   +4.65%     
- Complexity      976     1505     +529     
============================================
  Files           119      167      +48     
  Lines         11743    16150    +4407     
  Branches       2251     2807     +556     
============================================
+ Hits           6591     9817    +3226     
- Misses         4012     4993     +981     
- Partials       1140     1340     +200     

☔ 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.

[andygrove]

Note that this is also a performance optimization in the case where the rule is applied to a plan multiple times (which does happen with AQE)

[andygrove]

I still believe this is the wrong approach, but I do support having a fix rather than none^^

Understood. We at least have tests now that will be helpful if/when we want to implement optimizations to support partial Comet and Spark final.

[andygrove]

I am putting this on hold for now because I have a better understanding of some of the points in the original issue, and I have learned more from another issue that I have been working on. I will add notes on the issue.

[comphead]

It might be related to #2870

[andygrove]

I am putting this on hold for now because I have a better understanding of some of the points in the original issue, and I have learned more from another issue that I have been working on. I will add notes on the issue.

@parthchandra @comphead @mbutrovich This is now ready for review. This PR is step 1 of TBD (probably 2-3).

With this PR, Comet consistently falls back if either partial or final agg cannot be converted. We were already doing this in some cases, and this PR makes it more consistent and adds tests.

Note that Comet is falling back even when it isn't necessary. This PR doesn't change that, so there will be a future PR to fall back only if there is actually an incompatibility between Comet/Spark for the intermediate aggregation buffer (such as with bloom filters and ANSI mode numeric aggregates currently), rather than always falling back.