From 21a97f4c611b86789135fdd4ac99bd17576a4f2c Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 7 Feb 2024 18:44:28 -0800 Subject: [PATCH] Fix HllSketchHolderObjectStrategy#isSafeToConvertToNullSketch. (#15860) * Fix HllSketchHolderObjectStrategy#isSafeToConvertToNullSketch. The prior code from #15162 was reading only the low-order byte of an int representing the size of a coupon set. As a result, it would erroneously believe that a coupon set with a multiple of 256 elements was empty. --- .../hll/HllSketchHolderObjectStrategy.java | 9 ++- ...=> HllSketchHolderObjectStrategyTest.java} | 75 ++++++++++++++++++- 2 files changed, 79 insertions(+), 5 deletions(-) rename extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/{HllSketchObjectStrategyTest.java => HllSketchHolderObjectStrategyTest.java} (50%) diff --git a/extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchHolderObjectStrategy.java b/extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchHolderObjectStrategy.java index 93597a2621e..60b1cc87461 100644 --- a/extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchHolderObjectStrategy.java +++ b/extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchHolderObjectStrategy.java @@ -85,7 +85,7 @@ public class HllSketchHolderObjectStrategy implements ObjectStrategy objectStrategy.fromByteBufferSafe(buf4, garbageLonger.length).getSketch().copy() ); } + + @Test + public void testHllSketchIsNullEquivalent() + { + final Random random = new Random(0); + for (final TgtHllType tgtHllType : TgtHllType.values()) { + for (int lgK = 7; lgK < 22; lgK++) { + for (int sz : new int[]{0, 1, 2, 127, 128, 129, 255, 256, 257, 511, 512, 513, 16383, 16384, 16385}) { + final String description = StringUtils.format("tgtHllType[%s], lgK[%s], sz[%s]", tgtHllType, lgK, sz); + final HllSketch sketch = new HllSketch(lgK, tgtHllType); + for (int i = 0; i < sz; i++) { + sketch.update(random.nextLong()); + } + + final boolean expectEmpty = sz == 0; + + // -------------------------------- + // Compact array, little endian buf + final byte[] compactBytes = sketch.toCompactByteArray(); + // Add a byte of padding on either side + ByteBuffer buf = ByteBuffer.allocate(compactBytes.length + 2); + buf.order(ByteOrder.LITTLE_ENDIAN); + buf.position(1); + buf.put(compactBytes); + buf.position(1); + Assert.assertEquals( + "Compact array littleEndian " + description, + expectEmpty, + HllSketchHolderObjectStrategy.isSafeToConvertToNullSketch(buf, compactBytes.length) + ); + Assert.assertEquals(1, buf.position()); + + // ----------------------------- + // Compact array, big endian buf + buf.order(ByteOrder.BIG_ENDIAN); + Assert.assertEquals( + "Compact array bigEndian " + description, + expectEmpty, + HllSketchHolderObjectStrategy.isSafeToConvertToNullSketch(buf, compactBytes.length) + ); + Assert.assertEquals(1, buf.position()); + + // ---------------------------------- + // Updatable array, little endian buf + final byte[] updatableBytes = sketch.toUpdatableByteArray(); + // Add a byte of padding on either side + buf = ByteBuffer.allocate(updatableBytes.length + 2); + buf.order(ByteOrder.LITTLE_ENDIAN); + buf.position(1); + buf.put(updatableBytes); + buf.position(1); + Assert.assertEquals( + "Updatable array littleEndian " + description, + expectEmpty, + HllSketchHolderObjectStrategy.isSafeToConvertToNullSketch(buf, updatableBytes.length) + ); + Assert.assertEquals(1, buf.position()); + + // ------------------------------- + // Updatable array, big endian buf + buf.order(ByteOrder.BIG_ENDIAN); + Assert.assertEquals( + "Updatable array bigEndian " + description, + expectEmpty, + HllSketchHolderObjectStrategy.isSafeToConvertToNullSketch(buf, updatableBytes.length) + ); + Assert.assertEquals(1, buf.position()); + } + } + } + } }