cloud-fan commented on a change in pull request #27198: 
[SPARK-27986][SQL][FOLLOWUP] Respect filter in sql/toString of 
AggregateExpression
URL: https://github.com/apache/spark/pull/27198#discussion_r366223460
 
 

 ##########
 File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/interfaces.scala
 ##########
 @@ -149,10 +149,24 @@ case class AggregateExpression(
       case PartialMerge => "merge_"
       case Final | Complete => ""
     }
-    prefix + aggregateFunction.toAggString(isDistinct)
+    val aggFuncStr = prefix + aggregateFunction.toAggString(isDistinct)
+    mode match {
+      case Partial | Complete if filter.isDefined =>
+        s"$aggFuncStr filter ${filter.get.toString}"
+      case _ =>
+        aggFuncStr
+    }
   }
 
-  override def sql: String = aggregateFunction.sql(isDistinct)
+  override def sql: String = {
+    val aggFuncStr = aggregateFunction.sql(isDistinct)
+    mode match {
+      case Partial | Complete if filter.isDefined =>
 
 Review comment:
   another idea: if the filter is not used under some modes, can we drop it 
when we do things like `aggExprs.map(_.copy(mode = ABC))`? Then here we can 
blindly print the filter if it's there.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

Reply via email to