xiangfu0 commented on code in PR #19264:
URL: https://github.com/apache/pinot/pull/19264#discussion_r4110250926
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/query/GroupByOperator.java:
##########
@@ -167,6 +193,18 @@ protected GroupByResultsBlock getNextBlock() {
/// the final ORDER BY + LIMIT across all sets.
if (_queryContext.isGroupingSets()) {
/// The $groupingId discriminator is the key column immediately after
the union group-by columns.
+ if (groupByExecutor.isGroupingSetsBaseAggregation()) {
Review Comment:
Documented in the PR description (Behavior change section) and locked with a
test in 6be2ea7: `testBaseAggregationBypassesSegmentTrim` runs ORDER BY +
`minSegmentGroupTrimSize=1` and asserts base-path results are unchanged — the
trim is bypassed (base path stays exact where the expansion segment trim is an
approximation), not misapplied.
##########
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() {
Review Comment:
Partially restructured in 6be2ea7. Full centralization is not possible
because two of the carve-outs (MV columns, cardinality estimate) are
per-SEGMENT properties, not query properties. Instead the combine now accepts
BOTH layouts per block (see the mixed-layout thread), so a wrong or missing
exclusion degrades to a correct-but-different-path merge rather than silent
corruption. The query-level exclusions (filtered aggs, streaming, sorted
combine) stay in one place, `QueryContext#isGroupingSetsBaseAggregation`, and
`StreamingGroupByCombineOperatorTest#testGroupingSetsKeyColumnLayout` locks the
layout contract on the streaming path. Agreed a defensive layout check in the
other combine operators would be a good follow-up.
--
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]
