yashmayya commented on code in PR #19379:
URL: https://github.com/apache/pinot/pull/19379#discussion_r3916284462
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java:
##########
@@ -184,6 +187,7 @@ private Map<ExpressionContext, Integer>
getGroupByExpressionSizesFromPredicates(
// Populate the group-by expressions with sizes from the predicate map
return queryContext.getGroupByExpressions().stream()
.filter(predicateSizeMap::containsKey)
+ .filter(expression ->
projectOperator.getResultColumnContext(expression).isSingleValue())
.collect(Collectors.toMap(
Review Comment:
`Collectors.toMap` has no merge function, and `getGroupByExpressions()`
keeps duplicates, so `GROUP BY c0, c0` throws here when the option is on:
```
Duplicate key c0 (attempted merging values 1 and 1)
```
Pre-existing, but it means the duplicate-expression handling you added in
`getOptimizedGroupByCardinality` never gets to run. `Integer::min` as the merge
function fixes it, in the method you are already editing.
--
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]
