uros-b commented on code in PR #57629:
URL: https://github.com/apache/spark/pull/57629#discussion_r3683305335
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InsertMapSortExpression.scala:
##########
@@ -42,42 +44,73 @@ object InsertMapSortInGroupingExpressions extends
Rule[LogicalPlan] {
if (!plan.containsPattern(AGGREGATE)) {
return plan
}
- val shouldRewrite = plan.exists {
Review Comment:
The PR removes the plan-wide shouldRewrite pre-scan guard that
short-circuited the transformUpWithNewOutput traversal when no Aggregate in the
entire plan had map-typed grouping keys. The new code keeps only the
containsPattern(AGGREGATE) check and then enters transformUpWithNewOutput
unconditionally, visiting and evaluating every Aggregate node. For plans with
many aggregates that do not use maps (the common case in production), this adds
a traversal cost per rule invocation. The guard should be restored (e.g.,
plan.exists { case agg: Aggregate if (agg.groupingExpressions ++
distinctAggregateChildren(agg.aggregateExpressions)).exists(mapTypeExistsRecursively)
=> true; case _ => false }) or the early-nil return path should be made
cheaper.
--
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]
