xiangfu0 commented on code in PR #19264:
URL: https://github.com/apache/pinot/pull/19264#discussion_r4110248826
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java:
##########
@@ -89,20 +93,29 @@ public DefaultGroupByExecutor(QueryContext queryContext,
AggregationFunction[] a
// isDictionaryEncoded() flag rather than gating on dictionary nullness
alone.
hasNoDictionaryGroupByExpression |= !columnContext.isDictionaryEncoded();
}
- // Grouping-set queries expand each row into one group per grouping set,
so they always use the
- // multi-value (int[][]) executor path even though the union group-by
columns are single-valued.
- boolean groupingSets = queryContext.isGroupingSets();
- _hasMVGroupByExpression = hasMVGroupByExpression || groupingSets;
+ /// A grouping-set query using the legacy per-row expansion path expands
each row into one group per
+ /// grouping set, so it always uses the multi-value (int[][]) executor
path even though the union group-by
+ /// columns are single-valued. With base aggregation (the default), the
segment instead aggregates the base
+ /// grouping (union columns) exactly like a plain GROUP BY -- no forced MV
path, no grouping-set key
+ /// generator -- and emits the base groups; the combine phase merges them
and derives the individual grouping
+ /// sets in parallel. This keeps total work minimal (each row grouped
once, not once per set) regardless of
+ /// cardinality. Base aggregation is disabled only when a group-by column
is multi-valued: an MV column fans
+ /// a row across its values in the base grouping, so rolling that column
up would over-count the row (see
+ /// [QueryContext#isGroupingSetsBaseAggregation]).
+ boolean useBaseAggregation = queryContext.isGroupingSets() &&
queryContext.isGroupingSetsBaseAggregation()
Review Comment:
Fixed in 6be2ea7 by reinstating the base-group **cardinality gate**: base
aggregation now runs only when the estimated base-group count (the product of
the union columns' dictionary cardinalities) fits under
`groupingSetsBaseAggregationMaxGroups` (default: `numGroupsLimit`). Under
default settings, base keys therefore cannot overflow the limit per segment,
and limit-pressure queries route to expansion, whose totals stay exact. If a
user raises the gate and the base combine table still saturates, the response
now surfaces `numGroupsLimitReached` instead of failing silently. Added
`testNumGroupsLimitPressureKeepsTotalsExactByDefault` (fails without the gate)
and `EstimateBaseGroupCountTest`.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByCombineOperator.java:
##########
@@ -172,6 +184,24 @@ protected void processSegments() {
}
}
+ /// Resolves the merge-time record layout from the first result block's
schema. A base-aggregation grouping-set
+ /// segment emits base groups whose schema has no synthetic $groupingId
column, so it has exactly
+ /// `numUnionColumns + numAggregationFunctions` columns; the expansion path
(and non-grouping-set queries)
+ /// carries the full `$groupingId`-including layout. Deriving the mode from
the schema (rather than the query
+ /// context alone) also covers the MV-column carve-out, where a grouping-set
query still uses expansion.
+ /// Must be called under the same synchronization that guards the one-time
`_indexedTable` creation.
+ private void resolveMergeLayout(GroupByResultsBlock resultsBlock) {
Review Comment:
Fixed in 6be2ea7 with a stronger version of the suggestion: instead of
deciding once (or failing fast, which would break legitimately mixed queries),
the combine now handles layouts **per block** — each block is routed by its
schema width into a base-layout table or a full-layout table, and
`mergeResults()` derives the base table into grouping sets and then merges the
full-layout records in. Mixed segments (SV/MV columns, differing per-segment
cardinality estimates) are now first-class. Added
`MixedLayoutGroupByCombineOperatorTest`.
--
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]
