asdf2014 commented on code in PR #19678:
URL: https://github.com/apache/druid/pull/19678#discussion_r3580859181
##########
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. Both checks in that line operate on decoded scalars, so the
nibble masks were wrong on both sides: `(lowerNibble & 0x0f) == 0` marks
decoded 16/32/48 as empty (the odd-bucket case you describe, which estimated
0), and `(upperNibble & 0xf0) == 0` marks decoded 1-15 as empty, so a populated
neighbor register sharing the overflow byte was miscounted as well. Replaced
both with direct `== 0` tests on the decoded values.
The same pattern exists in `estimateDense`, where the position comparison
has always matched the overflow byte. A dense collector holding a single
odd-bucket overflow register (for example `add((short) 5, (byte) 3)` followed
by `add((short) 5, (byte) 16)`, with no serialization round-trip) also
estimated 0, independently of this PR's earlier changes. Fixed that occurrence
the same way.
Added regression tests: odd-bucket sparse round-trip (estimates 1), odd
bucket with a populated neighbor in the same byte (estimates 2), and dense
even/odd overflow without a round-trip (1 each).
--
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]
