Copilot commented on code in PR #19264:
URL: https://github.com/apache/pinot/pull/19264#discussion_r3790092774
##########
pinot-core/src/main/java/org/apache/pinot/core/util/GroupByUtils.java:
##########
@@ -87,6 +98,125 @@ public static GroupByResultsBlock
buildGroupingSetsResultsBlock(QueryContext que
return resultsBlock;
}
+ /// Builds the segment-level [GroupByResultsBlock] for a grouping-set query
from BASE groups, i.e. groups
+ /// aggregated once over the union of all grouping-set columns (a plain
GROUP BY), rather than by expanding
+ /// every input row into one group per grouping set. Each base group is then
projected into each grouping set:
+ /// its rolled-up (non-participating) columns are set to `null`, the
`$groupingId` discriminator is appended,
+ /// and the base group's aggregation intermediates are merged into the
derived group. This moves the per-set
+ /// fan-out from O(rows) to O(base groups), reusing the fast plain-GROUP-BY
scan path.
+ ///
+ /// Because a base group's intermediate result flows into every grouping
set, and [AggregationFunction#merge]
+ /// mutates its first argument, each base intermediate is cloned per set
before it becomes a merge target (see
+ /// [#cloneIntermediate]). This keeps the derivation exact for object-backed
accumulators (AVG, DISTINCTCOUNT,
+ /// percentiles, ...) as well as scalar ones.
+ ///
+ /// @param discriminatorColumnIndex index of the synthetic $groupingId
column, i.e. the number of union
+ /// group-by columns
+ public static GroupByResultsBlock
buildGroupingSetsResultsBlockFromBaseGroups(QueryContext queryContext,
+ DataSchema dataSchema, AggregationGroupByResult baseResult, int
discriminatorColumnIndex,
+ boolean numGroupsLimitReached, boolean numGroupsWarningLimitReached) {
+ AggregationFunction[] aggregationFunctions =
queryContext.getAggregationFunctions();
+ assert aggregationFunctions != null;
+ int numAggregationFunctions = aggregationFunctions.length;
+ List<int[]> groupingSets = queryContext.getGroupingSets();
+ int numSets = groupingSets.size();
+ int numUnionColumns = discriminatorColumnIndex;
+ // Per grouping set: membership mask over the union columns (true =
participates, false = rolled up to NULL).
+ boolean[][] setContains = new boolean[numSets][numUnionColumns];
+ for (int s = 0; s < numSets; s++) {
+ for (int columnIndex : groupingSets.get(s)) {
+ setContains[s][columnIndex] = true;
+ }
+ }
+
+ // Derived group table keyed on (projected union values..., $groupingId).
Values layout mirrors the record
+ // schema: key columns first, then the aggregation intermediates.
+ Map<Key, Record> derived = new HashMap<>();
Review Comment:
The derived table is no longer bounded by `numGroupsLimit`. The base
generator caps only base groups, but each retained base group can create
records in every set; for example, a limit of 2 with `ROLLUP(d1)` can
materialize 3 derived groups, and the worst case is 100,000 base groups × up to
4,096 sets. The optional trim runs only after this map is fully built, so this
bypasses the segment memory guardrail and can exhaust heap. Bound
materialization while deriving (with clearly defined global/per-set semantics)
and include derived counts in the limit/warning accounting.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/GroupingSetsGroupKeyGenerator.java:
##########
@@ -378,13 +545,39 @@ private Object[] buildKeysFromIds(FixedIntArray keyList) {
int[] ids = keyList.elements();
Object[] keys = new Object[_numGroupByExpressions + 1];
for (int i = 0; i < _numGroupByExpressions; i++) {
- keys[i] = ids[i] == ID_FOR_NULL ? null :
_onTheFlyDictionaries[i].get(ids[i]);
+ if (ids[i] == ID_FOR_NULL) {
+ keys[i] = null;
+ } else if (_dictionaries[i] != null) {
+ keys[i] = _dictionaries[i].getInternal(ids[i]);
+ } else {
+ keys[i] = _onTheFlyDictionaries[i].get(ids[i]);
+ }
}
/// The trailing slot stores the grouping-set ordinal directly (not a
dictionary id).
keys[_numGroupByExpressions] = ids[_numGroupByExpressions];
return keys;
}
+ /// Unpacks a long-packed composite key into the output row: each union
column value (NULL where the packed
+ /// id is the reserved sentinel) followed by the integer
grouping-set-ordinal discriminator. Inverse of the
+ /// packing done in [#generateKeysForBlockSvLongPacked] /
[#expandGroupIdsLongPacked].
+ private Object[] buildKeysFromLong(long key) {
+ Object[] keys = new Object[_numGroupByExpressions + 1];
+ for (int i = 0; i < _numGroupByExpressions; i++) {
+ int packedId = extractPackedId(key, i);
+ keys[i] = packedId == _nullPackedIds[i] ? null :
_dictionaries[i].getInternal(packedId);
+ }
+ keys[_numGroupByExpressions] = (int) (key >>>
_bitShifts[_numGroupByExpressions]);
Review Comment:
When the column slots consume exactly 64 bits and there is one grouping set,
the discriminator shift is 64. Java masks long shift distances modulo 64, so
`key >>> 64` is actually `key >>> 0`; the emitted `$groupingId` becomes the low
32 key bits instead of ordinal 0. Handle the single-set case explicitly (and
add a 64-bit boundary test).
--
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]
