jaceklaskowski commented on code in PR #40685:
URL: https://github.com/apache/spark/pull/40685#discussion_r1162709345
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -603,31 +603,21 @@ class Analyzer(override val catalogManager:
CatalogManager) extends RuleExecutor
aggregations: Seq[NamedExpression],
groupByAliases: Seq[Alias],
groupingAttrs: Seq[Expression],
- gid: Attribute): Seq[NamedExpression] = aggregations.map { agg =>
- // collect all the found AggregateExpression, so we can check an
expression is part of
- // any AggregateExpression or not.
- val aggsBuffer = ArrayBuffer[Expression]()
- // Returns whether the expression belongs to any expressions in
`aggsBuffer` or not.
- def isPartOfAggregation(e: Expression): Boolean = {
- aggsBuffer.exists(a => a.exists(_ eq e))
- }
- replaceGroupingFunc(agg, groupByExprs, gid).transformDown {
- // AggregateExpression should be computed on the unmodified value of
its argument
- // expressions, so we should not replace any references to grouping
expression
- // inside it.
- case e if AggregateExpression.isAggregate(e) =>
- aggsBuffer += e
- e
- case e if isPartOfAggregation(e) => e
+ gid: Attribute): Seq[NamedExpression] = {
+ def replaceExprs(e: Expression): Expression = e match {
+ case e if AggregateExpression.isAggregate(e) => e
case e =>
// Replace expression by expand output attribute.
val index = groupByAliases.indexWhere(_.child.semanticEquals(e))
if (index == -1) {
- e
+ e.mapChildren(replaceExprs)
} else {
groupingAttrs(index)
}
- }.asInstanceOf[NamedExpression]
+ }
+ aggregations.map { agg =>
+ replaceExprs(replaceGroupingFunc(agg, groupByExprs,
gid)).asInstanceOf[NamedExpression]
+ }
Review Comment:
nit: Can we agree on splitting this "convoluted" `map` into a series of
`map`s? 🙏 It's hard to read what the line does (seems easy at first sight but
takes longer).
```
aggregations
.map(agg => replaceGroupingFunc(agg, groupByExprs, gid)) // perhaps even
replaceGroupingFunc(_, groupByExprs, gid)?
.map(agg => replaceExprs(agg)) // perhaps replaceExprs along would be
enough?
.map(_.asInstanceOf[NamedExpression])
```
--
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]
