Copilot commented on code in PR #18662:
URL: https://github.com/apache/pinot/pull/18662#discussion_r3385847359
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextConverterUtils.java:
##########
@@ -114,6 +114,26 @@ 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) {
+ groupingSets = new ArrayList<>(groupingSetMasks.size());
+ for (int mask : groupingSetMasks) {
+ int[] indexes = new int[Integer.bitCount(mask)];
+ int idx = 0;
+ for (int bit = 0; bit < Integer.SIZE && idx < indexes.length; bit++) {
+ if ((mask & (1 << bit)) != 0) {
+ indexes[idx++] = bit;
+ }
+ }
+ groupingSets.add(indexes);
+ }
+ }
Review Comment:
`groupingSetMasks` is decoded into column indexes without validating that
the mask only references existing GROUP BY union columns. A
malformed/mismatched request (e.g. mask with a bit >= groupByList.size()) will
later cause out-of-bounds access when building grouping sets (and can crash the
broker/server). It’s safer to validate masks against the union size during
QueryContext conversion (and optionally ignore the special case where the union
is empty).
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -690,6 +724,172 @@ private static List<Expression>
convertSelectList(SqlNodeList selectList) {
return selectExpr;
}
+ /// Converts the GROUP BY clause into {@link PinotQuery#groupByList} and
(when grouping constructs are
+ /// present) {@link PinotQuery#groupingSets}.
+ ///
+ /// For a plain GROUP BY (no ROLLUP / CUBE / GROUPING SETS) this behaves
exactly like
+ /// {@link #convertSelectList} and leaves {@code groupingSets} unset, so
non-grouping-set queries are
+ /// unchanged. When grouping constructs are present, the grouping elements
are cross-multiplied into the
+ /// canonical, de-duplicated list of grouping sets (standard SQL semantics,
e.g.
+ /// {@code GROUP BY a, ROLLUP(b, c)} produces {@code {a,b,c}, {a,b}, {a}});
the ordered, de-duplicated
+ /// union of all participating columns is stored as {@code groupByList}, and
each grouping set is stored
+ /// as the sorted list of indexes into that union (an empty list = the
grand-total set {@code ()}).
Review Comment:
The doc comment for `setGroupByListAndGroupingSets` refers to
`PinotQuery#groupingSets` and describes per-set values as “sorted list of
indexes”, but the implementation actually populates
`PinotQuery#groupingSetMasks` (bitmask per set). Updating the comment avoids
misleading future maintainers.
--
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]
