xiangfu0 commented on code in PR #18662:
URL: https://github.com/apache/pinot/pull/18662#discussion_r3391810017
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextConverterUtils.java:
##########
@@ -114,6 +114,34 @@ public static QueryContext getQueryContext(PinotQuery
pinotQuery) {
}
}
+ /// GROUP BY GROUPING SETS / ROLLUP / CUBE: the wire carries one
membership bitmask per set over
+ /// groupByExpressions (the union of all grouping columns). Decode each
mask into the sorted list of
+ /// participating column indexes; a mask of 0 yields the empty
(grand-total) set (). Null when this is a
+ /// plain GROUP BY query.
+ List<int[]> groupingSets = null;
+ List<Integer> groupingSetMasks = pinotQuery.getGroupingSetMasks();
+ if (groupingSetMasks != null) {
+ int numGroupByExpressions = groupByExpressions != null ?
groupByExpressions.size() : 0;
+ groupingSets = new ArrayList<>(groupingSetMasks.size());
+ for (int mask : groupingSetMasks) {
Review Comment:
Fixed in 1f8623392f — conversion now also rejects an empty
`groupingSetMasks` list (would otherwise silently produce zero groups) and a
group-by union of more than 31 columns (cannot be represented in the INT
grouping-id bitmask), reusing `CalciteSqlParser.MAX_GROUPING_SETS_COLUMNS` as
the single source of truth. With the union capped, the per-mask check
simplifies to one unsigned shift that also rejects negative masks. Both shapes
covered in
`BrokerRequestToQueryContextConverterTest#testInvalidGroupingSetMasksRejected`.
--
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]
