xiangfu0 commented on code in PR #20228:
URL: https://github.com/apache/pinot/pull/20228#discussion_r4540330324
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/IntegerTupleSketchValueAggregator.java:
##########
@@ -126,22 +127,13 @@ public boolean isAggregatedValueFixedSize() {
return true;
}
- /// Returns the maximum number of storage bytes required for a Compact
Integer Tuple Sketch with the given
- /// number of actual entries. Note that this assumes the worst case of the
sketch in
- /// estimation mode, which requires storing theta and count.
- /// @return the maximum number of storage bytes required for a Compact
Integer Tuple Sketch with the given number
- /// of entries.
+ /// Returns the maximum serialized size after unioning at the configured
nominal entries. The union rounds the
+ /// nominal entries up to a power of two. In estimation mode the compact
sketch has a 24-byte header, followed by
+ /// an 8-byte hash and a 5-byte integer summary (value and mode) for each
retained entry.
@Override
public int getMaxAggregatedValueByteSize() {
- if (_nominalEntries == 0) {
- return 8;
- }
- if (_nominalEntries == 1) {
- return 16;
- }
- int longSizeInBytes = Long.BYTES;
- int intSizeInBytes = Integer.BYTES;
- return (_nominalEntries * (longSizeInBytes + intSizeInBytes)) + 24;
+ long nominalEntries = Util.ceilingPowerOf2(_nominalEntries);
+ return Math.toIntExact(nominalEntries * (Long.BYTES + Integer.BYTES +
Byte.BYTES) + 24);
Review Comment:
Non-blocking, informational: the bound is correct, but `MutableSegmentImpl`
reserves this many bytes per doc for aggregated metrics. At the default lgK=14
that grows from 196,632 to 213,016 B (+8.3%). For non-power-of-2 nominal
entries the increase is larger, e.g. k=1000 goes from 12,024 to 13,336 B
(+10.9%). Consider a release note for operators sizing consuming segments that
use tuple-sketch `aggregateMetrics`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/TupleIntSketchAccumulator.java:
##########
@@ -86,20 +86,14 @@ protected void flush() {
@Override
public TupleSketch<IntegerSummary> getResult() {
- if (_union == null) {
- _union = new TupleUnion<>(_nominalEntries, _setOperations);
- }
- // Return the default update "gadget" sketch as a compact sketch
- if (isEmpty()) {
- return _union.getResult();
- }
- // Corner-case: the parameters are not strictly respected when there is a
single sketch.
- // This single sketch might have been the result of a previously
accumulated union and
- // would already have the parameters set. The sketch is returned as-is
without adjusting
- // nominal entries which requires an additional union operation.
+ // A deserialized accumulator contains one sketch but does not yet have
union parameters set.
Review Comment:
Nit: this shortcut applies to any single-input accumulator, not only
deserialized ones. For example, a parameterized accumulator that received one
sketch returns it without trimming to `_nominalEntries`. The Theta and CPC
versions keep the original "corner-case" note. Consider keeping it here too so
the three stay consistent.
--
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]
