davecromberge opened a new pull request, #19449:
URL: https://github.com/apache/pinot/pull/19449
## What
When an accumulator reaches its threshold, `applyInternal` calls
`getResult()` to push the pending
batch into the union, and throws the returned sketch away:
```java
if (_accumulator.size() >= _threshold) {
getResult();
_accumulator.clear();
}
```
The union is the part that's needed. Building a sketch out of it is not.
This splits the union out of `getResult()` into a new `flush()`, and calls
that instead. For Theta
the discarded result was five passes over the union gadget's hash table plus
a compact sketch of up
to `nominalEntries` hashes, on every batch; for Tuple it also copied every
summary.
Merging 400 stored sketches at `accumulatorThreshold=10`:
| | before | after |
| --- | --- | --- |
| Theta, lgK=14 | 16.01 ms / 17.92 MB | **5.20 ms / 2.91 MB** |
| Tuple, lgK=12 | 9.05 ms / 9.97 MB | **4.26 ms / 2.58 MB** |
| CPC, lgK=10 | 0.39 ms / 3.20 MB | 0.43 ms / 3.14 MB |
CPC is unchanged — `CpcUnion.getResult()` is cheap next to Theta's. It's
included for consistency
and for the fix below.
Profiling a production cluster (16 servers, 80 minutes of live traffic) put
`ThetaUnionImpl.getResult` at 10.3% of all server CPU, and 21% in a
Theta-heavy window. Almost all
of it was discarded.
## It also fixes data loss at accumulatorThreshold=1
The threshold branch called `getResult()`, which returns the single
accumulated sketch *without*
unioning it, and `applyInternal` then cleared the accumulator — so the first
sketch of every group
was silently dropped, and a later `getResult()` hit
`IndexOutOfBoundsException` on the empty list.
`flush()` always unions, and the single-sketch shortcut now checks the
sketch is still pending.
Seven sketches of 1000 distinct values each, at threshold 1, before this
change:
```
ThetaSketchAccumulatorTest expected [6960.14] but found [5914.93]
CpcSketchAccumulatorTest expected [7000.78] but found [6003.72]
TupleIntSketchAccumulatorTest expected [6960.14] but found [5914.93]
```
## Tests
No existing test crossed a threshold — `testThresholdBehavior` sets it to 3
and applies 2 sketches
— so the flush path was uncovered, which is how both problems survived. Each
accumulator test now
merges seven sketches at thresholds one through four and compares against a
direct union of the
same sketches. The Tuple one also compares summary sums, since summaries are
summed and a sketch
merged twice doubles a summary while leaving the estimate correct.
## Note for reviewers
`flush()` is `protected abstract`, so any accumulator outside this repo
would need to implement it.
--
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]
