xiangfu0 commented on code in PR #19800:
URL: https://github.com/apache/pinot/pull/19800#discussion_r4225331983
##########
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:
Re-review of 913f8c2: the reachable funnel bug is fixed (FUNNEL* now
bypasses trim entirely), and the heap no longer finalizes live funnel state.
One residual on this thread: the check is a fail-unsafe denylist.
`cacheSortValue` finalizes the live intermediate that stays in the output row,
and the only guard against a destructive finalizer is
`getType().name().startsWith("FUNNEL")`. There is already a concrete non-FUNNEL
counterexample in the codebase:
`DistinctCountOffHeapAggregationFunction.extractFinalResult` calls
`set.close()` (frees native memory) on an OBJECT intermediate with INT final
type, so it passes every gate here. It is unreachable today only because its
`aggregateGroupBySV`/`extractGroupByResult` throw
`UnsupportedOperationException` — if group-by support lands (or any plugin adds
a destructive numeric-final finalizer), ordered trim silently corrupts (here,
use-after-free) with no compile error or test failure. Since the untrimmed
fallback is always correct, cons
ider failing safe at zero cost to current behavior: route unknown OBJECT
intermediates to `getResult(Integer.MAX_VALUE)` unless the function is known
safe to finalize (invert to an allowlist), or at minimum add
DISTINCTCOUNTOFFHEAP next to the FUNNEL check with a comment naming the
invariant.
--
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]
