xiangfu0 commented on code in PR #20063:
URL: https://github.com/apache/pinot/pull/20063#discussion_r4395222790
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageAggregationExecutor.java:
##########
@@ -84,19 +88,28 @@ public List<Object[]> getResult() {
AggregationFunction aggFunction = _aggFunctions[i];
Object value;
switch (_aggType) {
- case LEAF:
- value =
aggFunction.extractAggregationResult(_aggregateResultHolder[i]);
+ case LEAF: {
+ Object intermediateResult =
aggFunction.extractAggregationResult(_aggregateResultHolder[i]);
+ value = _leafReturnFinalResult ?
aggFunction.extractFinalResult(intermediateResult) : intermediateResult;
break;
+ }
case INTERMEDIATE:
value = _mergeResultHolder[i];
break;
- case FINAL:
- value = aggFunction.extractFinalResult(_mergeResultHolder[i]);
+ case FINAL: {
+ Object mergedResult = _mergeResultHolder[i];
+ // The merged result stays null when there is no input row, even if
the leaf returns final results. In that
+ // case, extract the final result of the empty input, e.g. 0 for
COUNT.
+ value = _leafReturnFinalResult && mergedResult != null
+ ? mergedResult
Review Comment:
The null-guard here (extract final result of empty input, e.g. COUNT=0) is
correctly the one difference from MultistageGroupByExecutor.getResultValue,
where an existing group always has >=1 row so mergedResult is never null. Good
call documenting it.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestVisitor.java:
##########
@@ -75,24 +75,29 @@ public Void visitAggregate(AggregateNode node,
ServerPlanRequestContext context)
if (!groupByList.isEmpty()) {
pinotQuery.setGroupByList(groupByList);
}
- /// GROUP BY GROUPING SETS / ROLLUP / CUBE: push the per-set row
expansion down to the single-stage engine. The
- /// per-set column-index lists are over groupByList (==
node.getGroupKeys()), so they transfer directly. The
- /// single-stage leaf expands each row across the sets and appends the
synthetic $groupingId ordinal column; the
- /// multi-stage final stage then groups on it (it is one of the group
keys), keeping the grouping sets distinct.
+ // GROUP BY GROUPING SETS / ROLLUP / CUBE: push the per-set row
expansion down to the single-stage engine. The
+ // per-set column-index lists are over groupByList (==
node.getGroupKeys()), so they transfer directly. The
+ // single-stage leaf expands each row across the sets and appends the
synthetic $groupingId ordinal column; the
+ // multi-stage final stage then groups on it (it is one of the group
keys), keeping the grouping sets distinct.
if (node.isGroupingSets()) {
pinotQuery.setGroupingSets(node.getGroupingSets());
}
- List<Expression> selectList =
CalciteRexExpressionParser.convertAggregateList(groupByList, node.getAggCalls(),
- node.getFilterArgs(), pinotQuery.getSelectList());
+ List<Expression> selectList =
+ CalciteRexExpressionParser.convertAggregateList(groupByList,
node.getAggCalls(), node.getFilterArgs(),
+ pinotQuery.getSelectList());
for (Expression expression : selectList) {
applyTimestampIndex(expression, pinotQuery);
}
pinotQuery.setSelectList(selectList);
if (node.getAggType() == AggregateNode.AggType.DIRECT) {
-
pinotQuery.putToQueryOptions(CommonConstants.Broker.Request.QueryOptionKey.SERVER_RETURN_FINAL_RESULT,
"true");
+
pinotQuery.putToQueryOptions(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true");
} else if (node.isLeafReturnFinalResult()) {
- pinotQuery.putToQueryOptions(
-
CommonConstants.Broker.Request.QueryOptionKey.SERVER_RETURN_FINAL_RESULT_KEY_UNPARTITIONED,
"true");
+ // SERVER_RETURN_FINAL_RESULT_KEY_UNPARTITIONED only applies to
group-by queries.
+ if (groupByList.isEmpty()) {
+
pinotQuery.putToQueryOptions(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true");
+ } else {
+
pinotQuery.putToQueryOptions(QueryOptionKey.SERVER_RETURN_FINAL_RESULT_KEY_UNPARTITIONED,
"true");
+ }
Review Comment:
Correct split: for no-GROUP-BY, SERVER_RETURN_FINAL_RESULT_KEY_UNPARTITIONED
was effectively a no-op (AggregationResultsBlock only honors
SERVER_RETURN_FINAL_RESULT), so routing non-group-by to
SERVER_RETURN_FINAL_RESULT is what makes leaf-return-final actually take
effect. GROUP BY path unchanged.
--
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]
