Skip to content

fix: HyperLogLogCollector returns zero cardinality when a single element overflows into sparse mode #19678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asdf2014 wants to merge 3 commits into
base: master
Choose a base branch
from
+126 −5
Open

fix: HyperLogLogCollector returns zero cardinality when a single element overflows into sparse mode #19678

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
483a1b2
fix: HyperLogLogCollector returns zero cardinality when a single elem…
asdf2014 Jul 12, 2026
a37a8a8
Normalize sparse position before matching the overflow register
asdf2014 Jul 13, 2026
69a616a
Check decoded register values against zero when applying the overflow…
asdf2014 Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions processing/src/main/java/org/apache/druid/hll/HyperLogLogCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ private static double estimateSparse(
final ByteBuffer copy = buf.asReadOnlyBuffer();
double e = 0.0d;
int zeroCount = NUM_BUCKETS - 2 * (buf.remaining() / 3);
boolean overflowRegisterApplied = false;
while (copy.hasRemaining()) {
short position = copy.getShort();
final int register = (int) copy.get() & 0xff;
Expand All @@ -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;

@FrankChen021 FrankChen021 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Normalize sparse positions before marking overflow applied

position is the serialized byte offset, which toByteBuffer() writes as the payload index plus getNumHeaderBytes() (7 for a V1 sketch), while overflowPosition is only overflowRegister >>> 1. Consequently this flag can remain false after visiting the overflow bucket's actual sparse byte, or become true for an unrelated entry. For example, if a bucket first receives an in-range value and then an overflow value, a sparse round-trip stores its tuple at overflowPosition + 7; the new fallback then treats the overflow as a second previously empty register and decrements zeroCount again, making one populated HLL register estimate approximately 2 instead of 1. Compare positions in the same coordinate system before deciding whether standalone compensation is needed.

@asdf2014 asdf2014 Jul 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@FrankChen021 FrankChen021 Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks—the serialized-position normalization fixes the original double-count for upper-nibble overflow buckets. One parity case remains: for an odd overflow bucket, lowerNibble is already decoded to a scalar (for example, 16), so ((lowerNibble & 0x0f) == 0) incorrectly marks it as zero. add((short) 5, (byte) 3), then add((short) 5, (byte) 16), followed by a sparse round-trip therefore reaches zeroCount == NUM_BUCKETS and estimates 0. Please test decoded values with upperNibble == 0 / lowerNibble == 0 and add odd-bucket regression coverage.

Reviewed 2 of 2 changed files.

@asdf2014 asdf2014 Jul 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

} else {
e += MIN_NUM_REGISTER_LOOKUP[minNum][register];
zeroCount += NUM_ZERO_LOOKUP[register];
}
}

// A sparse buffer only stores registers set through the regular nibble range. When the only information about
// a bucket lives in the overflow register (positionOf1 exceeded registerOffset + RANGE) and that bucket is not
// among the entries above, it was implicitly counted as an empty register. Account for it explicitly so linear
// counting does not treat the collector as empty and estimate a cardinality of zero.
if (overflowValue != 0 && !overflowRegisterApplied) {
zeroCount -= 1;
e += 1.0d / Math.pow(2, overflowValue);
}

e += zeroCount;
return applyCorrection(e, zeroCount);
}
Expand Down
43 changes: 43 additions & 0 deletions processing/src/test/java/org/apache/druid/hll/HyperLogLogCollectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,49 @@ public void testEstimation()
Assert.assertEquals(expectedVals[valsToCheckIndex], collector.estimateCardinality(), 0.0d);
}

@Test
public void testSparseOverflowRegisterEstimation()
{
// Reproduces the case where a single element whose positionOf1 exceeds the 4-bit nibble range
// (registerOffset 0 + RANGE 15) is stored only in the overflow register, leaving the sparse buffer empty.
// estimateSparse() must still account for that one non-empty register instead of reporting a cardinality of 0.

// Direct form: bucket 5 with positionOf1 = 16 (> RANGE) goes straight to the overflow register.
HyperLogLogCollector direct = HyperLogLogCollector.makeLatestCollector();
direct.add((short) 5, (byte) 16);
Assert.assertEquals(1L, direct.estimateCardinalityRound());
Assert.assertEquals(1.0d, direct.estimateCardinality(), 0.05d);

// Same situation reached through the byte[] hashing path used during ingestion: leading bytes 0x00 0x80
// give positionOf1 = 16, and the trailing bytes select the bucket.
byte[] hashedValue = new byte[16];
hashedValue[1] = (byte) 0x80;
hashedValue[15] = 0x05;
HyperLogLogCollector hashed = HyperLogLogCollector.makeLatestCollector();
hashed.add(hashedValue);
Assert.assertEquals(1L, hashed.estimateCardinalityRound());
}

@Test
public void testSparseOverflowRegisterSharesByteWithEntry()
{
// Adding regular (in-range) registers converts the collector to dense storage, so estimateSparse's overflow
// handling can only be reached via a sparse-serialized buffer. Build a low-cardinality collector holding both
// regular registers and an overflow register, then round-trip it through toByteArray() (which serializes
// sparsely while numNonZeroRegisters < DENSE_THRESHOLD). The restored collector uses estimateSparse, and the
// overflow register coincides with a populated sparse entry, so the in-loop overflow branch runs instead of the
// standalone compensation added for the empty-overflow case.
HyperLogLogCollector source = HyperLogLogCollector.makeLatestCollector();
for (int bucket = 0; bucket < 60; bucket++) {
source.add((short) bucket, (byte) 1);
}
source.add((short) 30, (byte) 16); // overflow register lands on an already-populated byte

HyperLogLogCollector sparse = HyperLogLogCollector.makeCollector(ByteBuffer.wrap(source.toByteArray()));
long estimate = sparse.estimateCardinalityRound();
Assert.assertTrue("expected a sane non-zero estimate near 60, got " + estimate, estimate >= 40 && estimate <= 90);
}

@Test
public void testEstimationReadOnlyByteBuffers()
{
Expand Down
Loading