xiangfu0 commented on code in PR #19800:
URL: https://github.com/apache/pinot/pull/19800#discussion_r4253416921
##########
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:
Fixed in
[01a311b119](https://github.com/apache/pinot/commit/01a311b11948fc4b23909e51af1daaef7e12f227).
The first cardinality is read directly; single-step funnels and empty operands
do not copy or intersect populations. The first nonempty intersection uses
`RoaringBitmap.and()`, later steps reuse that private result, and empty
intersections stop evaluation. Input bitmaps remain intact for both default and
partitioned callers.
Local validation: 27 focused
finalizer/null-handling/bitmap/partitioned/partitioned-sorted tests passed,
including repeated extraction followed by merging. All four `pinot-core`
formatting/checkstyle/license checks passed, with no warnings on added lines.
Two regressions cover no-clone single/empty cases, later private intersections,
unchanged populations and stopping before later steps; both fail against the
preceding `4292f86188` finalizer and pass with this change.
The push is verified. Hosted CI is running for the new head. The approval at
`4292f86188` remains recorded; this follow-up commit has not had a new human
review.
##########
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:
Agreed. The object-sort path allocates a `SortedRow` and key array for each
candidate, including discarded candidates. Lazy caching avoids repeated
extraction for reached collations; `SerializedHLL.compareTo()` still calls
`cardinality()` on both operands. The scalar SUM benchmark supplies no
allocation or latency evidence for these object costs.
I have kept the shared heap loop unchanged in this bug fix. Following your
non-blocking recommendation, object measurements remain a follow-up: high group
counts, varying top-K, allocation and latency, including raw HLL. Leaving this
thread open for that measurement rather than marking it fixed.
##########
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:
Agreed. Nonempty window funnels make a shallow O(N) copy of event references
before MaxStep/MatchStep can terminate on completion, preserving the
accumulator for later extraction and merging. The events themselves are reused.
The scalar benchmark does not measure this cost.
Following your non-blocking recommendation, future funnel measurements
should include large queues completing in the first window and queues requiring
full evaluation, with first-window size considered separately from total queue
size. Leaving this thread open for those measurements.
--
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]
