andygrove commented on code in PR #4565:
URL: https://github.com/apache/datafusion-comet/pull/4565#discussion_r4178224751
##########
spark/src/main/scala/org/apache/spark/sql/comet/operators.scala:
##########
@@ -2430,24 +2434,69 @@ object CometObjectHashAggregateExec
}
}
-case class CometHashAggregateExec(
- override val nativeOp: Operator,
- override val originalPlan: SparkPlan,
- override val output: Seq[Attribute],
- groupingExpressions: Seq[NamedExpression],
- aggregateExpressions: Seq[AggregateExpression],
- aggregateAttributes: Seq[Attribute],
- resultExpressions: Seq[NamedExpression],
- input: Seq[Attribute],
- child: SparkPlan,
- override val serializedPlanOpt: SerializedPlan)
+object CometSortAggregateExec extends CometBaseAggregate[SortAggregateExec] {
+
+ // Spark plans SortAggregate for TypedImperativeAggregate functions whose
intermediate buffer
+ // formats differ between Spark and Comet, the same risk as
ObjectHashAggregate.
+ override protected def requiresCometShuffle: Boolean = true
+
+ override protected def operatorSupportLevel(op: SortAggregateExec):
SupportLevel = {
+ // Spark's sort aggregation buffers in an UnsafeRow, which latches a
decimal sum that leaves
+ // the precision, only when every buffer field is mutable and codegen does
not apply;
+ // otherwise the sum stays unbounded. Comet latches only when grouped, so
decline rather
+ // than track every combination.
+ if (hasMaxPrecisionDecimalSum(op)) {
Review Comment:
Confirmed with your case: without a guard the native sort aggregate returned
NULL with ANSI off and `ARITHMETIC_OVERFLOW` with ANSI on, while Spark returned
`0.26666666666666666666666666666666666667` in both. 27f7269e22 makes
`CometSortAggregateExec` decline a decimal `AVG` or `TRY_AVG` whose sum is at
maximum precision (input precision 28 and up), next to the SUM check, since the
native AVG records the overflow whether or not the aggregate is grouped.
`CometAggregateSuite` now runs your case under both ANSI settings and checks
the result and the fallback reason, and the Sort Aggregation section of the
compatibility guide lists both fallbacks. I kept this to sort aggregates: the
ObjectHashAggregate case is #5509 and the ungrouped hash case is in #5420.
--
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]
