xiangfu0 commented on PR #19264:
URL: https://github.com/apache/pinot/pull/19264#issuecomment-5349019713
Pushed eba1229, which reworks base aggregation to do the grouping-set
fan-out **once, in parallel, at combine time** — removing the need for the
cardinality gate entirely (base aggregation is now a strict win in all regimes).
**Change:** segments emit only the BASE groups (union grouping, no
`$groupingId`). `GroupByCombineOperator` merges base groups across segments,
then in `mergeResults()` derives the individual grouping sets **once**,
parallelized across the combine thread pool by base-group ranges into a shared
concurrent grouping-set table. Total work is minimal (each row grouped once,
never per set) and the fan-out is multi-threaded. The prior
`groupingSetsBaseAggregationMaxGroups` gate/option is removed. Object
intermediates are cloned per derived record so concurrent
cross-set/cross-thread merges stay exact.
**Benchmark (50 segments × 15k rows, base aggregation vs.
`groupingSetsBaseAggregation=false` expansion):**
| Query | Low-card `EXP(0.5)` | High-card `EXP(0.001)` |
|---|---|---|
| `ROLLUP(D1,D2)` | 1.3 vs 13.4 ms — **~10x** | 2.5 vs 231.6 ms — **~91x** |
| `ROLLUP(D1,D2,D3)` | 1.5 vs 24.0 ms — **~16x** | 10.8 vs 286.7 ms —
**~27x** |
| `CUBE(D1,D2,D3)` | 1.5 vs 37.7 ms — **~25x** | 11.2 vs 462.2 ms — **~41x**
|
| `GROUPING SETS (5)` | 1.6 vs 19.1 ms — **~12x** | 11.3 vs 246.4 ms —
**~22x** |
High-cardinality CUBE went from ~2x *slower* (the regression the gate
protected against) to **~40x faster** than expansion. (Small-dataset
micro-numbers with wide error bars, but the order-of-magnitude difference is
unambiguous.)
**Correctness fix (found by review):** `IndexedTable` now derives its
key-column count from the schema (`columns − aggregations`) instead of always
from `getNumGroupByKeyColumns()`. The base combine table's records omit the
`$groupingId` column, so the old fixed count merged aggregations at the wrong
offset — dropping the first aggregation's cross-segment contribution and
reading out of bounds when a base key repeated across segments. Added
`DeriveGroupingSetsTest`, which exercises the parallel derive across thread
counts {1,2,4,8} and the cross-segment base merge; it reproduces the
`ArrayIndexOutOfBoundsException` without the fix and passes with it.
All 75 grouping-sets integration tests + the new unit tests pass;
spotless/license/checkstyle clean.
--
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]
