tlfua opened a new pull request, #7566: URL: https://github.com/apache/datafusion-comet/pull/7566
## Which issue does this PR close? Closes #6906. ## Rationale for this change `CometExecRule.canAggregateBeConverted` is the check that `tagUnsafePartialAggregates` uses to decide whether a buffer-producing aggregate needs the `COMET_UNSAFE_PARTIAL` tag, and its scaladoc says it has to mirror the predicate checks in `CometBaseAggregate.doConvert`. #4035 added a guard for non-default collated grouping keys to `doConvert` only, so `canAggregateBeConverted` reported an aggregate grouped on a collated string as convertible while `doConvert` rejects it. As the issue explains, this is latent today, but once reachable the tagging pass would replace the specific collation fallback reason with the generic "Partial aggregate disabled" one. ## What changes are included in this PR? - `operators.scala`: new `CometBaseAggregate.unsupportedGroupingReason`, which holds the map-typed and collated grouping key checks with their existing fallback reasons. `doConvert` calls it, with the same check order and reasons as before. - `CometExecRule.scala`: `checkAggregateConversion` calls the same helper instead of repeating only the map-type check, and the scaladoc notes which checks are now shared. - `pr_build_linux.yml` / `pr_build_macos.yml`: list the new suite. ## How are these changes tested? New `spark/src/test/spark-4.x/org/apache/comet/rules/CometAggregateCollationSuite.scala` (Spark 4.x only, since collated strings do not exist in Spark 3.x). End-to-end SQL cannot reach the gap, because Comet declines the sort that Spark places under a collated grouping key, so the suite plans the query with Comet disabled and calls `canAggregateBeConverted` on the partial aggregate directly: - A `UTF8_LCASE` grouping key is rejected. This fails without the fix, because a collated attribute serializes through `exprToProto`. - A default `UTF8_BINARY` grouping key is still accepted, so the rejection comes from the collation guard. Locally, `make format` (which compiles main and test sources for the default Spark 4.1 profile) succeeded, and `dev/ci/check-suites.py`, `dev/ci/check-ci-config.py`, and `dev/ci/check-versioned-source-parity.py` passed. The suite itself runs in CI. -- 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]
