uros-b commented on code in PR #57629:
URL: https://github.com/apache/spark/pull/57629#discussion_r3683347765
##########
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 {
- case agg: Aggregate if
agg.groupingExpressions.exists(mapTypeExistsRecursively) => true
- case _ => false
- }
- if (!shouldRewrite) {
- return plan
- }
plan transformUpWithNewOutput {
case agg @ Aggregate(groupingExprs, aggregateExpressions, child, hint) =>
- val exprToMapSort = new mutable.HashMap[Expression, NamedExpression]
- val newGroupingKeys = groupingExprs.map { expr =>
- val inserted = insertMapSortRecursively(expr)
- if (expr.ne(inserted)) {
- exprToMapSort.getOrElseUpdate(
- expr.canonicalized,
- Alias(inserted, "_groupingmapsort")()
- ).toAttribute
+ val distinctExpressions =
+ if
(conf.getConf(SQLConf.INSERT_MAP_SORT_IN_DISTINCT_AGGREGATES_ENABLED)) {
+ distinctAggregateChildren(aggregateExpressions)
} else {
- expr
+ Seq.empty
}
+ val expressionsToNormalize = groupingExprs ++ distinctExpressions
+ if (!expressionsToNormalize.exists(mapTypeExistsRecursively)) {
+ agg -> Nil
+ } else {
+ val groupingMapSortAliases = mapSortAliases(groupingExprs)
+ val distinctMapSortAliases = mapSortAliases(distinctExpressions,
groupingMapSortAliases)
+ val newGroupingKeys = groupingExprs.map { expr =>
+
groupingMapSortAliases.get(expr.canonicalized).map(_.toAttribute).getOrElse(expr)
+ }
+ val newAggregateExprs = aggregateExpressions.map {
+ case named if groupingMapSortAliases.contains(named.canonicalized)
=>
+ // If we replace the top-level named expr, then should add back
the original name
+
groupingMapSortAliases(named.canonicalized).toAttribute.withName(named.name)
+ case other =>
+ other.transformDown {
Review Comment:
The transformUp -> transformDown switch is unexplained although it appears
safe: after PullOutGroupingExpressions the alias keys are attributes or
foldables, i.e. leaves, so traversal order doesn't change the grouping path;
which also explains why no existing golden or plan-stability file moved.
It is load-bearing though, in two directions: top-down is required so the
AggregateExpression is seen before its children, and continuing to descend
after the distinct case is what keeps grouping-alias substitution working
inside arguments like count(DISTINCT map_keys(m)). That deserves a comment,
because a future refactor back to transformUp would silently break it.
--
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]
