xiangfu0 commented on code in PR #19264:
URL: https://github.com/apache/pinot/pull/19264#discussion_r4127620228
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java:
##########
@@ -252,6 +253,26 @@ public int getNumExtraGroupByKeyColumns() {
return isGroupingSets() ? 1 : 0;
}
+ /// Returns whether grouping-set queries should aggregate the base grouping
(union columns) once per segment
+ /// and derive the individual grouping-set records from those base groups,
instead of expanding every input
+ /// row into one group per grouping set. Enabled by default; the
`groupingSetsBaseAggregation=false` query
+ /// option forces the legacy per-row expansion path. See
[CommonConstants.Broker.Request.QueryOptionKey].
+ ///
+ /// Only the standard
[org.apache.pinot.core.operator.combine.GroupByCombineOperator] performs the
merge-time
+ /// derive that turns the emitted base groups back into grouping-set
records. Base aggregation is therefore
+ /// disabled whenever a different combine path would run: filtered
aggregations (distinct shared group-key
+ /// generator), the streaming combine used on MSE leaf stages
(`streamingGroupByFlushThreshold > 0`, which
+ /// flushes incrementally and cannot derive over the fully-merged base
table), and the sorted-aggregate combine
+ /// ([#shouldSortAggregateUnderSafeTrim()]). In all these cases the segment
expands per row so the emitted rows
+ /// already carry the $groupingId discriminator.
+ public boolean isGroupingSetsBaseAggregation() {
+ if (_hasFilteredAggregations || _streamingGroupByFlushThreshold > 0 ||
shouldSortAggregateUnderSafeTrim()) {
Review Comment:
Fixed in 119c437 with the flag you suggested:
`AggregationFunction#canMergeIntermediatesAcrossGroups()` (default true),
overridden to false for `SEGMENTPARTITIONEDDISTINCTCOUNT` and partitioned
`FUNNEL_COUNT` (via the funnel `MergeStrategy`, so only the partitioned
strategies opt out), plus `EXPRMIN`/`EXPRMAX` (deserialized-intermediate
immutability). `QueryContext#isGroupingSetsBaseAggregation()` returns false
when any aggregation cannot merge across groups, so those queries always take
the expansion path — including when the broker rewrites `DISTINCTCOUNT` to the
segment-partitioned form.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java:
##########
@@ -252,6 +253,26 @@ public int getNumExtraGroupByKeyColumns() {
return isGroupingSets() ? 1 : 0;
}
+ /// Returns whether grouping-set queries should aggregate the base grouping
(union columns) once per segment
+ /// and derive the individual grouping-set records from those base groups,
instead of expanding every input
+ /// row into one group per grouping set. Enabled by default; the
`groupingSetsBaseAggregation=false` query
+ /// option forces the legacy per-row expansion path. See
[CommonConstants.Broker.Request.QueryOptionKey].
+ ///
+ /// Only the standard
[org.apache.pinot.core.operator.combine.GroupByCombineOperator] performs the
merge-time
+ /// derive that turns the emitted base groups back into grouping-set
records. Base aggregation is therefore
+ /// disabled whenever a different combine path would run: filtered
aggregations (distinct shared group-key
+ /// generator), the streaming combine used on MSE leaf stages
(`streamingGroupByFlushThreshold > 0`, which
+ /// flushes incrementally and cannot derive over the fully-merged base
table), and the sorted-aggregate combine
+ /// ([#shouldSortAggregateUnderSafeTrim()]). In all these cases the segment
expands per row so the emitted rows
+ /// already carry the $groupingId discriminator.
+ public boolean isGroupingSetsBaseAggregation() {
+ if (_hasFilteredAggregations || _streamingGroupByFlushThreshold > 0 ||
shouldSortAggregateUnderSafeTrim()) {
+ return false;
+ }
+ String option =
_queryOptions.get(CommonConstants.Broker.Request.QueryOptionKey.GROUPING_SETS_BASE_AGGREGATION);
+ return option == null || Boolean.parseBoolean(option);
Review Comment:
Done in 119c437 — the feature is now **off by default**;
`groupingSetsBaseAggregation=true` opts in. The unbounded output is also fixed:
the derived table is capped at `numGroupsLimit`, admitting COARSEST sets first
(grand total/subtotals always survive; only the fine sets trim) and flagging
`groupsTrimmed` — the CUBE-over-12-columns example now sends at most
`numGroupsLimit` rows like expansion. Agreed on the benchmark gap (COUNT/SUM
only) and on the gate not modeling the collapse ratio or per-merge cost — both
are noted in the PR description as prerequisites for any future default-on
discussion. Thanks for the thorough repros; the serialize-mutation race in
particular was an excellent catch.
--
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]
