peter-toth commented on code in PR #40488:
URL: https://github.com/apache/spark/pull/40488#discussion_r1142961725


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala:
##########
@@ -296,12 +298,17 @@ object PhysicalAggregation {
       // build a set of semantically distinct aggregate expressions and 
re-write expressions so
       // that they reference the single copy of the aggregate function which 
actually gets computed.
       // Non-deterministic aggregate expressions are not deduplicated.
-      val equivalentAggregateExpressions = new EquivalentExpressions
+      val equivalentAggregateExpressions = mutable.Map.empty[Expression, 
Expression]
       val aggregateExpressions = resultExpressions.flatMap { expr =>
         expr.collect {
-          // addExpr() always returns false for non-deterministic expressions 
and do not add them.
           case a
-            if AggregateExpression.isAggregate(a) && 
!equivalentAggregateExpressions.addExpr(a) =>

Review Comment:
   Besides the above, although `.addExpr()` fits here well and does the job, 
isn't it a bit weird that an add-like method of a collection-like object 
doesn't return `true` when an item was added, but actually it flips the meaning 
of the return value? If it was used at multiple places then I would keep it, 
but we use it only here. Maybe I'm just nitpicking...
   Anyways, I'm ok with https://github.com/apache/spark/pull/40473 too.



-- 
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: reviews-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to