yashmayya commented on code in PR #19417:
URL: https://github.com/apache/pinot/pull/19417#discussion_r3899113508
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/CombinePlanNode.java:
##########
@@ -130,10 +130,12 @@ private BaseCombineOperator getCombineOperator() {
// Use streaming operator only for non-empty selection-only query
return new StreamingSelectionOnlyCombineOperator(operators,
_queryContext, _executorService);
}
+ // Streaming flushes partial aggregates, so it needs an aggregation
above to merge them back together.
+ // Leaves that must return final results are excluded, see
StreamingGroupByCombineOperator.
int flushThreshold = _queryContext.getStreamingGroupByFlushThreshold();
if (flushThreshold > 0 &&
QueryContextUtils.isAggregationQuery(_queryContext)
- && _queryContext.getGroupByExpressions() != null) {
- // Use streaming group-by operator for MSE leaf stages with flush
threshold
+ && _queryContext.getGroupByExpressions() != null &&
!_queryContext.isServerReturnFinalResult()
Review Comment:
Grouping sets are unaffected — they can never carry either flag, so this
condition is a no-op for them and they keep streaming (which is safe, since
they always get LEAF + EXCHANGE + FINAL).
I checked by walking the planned `AggregateNode`s for each shape, on both
planners:
| Query | v1 | v2 |
|---|---|---|
| `GROUP BY ROLLUP(col1)` | LEAF+FINAL, `leafReturnFinalResult=false` | same
|
| `+ is_partitioned_by_group_by_keys` | rejected: "Aggregate hints are not
supported with GROUP BY GROUPING SETS / ROLLUP / CUBE" | rejected: "...without
a repartitioning exchange is not yet supported" |
| `+ is_leaf_return_final_result` | LEAF+FINAL,
`leafReturnFinalResult=false` (hint not applied) | same |
| `GROUP BY GROUPING SETS ((col1), ())` `+ is_leaf_return_final_result` |
LEAF+FINAL, `leafReturnFinalResult=false` | same |
| plain `GROUP BY col1` `+ is_leaf_return_final_result` (control) |
LEAF+FINAL, `leafReturnFinalResult=true` | same |
So no `AggType.DIRECT` (hence no `serverReturnFinalResult`), and
`leafReturnFinalResult` is forced `false` rather than read from the hint —
`PinotAggregateExchangeNodeInsertRule.createPlan` passes a literal `false` on
the grouping-sets branch, and
`AggregatePushdownRule.addPartialAggregateForGroupingSets` does the same. The
last row is the control: it shows the check would have caught
`leafReturnFinalResult=true` had it been reachable.
Trim behaviour also stays safe.
`GroupByUtils.createIndexedTableForCombineOperator` gives grouping sets a
trim-disabled table capped at `numGroupsLimit`; the streaming operator's table
is trim-disabled with `resultSize = Integer.MAX_VALUE`, so it keeps at least as
many groups and defers ORDER BY + LIMIT upward the same way.
--
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]
