asdf2014 commented on code in PR #19678:
URL: https://github.com/apache/druid/pull/19678#discussion_r3571071420
##########
processing/src/main/java/org/apache/druid/hll/HyperLogLogCollector.java:
##########
@@ -190,12 +191,22 @@ private static double estimateSparse(
}
e += 1.0d / Math.pow(2, upperNibble) + 1.0d / Math.pow(2, lowerNibble);
zeroCount += (((upperNibble & 0xf0) == 0) ? 1 : 0) + (((lowerNibble &
0x0f) == 0) ? 1 : 0);
+ overflowRegisterApplied = true;
Review Comment:
Confirmed. In the sparse buffer `position` is the serialized offset (payload
byte index + `getNumHeaderBytes()`), whereas `overflowPosition` is
`overflowRegister >>> 1`, so the in-loop check never matched the overflow
bucket's own byte and `overflowRegisterApplied` was unreliable. In the
in-range-then-overflow case the register was then counted twice — its sparse
entry plus the fallback — estimating 2 for a single populated register.
Fixed by normalizing `position` to the payload byte index (`position -
numHeaderBytes`) before comparing with `overflowPosition`, so the in-loop
overflow handling matches the correct entry and the fallback only fires when
the overflow bucket has no sparse entry. Added regression tests for the
single-register round-trip (now estimates 1, was 2) and for an overflow
register sitting alongside other sparse entries.
--
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]
