viirya commented on code in PR #47525:
URL: https://github.com/apache/spark/pull/47525#discussion_r1698739188
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteDistinctAggregates.scala:
##########
@@ -197,6 +197,15 @@ import org.apache.spark.util.collection.Utils
* techniques.
*/
object RewriteDistinctAggregates extends Rule[LogicalPlan] {
+ private def mustRewrite(
+ aggregateExpressions: Seq[AggregateExpression],
+ groupingExpressions: Seq[Expression]): Boolean = {
+ // If there are any AggregateExpressions with filter, we need to rewrite
the query.
+ // Also, if there are no grouping expressions and all aggregate
expressions are foldable,
+ // we need to rewrite the query, e.g. SELECT COUNT(DISTINCT 1).
Review Comment:
Compared to the comment in `mayNeedtoRewrite` which explains why rewriting
is necessary. This comment doesn't do any explanation but just claims it needs
to rewrite the query. This comment simply describes what the code does and it
is obvious.
To better improve the code readability, it would be better to explain why
the rewriting is needed for the case.
--
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
