xiangfu0 commented on code in PR #19800:
URL: https://github.com/apache/pinot/pull/19800#discussion_r4224821117
##########
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();
+ for (int index : columnsToFinalize) {
+ AggregationFunction aggFunction = _aggFunctions[index -
_groupKeyIds.length];
+ Object intermediateResult = row[index];
+ // Funnel finalization consumes the event queue, so extract from a copy.
+ if (intermediateResult instanceof PriorityQueue<?> queue) {
+ intermediateResult = new PriorityQueue<>(queue);
+ }
+ Object finalResult = aggFunction.extractFinalResult(intermediateResult);
+ sortKeys[index] = finalResult != null
Review Comment:
Addressed in `913f8c225d`. SUMPRECISION now keeps its native BigDecimal
intermediate ordering, and finalized numeric sort keys are no longer converted
through the SSE result metadata. The 5..9,100 regression fails on the prior PR
head and now retains the 100 group. 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();
+ for (int index : columnsToFinalize) {
+ AggregationFunction aggFunction = _aggFunctions[index -
_groupKeyIds.length];
+ Object intermediateResult = row[index];
+ // Funnel finalization consumes the event queue, so extract from a copy.
+ if (intermediateResult instanceof PriorityQueue<?> queue) {
+ intermediateResult = new PriorityQueue<>(queue);
+ }
+ Object finalResult = aggFunction.extractFinalResult(intermediateResult);
+ sortKeys[index] = finalResult != null
+ ? TypeUtils.convert(finalResult,
aggFunction.getFinalResultColumnType().getStoredType()) : null;
Review Comment:
Addressed in `913f8c225d`. Intermediate ordered trim now preserves all
groups when an OBJECT aggregate has a non-numeric final sort type, including
HISTOGRAM/ARRAY_AGG arrays. The HISTOGRAM regression reproduces the prior
double[]-to-Comparable failure and now passes without finalizing the array key.
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();
+ for (int index : columnsToFinalize) {
+ AggregationFunction aggFunction = _aggFunctions[index -
_groupKeyIds.length];
+ Object intermediateResult = row[index];
+ // Funnel finalization consumes the event queue, so extract from a copy.
+ if (intermediateResult instanceof PriorityQueue<?> queue) {
+ intermediateResult = new PriorityQueue<>(queue);
+ }
+ Object finalResult = aggFunction.extractFinalResult(intermediateResult);
Review Comment:
Addressed in `913f8c225d`. Intermediate raw-sketch STRING orders now bypass
trimming and retain every sketch for downstream merging and sorting. This
avoids selecting groups by a partial serialization or changing the requested
final STRING ordering to estimate ordering. The raw-HLL regression now verifies
that all six partials survive a trim capacity of five. 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();
+ for (int index : columnsToFinalize) {
+ AggregationFunction aggFunction = _aggFunctions[index -
_groupKeyIds.length];
+ Object intermediateResult = row[index];
+ // Funnel finalization consumes the event queue, so extract from a copy.
+ if (intermediateResult instanceof PriorityQueue<?> queue) {
Review Comment:
Addressed in `913f8c225d`. Removed the PriorityQueue copy guard. Funnel sort
keys now bypass intermediate trimming, so trim never consumes their live event
state. The expanded six-group funnel regression verifies all events survive to
FINAL. A broader non-destructive AggregationFunction contract and the
pre-existing SSE path are separate from this minimal MSE bug fix. 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);
Review Comment:
Addressed in `913f8c225d`. Added the early return when genuinely
intermediate output has no more groups than the trim capacity. It uses the
ordinary result path without sorting/finalization; DIRECT, FINAL and
leafReturnFinalResult output keep their existing heap behavior. 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]
