zhztheplayer commented on code in PR #6689:
URL: https://github.com/apache/incubator-gluten/pull/6689#discussion_r1704823529


##########
backends-velox/src/main/scala/org/apache/gluten/extension/CollectRewriteRule.scala:
##########
@@ -48,6 +51,19 @@ case class CollectRewriteRule(spark: SparkSession) extends 
Rule[LogicalPlan] {
     out
   }
 
+  private def needReplace(plan: LogicalPlan): Boolean = plan match {
+    // Do not replace with objectHashAggregate if disable objectHashAgg 
collectRewrite.
+    case PhysicalAggregation(_, aggregateExpr, _, _)
+        if !GlutenConfig.getConf.veloxObjectHashAggCollectRewriteEnabled =>
+      val aggregateExpressions = aggregateExpr.map(expr => 
expr.asInstanceOf[AggregateExpression])
+      val useHash = SparkShimLoader.getSparkShims.supportsHashAggregate(
+        aggregateExpressions.flatMap(_.aggregateFunction.aggBufferAttributes))
+      val useObjectHash = plan.conf.useObjectHashAggregation &&
+        
SparkShimLoader.getSparkShims.supportsObjectHashAggregate(aggregateExpressions)
+      useHash || !useObjectHash
+    case _ => true
+  }

Review Comment:
   Sorry I still don't quite understand (thought it will be a simple switch to 
turn the rule on/off). If all you wanted is to fallback to vanilla Spark's 
object hash aggregate, why not just disable this rule?
   
   If it's hard to explain, would you like to share the comparison of query 
plans in text before and after the change?
   



-- 
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]

Reply via email to