xiangfu0 commented on code in PR #19800:
URL: https://github.com/apache/pinot/pull/19800#discussion_r4224822309
##########
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:
Addressed in `913f8c225d`. AVG/AVGMV and MINMAXRANGE/MINMAXRANGEMV now
retain the original raw-row heap using their natural Comparable intermediates,
along with SUMPRECISION. They allocate no per-group sort wrappers or boxed
final keys. Validation: all 40 focused runtime tests and the four
affected-module hygiene checks pass.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -230,6 +291,30 @@ public List<Object[]> getResult(Comparator<Object[]>
comparator, int maxRows) {
return result;
}
+ private SortedRow getSortedRow(Object[] row, int[] columnsToFinalize) {
+ if (columnsToFinalize.length == 0) {
+ return new SortedRow(row, row);
+ }
+ // Extract each sort value once per group, retaining intermediate values
for downstream aggregation.
+ Object[] sortKeys = row.clone();
Review Comment:
Addressed in `913f8c225d`. Sort-key arrays now contain only ORDER BY
columns, and values are finalized lazily when their collation is actually
compared. Earlier keys short-circuit later finalization, and each cache also
distinguishes an uninitialized key from a finalized null. Validation: all 40
focused runtime tests and the four affected-module hygiene checks pass.
##########
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
+ || intermediateStoredType !=
aggFunction.getFinalResultColumnType().getStoredType();
+ }).distinct().toArray()
+ : new int[0];
+ SortUtils.SortComparator sortComparator = new
SortUtils.SortComparator(collations, true);
+ // Scalar intermediate values already have their final stored
representation; keep the original heap path.
+ if (columnsToFinalize.length == 0) {
+ return getResult(sortComparator, maxRows);
+ }
+ Comparator<SortedRow> comparator = (r1, r2) ->
sortComparator.compare(r1.sortKeys(), r2.sortKeys());
Review Comment:
Addressed in `913f8c225d`. Both paths now share one generic top-K heap loop.
The raw-row path uses Function.identity(), while the object path decorates and
unwraps compact cached sort rows. Heap selection, draining and rowsProcessed
accounting are implemented once. Validation: all 40 focused runtime tests and
the four affected-module hygiene checks pass.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -230,6 +291,30 @@ public List<Object[]> getResult(Comparator<Object[]>
comparator, int maxRows) {
return result;
}
+ private SortedRow getSortedRow(Object[] row, int[] columnsToFinalize) {
+ if (columnsToFinalize.length == 0) {
Review Comment:
Addressed in `913f8c225d`. Removed the unreachable empty-finalization branch
while replacing getSortedRow with the compact lazy cache. Validation: all 40
focused runtime tests and the four affected-module hygiene checks pass.
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/AggregateOperatorTest.java:
##########
@@ -351,6 +353,133 @@ public void testGroupTrimHintOverridesContextValue() {
assertEquals(getAggregateOperator(context, nodeHint, 5, List.of(new
RelFieldCollation(1))).getGroupTrimSize(), 30);
}
+ @DataProvider
Review Comment:
Addressed in `913f8c225d`. Replaced the five INTERMEDIATE parameter rows
with reachable LEAF leafReturnFinalResult=true cases. The ten rows now cover
both LEAF output modes, followed by FINAL merging, without increasing the
parameterized test count. Validation: all 40 focused runtime tests and the four
affected-module hygiene checks pass.
--
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]
