Jackie-Jiang commented on code in PR #19800:
URL: https://github.com/apache/pinot/pull/19800#discussion_r4253117012
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/funnel/BitmapMergeStrategy.java:
##########
@@ -41,11 +41,12 @@ public List<RoaringBitmap> merge(List<RoaringBitmap>
intermediateResult1, List<R
@Override
public LongArrayList extractFinalResult(List<RoaringBitmap> stepsBitmaps) {
long[] result = new long[_numSteps];
- result[0] = stepsBitmaps.get(0).getCardinality();
+ RoaringBitmap intersection = stepsBitmaps.get(0).clone();
Review Comment:
Non-blocking cleanup: the bitmap path still clones the entire first
population for single-step funnels or an empty second step. We can read the
first cardinality directly, create a private intersection with
`RoaringBitmap.and(first, second)` only when needed, then mutate that private
result and stop once it is empty. This preserves the input state while avoiding
unnecessary copies, including for the default and partitioned funnel callers.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/funnel/window/FunnelMaxStepAggregationFunction.java:
##########
@@ -51,6 +51,7 @@ public Integer extractFinalResult(@Nullable
PriorityQueue<FunnelStepEvent> stepE
if (stepEvents == null || stepEvents.isEmpty()) {
return finalMaxStep;
}
+ stepEvents = new PriorityQueue<>(stepEvents);
Review Comment:
Non-blocking performance follow-up for the window funnel finalizers:
preserving the queue is necessary, but each extraction now copies all event
references, even when evaluation can terminate early after finding a complete
funnel. This is a shallow copy, not a copy of the events themselves. Please
include large queues and early-completion cases in future object/funnel
benchmarks; the current scalar benchmark does not measure this cost.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -203,15 +258,15 @@ public List<Object[]> getResult(Comparator<Object[]>
comparator, int maxRows) {
int idx = 0;
while (idx < numGroups && groupKeyIterator.hasNext()) {
- Object[] row = getRow(groupKeyIterator, numKeys, numFunctions,
resultStoredTypes);
+ T row = decorate.apply(getRow(groupKeyIterator, numKeys, numFunctions,
resultStoredTypes));
sortedRows.add(row);
idx++;
}
while (groupKeyIterator.hasNext()) {
idx++;
// TODO: allocate new array row only if row enters set
- Object[] row = getRow(groupKeyIterator, numKeys, numFunctions,
resultStoredTypes);
+ T row = decorate.apply(getRow(groupKeyIterator, numKeys, numFunctions,
resultStoredTypes));
Review Comment:
Non-blocking performance follow-up: the object path creates a `SortedRow`
and sort-key array for every candidate, including candidates immediately
discarded. Only top-K wrappers remain in the heap, but high group counts may
produce substantial short-lived allocation. The scalar SUM benchmark does not
cover this path. It would be useful to measure allocation and latency for
object aggregates at high group counts before restructuring the shared heap
loop. Also, caching a native raw-HLL wrapper avoids repeated extraction but
does not cache its comparison cost: `SerializedHLL.compareTo()` still calls
`cardinality()` on both operands.
--
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]
