xiangfu0 commented on code in PR #19800:
URL: https://github.com/apache/pinot/pull/19800#discussion_r4253915032
##########
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:
Measured the object path before restructuring it, then fixed wrapper
allocation in
[1ca3cc9edc](https://github.com/apache/pinot/commit/1ca3cc9edc04a49b45236d02f63b2cf700109fec).
Rejected candidates and entries polled from the heap are recycled, with their
cached keys reset before reuse. This bounds `SortedRow` and key-array
allocation to K+1 per trim; heap-owned entries are never modified and the
original intermediate payloads remain mergeable.
| Aggregate | K | Before ms/op | After ms/op | Before B/op | After B/op |
|---|---:|---:|---:|---:|---:|
| AVG | 500 | 14.262 ± 6.078 | 7.525 ± 2.529 | 11,202,562 | 6,426,586 |
| AVG | 5,000 | 33.579 ± 29.211 | 16.044 ± 1.817 | 11,238,629 | 6,678,616 |
| Raw HLL | 500 | 1366.643 ± 146.105 | 1160.987 ± 24.530 | 10,406,050 |
5,630,094 |
| Raw HLL | 5,000 | 4282.882 ± 267.221 | 4182.569 ± 51.501 | 10,445,520 |
5,885,589 |
| SUM control | 500 | 4.227 ± 0.484 | 3.807 ± 0.099 | 4,002,223 | 4,002,221 |
| SUM control | 5,000 | 10.065 ± 0.464 | 8.819 ± 0.572 | 4,038,243 |
4,038,239 |
Total trim allocation falls 40.6–45.9% for the measured OBJECT cases. Native
`SerializedHLL.compareTo()` still recomputes both operands' cardinalities:
candidate raw-HLL trims take about 1.16s at K=500 and 4.18s at K=5,000 here.
This change deliberately retains that native comparison contract; it does not
add a public sketch cache or aggregation-function allowlist.
JMH 1.37, Temurin 25.0.4.1 on an Apple M4 Pro shared host; one thread, two
forks, 3 × 1s warmup and 5 measured iterations per fork, GC profiler, 512 MiB
initial/1 GiB maximum heap. Object measurements use 2s iterations; funnel/copy
measurements use 1s. Errors shown are JMH 99.9% confidence errors.
The object fixture invokes production `getResult()` on 100,000 preaggregated
groups, aggregate DESC then INT group ASC, with varied AVG counts and
independent raw-HLL log2m=12 sketches containing 1–32 distinct inputs/group.
Fixture creation, merging and correctness checks run in trial setup before
warmup; the measured operation includes comparator construction, trimming and
result draining. There are no allocating teardown checks. All 22 requested
parameter cases completed and passed the independent result/preservation
checks. The harness and raw results remain outside the PR.
These are trim/finalizer measurements, excluding upstream aggregation,
planning, networking and whole-query QPS/p95/p99. The baseline AVG timings are
noisy, so I am claiming the measured allocation reduction rather than a general
latency speedup. SUM allocation is effectively unchanged; the observed control
timings show no regression in these fixtures.
Validation: 67 focused MSE/funnel tests passed, as did all four
affected-module `pinot-query-runtime` formatting/checkstyle/license checks. The
new regression checks recycled-entry resets, ties, nulls, repeated trimming and
payload preservation; a missing-reset mutant fails it. The fork branch and live
PR head are verified at the pushed commit. Exact-head hosted CI is running. The
recorded human approval is on the earlier `4292f86188` head; this follow-up has
not received a fresh human review.
##########
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:
Completed the requested large-queue measurements for production MaxStep and
MatchStep finalization at the pushed
[1ca3cc9edc](https://github.com/apache/pinot/commit/1ca3cc9edc04a49b45236d02f63b2cf700109fec)
head. The window-finalizer sources are unchanged from the measured
`01a311b119` overlay. I kept the private queue copy because repeated extraction
and subsequent merging must preserve the accumulator.
| Function | Events | First-window completion µs/op | Full traversal µs/op |
Early B/op | Full B/op |
|---|---:|---:|---:|---:|---:|
| MAX_STEP | 10,000 | 1.330 ± 0.285 | 470.149 ± 18.146 | 40,160 | 40,163 |
| MAX_STEP | 100,000 | 8.430 ± 0.220 | 6241.274 ± 356.042 | 400,132 |
400,203 |
| MATCH_STEP | 10,000 | 0.914 ± 0.013 | 422.043 ± 12.989 | 40,160 | 40,219 |
| MATCH_STEP | 100,000 | 8.581 ± 0.400 | 5981.614 ± 117.375 | 400,160 |
400,258 |
The early case completes with events at timestamps 0, 1 and 2 in a window of
100, followed by a distant tail. It therefore terminates in the first
three-event window while still copying all N references. The full case has no
step-0 event, so it traverses the entire queue searching for a start; it is not
a successful multi-window funnel.
Separately, the exact JDK constructor used by the finalizer (`new
PriorityQueue<>(queue)`) measured:
| Queue events | Shallow copy µs/op | B/op |
|---:|---:|---:|
| 10,000 | 0.912 ± 0.093 | 40,048 |
| 100,000 | 8.454 ± 0.883 | 400,048 |
This confirms the O(N) shallow reference-copy allocation, including early
completion; the events themselves are shared. The copy measurements are
separate fixtures and should not be subtracted from finalization means to claim
exact evaluator time.
JMH 1.37, Temurin 25.0.4.1 on an Apple M4 Pro shared host; one thread, two
forks, 3 × 1s warmup and 5 measured iterations per fork, GC profiler, 512 MiB
initial/1 GiB maximum heap. Object measurements use 2s iterations; funnel/copy
measurements use 1s. Errors shown are JMH 99.9% confidence errors.
Fixtures are constructed before warmup. Setup checks two extractions and
unchanged queue contents/event references after each; no teardown allocations
contaminate the GC measurements. All eight finalizer cases and both
isolated-copy cases completed. These are shared-host absolute costs, excluding
real query resource-accounting/cancellation workloads and whole-query latency.
The harness and raw results remain outside the PR.
Validation at the verified pushed head: 67 focused MSE/funnel tests passed,
including repeated extraction followed by merging for all five window variants,
plus all four affected-module checks. Exact-head hosted CI is running; the
existing human approval remains on the earlier `4292f86188` head.
--
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]
