xiangfu0 commented on code in PR #19800:
URL: https://github.com/apache/pinot/pull/19800#discussion_r4225333128
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -186,14 +188,73 @@ public void processBlock(MseBlock.Data block) {
}
}
+ /// Get aggregation result limited to first `maxRows` rows, ordered with
`collations`.
+ public List<Object[]> getResult(List<RelFieldCollation> collations, int
maxRows) {
+ int numGroups = Math.min(_groupIdGenerator.getNumGroups(), maxRows);
+ if (numGroups == 0) {
+ return List.of();
+ }
+
+ int numKeys = _groupKeyIds.length;
+ int numFunctions = _aggFunctions.length;
+ int[] columnsToFinalize = _aggType.isOutputIntermediateFormat() &&
!_leafReturnFinalResult
+ ?
collations.stream().mapToInt(RelFieldCollation::getFieldIndex).filter(index ->
index >= numKeys)
+ .filter(index -> {
+ AggregationFunction aggFunction = _aggFunctions[index - numKeys];
+ ColumnDataType intermediateStoredType =
aggFunction.getIntermediateResultColumnType().getStoredType();
+ return intermediateStoredType == ColumnDataType.OBJECT
Review Comment:
Re-review of 913f8c2: the allowlist restores the raw-heap path as requested,
and `AvgPair`/`MinMaxRangePair`/`BigDecimal` natural order does match the final
numeric order for all normal values — resolved for the reported cost issue. One
caveat the switch comment ("already compare by the aggregate's numeric value")
glosses over: `AvgPair.compareTo` and `MinMaxRangePair.compareTo` compare with
raw `>`/`<` doubles, so a NaN average/range compares equal (0) to every other
group. That makes the trim heap's comparator non-transitive when any group's
sum is NaN (e.g. a DOUBLE column containing NaN), so which groups survive the
leaf trim becomes insertion-order dependent, while the downstream FINAL sort
uses `Double.compareTo`, which ranks NaN greatest — a NaN group that belongs in
the top-K under `ORDER BY AVG(x) DESC` can be silently trimmed. The previous
head's finalized `Double` sort keys had the total order; this head trades that
back for zero allocation (and matches pre-PR maste
r behavior, so this is an edge-case note rather than a blocker). If you want
the allowlist watertight, fixing `AvgPair`/`MinMaxRangePair.compareTo` to use
`Double.compare` would align all three orderings without touching this executor.
--
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]
