Merge pull request #486 from apache/fix_issue_484

Fix Issue 484.
apache · Dec 21, 2023 · 4362127 · 4362127
2 parents 4d43fc3 + 62721f7
commit 4362127
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 7 deletions.
diff --git a/src/main/java/org/apache/datasketches/kll/KllDirectCompactItemsSketch.java b/src/main/java/org/apache/datasketches/kll/KllDirectCompactItemsSketch.java
@@ -32,6 +32,7 @@
 import java.lang.reflect.Array;
 import java.util.Comparator;
 
+import org.apache.datasketches.common.ArrayOfBooleansSerDe;
 import org.apache.datasketches.common.ArrayOfItemsSerDe;
 import org.apache.datasketches.common.SketchesArgumentException;
 import org.apache.datasketches.memory.Memory;
@@ -154,6 +155,7 @@ byte[] getMinMaxByteArr() { //this is only used by COMPACT_FULL
   @Override
   int getMinMaxSizeBytes() { //this is only used by COMPACT_FULL
     final int offset = DATA_START_ADR + getNumLevels() * Integer.BYTES;
+    if (serDe instanceof ArrayOfBooleansSerDe) { return 2; }
     return serDe.sizeOf(mem, offset, 2);
   }
 

diff --git a/src/main/java/org/apache/datasketches/kll/KllHelper.java b/src/main/java/org/apache/datasketches/kll/KllHelper.java
@@ -432,13 +432,13 @@ static  byte[] toByteArray(final KllSketch srcSk, final boolean updatable) {
     final byte m = (byte) srcSk.getM();
 
     //load first 8 bytes
-    wbuf.putByte(preInts);
+    wbuf.putByte(preInts); //byte 0
     wbuf.putByte(serVer);
     wbuf.putByte(famId);
     wbuf.putByte(flags);
     wbuf.putShort(k);
     wbuf.putByte(m);
-    wbuf.incrementPosition(1);
+    wbuf.incrementPosition(1); //byte 7 is unused
 
     if (tgtStructure == COMPACT_EMPTY) {
       return bytesOut;

diff --git a/src/main/java/org/apache/datasketches/kll/KllMemoryValidate.java b/src/main/java/org/apache/datasketches/kll/KllMemoryValidate.java
@@ -36,6 +36,7 @@
 import static org.apache.datasketches.kll.KllSketch.SketchType.FLOATS_SKETCH;
 import static org.apache.datasketches.kll.KllSketch.SketchType.ITEMS_SKETCH;
 
+import org.apache.datasketches.common.ArrayOfBooleansSerDe;
 import org.apache.datasketches.common.ArrayOfItemsSerDe;
 import org.apache.datasketches.common.Family;
 import org.apache.datasketches.common.SketchesArgumentException;
@@ -175,7 +176,11 @@ static int computeSketchBytes( //for COMPACT_FULL or UPDATABLE only
 
     int offsetBytes = DATA_START_ADR + levelsLen * Integer.BYTES;
     if (sketchType == ITEMS_SKETCH) {
-      offsetBytes += serDe.sizeOf(srcMem, offsetBytes, numItems + 2); //2 for min & max
+      if (serDe instanceof ArrayOfBooleansSerDe) {
+        offsetBytes += serDe.sizeOf(srcMem, offsetBytes, numItems) + 2; //2 for min & max
+      } else {
+        offsetBytes += serDe.sizeOf(srcMem, offsetBytes, numItems + 2); //2 for min & max
+      }
     } else {
       final int typeBytes = sketchType.getBytes();
       offsetBytes += (numItems + 2) * typeBytes; //2 for min & max

diff --git a/src/test/java/org/apache/datasketches/common/ArrayOfXSerDeTest.java b/src/test/java/org/apache/datasketches/common/ArrayOfXSerDeTest.java
@@ -34,7 +34,7 @@ public void checkBooleanItems() {
     WritableMemory wmem;
     ArrayOfBooleansSerDe serDe = new ArrayOfBooleansSerDe();
 
-    Boolean[] items = {true,false,true,true,false,false};
+    Boolean[] items = {true,false,true,false,true,false,true,false,true,false};
     bytes = serDe.sizeOf(items);
     byteArr = serDe.serializeToByteArray(items);
     assertEquals(byteArr.length, bytes);

diff --git a/src/test/java/org/apache/datasketches/kll/KllMiscItemsTest.java b/src/test/java/org/apache/datasketches/kll/KllMiscItemsTest.java
@@ -27,6 +27,7 @@
 
 import java.util.Comparator;
 
+import org.apache.datasketches.common.ArrayOfBooleansSerDe;
 import org.apache.datasketches.common.ArrayOfStringsSerDe;
 import org.apache.datasketches.common.SketchesArgumentException;
 import org.apache.datasketches.common.Util;
@@ -437,6 +438,33 @@ public void checkGetSingleItem() {
     assertEquals(skCompact.getSingleItem(), "1");
   }
 
+  @Test
+  public void checkIssue484() {
+    Boolean[] items = { true,false,true,false,true,false,true,false,true,false };
+    KllItemsSketch<Boolean> sketch = KllItemsSketch.newHeapInstance(Boolean::compareTo, new ArrayOfBooleansSerDe());
+    for (int i = 0; i < items.length; i++) { sketch.update(items[i]); }
+    byte[] serialized = sketch.toByteArray();
+    KllItemsSketch<Boolean> deserialized =
+        KllItemsSketch.wrap(Memory.wrap(serialized), Boolean::compareTo, new ArrayOfBooleansSerDe());
+    checkSketchesEqual(sketch, deserialized);
+  }
+
+  private static <T> void checkSketchesEqual(KllItemsSketch<T> expected, KllItemsSketch<T> actual) {
+    KllItemsSketchSortedView<T> expSV = expected.getSortedView();
+    KllItemsSketchSortedView<T> actSV = actual.getSortedView();
+    int N = (int)actSV.getN();
+    long[] expCumWts = expSV.getCumulativeWeights();
+    Boolean[] expItemsArr = (Boolean[])expSV.getQuantiles();
+    long[] actCumWts = actSV.getCumulativeWeights();
+    Boolean[] actItemsArr = (Boolean[])actSV.getQuantiles();
+    printf("%3s %8s %8s\n", "i","Actual", "Expected");
+    for (int i = 0; i < N; i++) {
+      printf("%3d %8s %8s\n", i, actItemsArr[i].toString(), expItemsArr[i].toString());
+    }
+    assertEquals(actCumWts, expCumWts);
+    assertEquals(actItemsArr, expItemsArr);
+  }
+
   private final static boolean enablePrinting = false;
 
   /**

diff --git a/src/test/java/org/apache/datasketches/quantiles/ItemsSketchTest.java b/src/test/java/org/apache/datasketches/quantiles/ItemsSketchTest.java
@@ -30,6 +30,7 @@
 import java.util.Arrays;
 import java.util.Comparator;
 
+import org.apache.datasketches.common.ArrayOfBooleansSerDe;
 import org.apache.datasketches.common.ArrayOfDoublesSerDe;
 import org.apache.datasketches.common.ArrayOfItemsSerDe;
 import org.apache.datasketches.common.ArrayOfLongsSerDe;
@@ -647,16 +648,53 @@ public void getQuantiles() {
     assertEquals(quantiles1, quantiles2);
   }
 
+  @Test
+  public void checkIssue484() {
+    Boolean[] items = { true,false,true,false,true,false,true,false,true,false };
+    ItemsSketch<Boolean> sketch = ItemsSketch.getInstance(Boolean.class, Boolean::compareTo);
+    for (int i = 0; i < items.length; i++) { sketch.update(items[i]); }
+    byte[] serialized = sketch.toByteArray(new ArrayOfBooleansSerDe());
+    ItemsSketch<Boolean> deserialized =
+        ItemsSketch.getInstance(Boolean.class, Memory.wrap(serialized), Boolean::compareTo, new ArrayOfBooleansSerDe());
+    checkSketchesEqual(sketch, deserialized);
+  }
+
+  private static <T> void checkSketchesEqual(ItemsSketch<T> expected, ItemsSketch<T> actual) {
+    ItemsSketchSortedView<T> expSV = expected.getSortedView();
+    ItemsSketchSortedView<T> actSV = actual.getSortedView();
+    int N = (int)actSV.getN();
+    long[] expCumWts = expSV.getCumulativeWeights();
+    Boolean[] expItemsArr = (Boolean[])expSV.getQuantiles();
+    long[] actCumWts = actSV.getCumulativeWeights();
+    Boolean[] actItemsArr = (Boolean[])actSV.getQuantiles();
+    printf("%3s %8s %8s\n", "i","Actual", "Expected");
+    for (int i = 0; i < N; i++) {
+      printf("%3d %8s %8s\n", i, actItemsArr[i].toString(), expItemsArr[i].toString());
+    }
+    assertEquals(actCumWts, expCumWts);
+    assertEquals(actItemsArr, expItemsArr);
+  }
+
   @Test
   public void printlnTest() {
     println("PRINTING: "+this.getClass().getName());
   }
 
+  private final static boolean enablePrinting = false;
+
+  /**
+   * @param format the format
+   * @param args the args
+   */
+  private static final void printf(final String format, final Object ...args) {
+    if (enablePrinting) { System.out.printf(format, args); }
+  }
+
   /**
-   * @param s value to print
+   * @param o the Object to println
    */
-  static void println(final String s) {
-    //System.out.println(s); //disable here
+  private static final void println(final Object o) {
+    if (enablePrinting) { System.out.println(o.toString()); }
   }
 
 }