xiangfu0 commented on code in PR #20154:
URL: https://github.com/apache/pinot/pull/20154#discussion_r4474009131
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLPlusValueAggregator.java:
##########
@@ -77,7 +78,7 @@ public HyperLogLogPlus getInitialAggregatedValue(@Nullable
Object rawValue) {
} else {
initialValue = new HyperLogLogPlus(_p, _sp);
offerValue(initialValue, rawValue);
- _maxByteSize = HyperLogLogPlusUtils.byteSize(_p, _sp);
+ _maxByteSize = HyperLogLogPlusUtils.maxByteSize(_p, _sp);
Review Comment:
Non-blocking, predates this PR: the `byte[]` branch above (line 77) still
records the first input's length. That isn't an upper bound for sparse (`sp >
0`) sketches. In a local probe at p=12, sp=20,
`getMaxAggregatedValueByteSize()` returned 11 after a 1-value sketch, and the
merged value then serialized to 3817 bytes. So a star-tree over a
pre-serialized HLL++ BYTES column with `sp > 0` can still get the undersized
V2/V3 max entry length this PR fixes for raw input. A follow-up could derive
the bound from the input sketch's p/sp.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLPlusValueAggregator.java:
##########
@@ -132,7 +133,7 @@ public boolean isAggregatedValueFixedSize() {
public int getMaxAggregatedValueByteSize() {
// NOTE: For aggregated metrics, initial aggregated value might have not
been generated. Returns the byte size
// based on p and sp.
- return _maxByteSize > 0 ? _maxByteSize : HyperLogLogPlusUtils.byteSize(_p,
_sp);
+ return _maxByteSize > 0 ? _maxByteSize :
HyperLogLogPlusUtils.maxByteSize(_p, _sp);
Review Comment:
Non-blocking, predates this PR: with `sp > 0` this bound never equals the
actual length, but `isAggregatedValueFixedSize()` (line 128) still returns
`true`. So `distinctCountHLLPlus(metric, 12, 20)` passes ingestion-aggregation
validation and then fails on the first row: `Expected value size to be: 15369
but got: 11` (local probe at this SHA; base fails the same way with 2744).
Returning `_sp == 0` there would turn this into a table-config validation error.
--
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]
