Zouxxyy commented on PR #11264:
URL:
https://github.com/apache/incubator-gluten/pull/11264#issuecomment-3633197535
> Original design is to put all groupings into velox group keys. Is there
any cases we may generate more than 1 AggregateRel.groupings? Will it cause
issue on original design?
Currently, `AggregateRel.groupings` is actually a `List<List<keys>>`,
representing multiple grouping sets. However, Velox's AggregationNode only
supports a single grouping set.
In the current implementation of AggregateRelNode.toProtobuf, only
groupings[0] = groupingKeys is populated.
I discovered this issue because we have developed an AggregationNode that
supports multiple grouping sets.
In this PR, no proto changes are made—only defensive code is added. In fact,
you’ll notice that the following two nested loops look quite odd:
```c++
for (const auto& grouping : aggRel.groupings()) {
for (const auto& groupingExpr : grouping.grouping_expressions()) {
// Velox's groupings are limited to be Field.
veloxGroupingExprs.emplace_back(exprConverter_->toVeloxExpr(groupingExpr.selection(),
inputType));
}
}
```
--
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]
