andygrove opened a new issue, #6906:
URL: https://github.com/apache/datafusion-comet/issues/6906
### What is the problem the feature request solves?
`CometExecRule.canAggregateBeConverted` is the conservative check that the
`tagUnsafePartialAggregates` pass uses to decide whether a buffer-producing
aggregate needs the `COMET_UNSAFE_PARTIAL` tag. Its scaladoc says it has to
mirror the predicate checks in `CometBaseAggregate.doConvert`
(`operators.scala`), or the tagging pass drifts and either misses a tag or adds
a spurious one.
The two have drifted. #4035 added this guard to `doConvert` but not to
`canAggregateBeConverted`:
```scala
if (groupingExpressions.exists(expr =>
isStringCollationType(expr.dataType))) {
withFallbackReason(aggregate, "Grouping on non-default collated strings is
not supported")
return None
}
```
So `canAggregateBeConverted` reports an aggregate grouped on a non-default
collated string as convertible, when `doConvert` rejects it.
I couldn't make this visible on Spark 4.1. A collated grouping key makes
Spark plan a `SortAggregate` over a `Sort`, and Comet already declines that
sort ("Sort does not support non-default string collation"), so neither
aggregate stage gets a native child. A Parquet scan of a collated column
doesn't run natively either. For example, `SELECT k, abs(avg(v)) FROM (SELECT
CAST(s AS STRING COLLATE UTF8_LCASE) AS k, v FROM t) GROUP BY k` shows the same
fallback reasons with and without `spark.comet.expression.Abs.enabled=false`,
which makes the final aggregate unconvertible for another reason.
So this is latent today. It becomes reachable if Comet starts running sorts
on collated keys natively, or if Spark plans a hash aggregate for such keys.
The tagging pass would then tag a partial aggregate that `doConvert` rejects
anyway. Since `doConvert` checks the tag first, its reason ("Partial aggregate
disabled: corresponding final aggregate cannot be converted to Comet and
intermediate buffer formats are incompatible") would replace the more specific
collation reason.
### Describe the potential solution
Add the same `isStringCollationType` check to `canAggregateBeConverted`.
Better, as its scaladoc suggests, move the checks the two share (map-typed
keys, collated keys, grouping expression conversion) into one predicate that
both call, so the next guard added to `doConvert` can't drift the same way.
### Additional context
Found while reviewing #6899, which removes the "No group by or aggregation"
guard from both places.
--
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]
