From 3c9da0dcc0ca26572227fa1ab1107589c58912f3 Mon Sep 17 00:00:00 2001 From: lrhodes Date: Sat, 30 Mar 2019 14:47:37 -0700 Subject: [PATCH 1/2] Jon's fix plus my unit test --- .../sketches/quantiles/DoublesMergeImpl.java | 9 +- .../sketches/quantiles/DoublesSketch.java | 2 +- .../sketches/quantiles/DebugUnionTest.java | 123 ++++++++++++++++++ 3 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 src/test/java/com/yahoo/sketches/quantiles/DebugUnionTest.java diff --git a/src/main/java/com/yahoo/sketches/quantiles/DoublesMergeImpl.java b/src/main/java/com/yahoo/sketches/quantiles/DoublesMergeImpl.java index 76b7bfaaa..fbd996b93 100644 --- a/src/main/java/com/yahoo/sketches/quantiles/DoublesMergeImpl.java +++ b/src/main/java/com/yahoo/sketches/quantiles/DoublesMergeImpl.java @@ -76,21 +76,19 @@ static void mergeInto(final DoublesSketch src, final UpdateDoublesSketch tgt) { assert srcBitPattern == (srcN / (2L * srcK)); final DoublesSketchAccessor tgtSketchBuf = DoublesSketchAccessor.wrap(tgt, true); + long newTgtBitPattern = tgt.getBitPattern(); for (int srcLvl = 0; srcBitPattern != 0L; srcLvl++, srcBitPattern >>>= 1) { if ((srcBitPattern & 1L) > 0L) { - final long newTgtBitPattern = DoublesUpdateImpl.inPlacePropagateCarry( + newTgtBitPattern = DoublesUpdateImpl.inPlacePropagateCarry( srcLvl, srcSketchBuf.setLevel(srcLvl), scratch2KAcc, false, tgtK, tgtSketchBuf, - tgt.getBitPattern() + newTgtBitPattern ); - - tgt.putBitPattern(newTgtBitPattern); - // won't update tgt.n_ until the very end } } @@ -100,6 +98,7 @@ static void mergeInto(final DoublesSketch src, final UpdateDoublesSketch tgt) { } tgt.putN(nFinal); + tgt.putBitPattern(newTgtBitPattern); // no-op if direct assert (tgt.getN() / (2L * tgtK)) == tgt.getBitPattern(); // internal consistency check diff --git a/src/main/java/com/yahoo/sketches/quantiles/DoublesSketch.java b/src/main/java/com/yahoo/sketches/quantiles/DoublesSketch.java index ec48ef29c..6d5efe832 100644 --- a/src/main/java/com/yahoo/sketches/quantiles/DoublesSketch.java +++ b/src/main/java/com/yahoo/sketches/quantiles/DoublesSketch.java @@ -138,7 +138,7 @@ public abstract class DoublesSketch { * received in exactly the same order. This is only useful when performing test comparisons, * otherwise is not recommended. */ - public static final Random rand = new Random(); + public static Random rand = new Random(); DoublesSketch(final int k) { Util.checkK(k); diff --git a/src/test/java/com/yahoo/sketches/quantiles/DebugUnionTest.java b/src/test/java/com/yahoo/sketches/quantiles/DebugUnionTest.java new file mode 100644 index 000000000..9f27a3564 --- /dev/null +++ b/src/test/java/com/yahoo/sketches/quantiles/DebugUnionTest.java @@ -0,0 +1,123 @@ +/* + * Copyright 2019, Yahoo! Inc. Licensed under the terms of the + * Apache License 2.0. See LICENSE file at the project root for terms. + */ + +package com.yahoo.sketches.quantiles; + +import static com.yahoo.sketches.quantiles.Util.LS; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + +import java.util.HashSet; +import java.util.Random; + +import org.testng.annotations.Test; + +import com.yahoo.memory.WritableDirectHandle; +import com.yahoo.memory.WritableMemory; + +/** + * @author Lee Rhodes + */ +public class DebugUnionTest { + + @Test + public void test() { + final int n = 70_000; + final int valueLimit = 1000; + final int numSketches = 3; + final int sketchK = 8; + final int unionK = 8; + UpdateDoublesSketch[] sketchArr = new UpdateDoublesSketch[numSketches]; + + //builds the input sketches, all on heap + DoublesSketch.rand = new Random(1); //make deterministic for test + final HashSet set = new HashSet<>(); //holds input values + for (int s = 0; s < numSketches; s++) { + sketchArr[s] = buildHeapSketch(sketchK, n, valueLimit, set); + } + + //loads the on heap union + DoublesSketch.rand = new Random(1); //make deterministic for test + DoublesUnion hUnion = DoublesUnion.builder().setMaxK(unionK).build(); + for (int s = 0; s < numSketches; s++) { hUnion.update(sketchArr[s]); } + DoublesSketch hSketch = hUnion.getResult(); + + //loads the direct union + DoublesSketch.rand = new Random(1); //make deterministic for test + DoublesUnion dUnion; + DoublesSketch dSketch; + try ( WritableDirectHandle wdh = WritableMemory.allocateDirect(10_000_000) ) { + WritableMemory wmem = wdh.get(); + dUnion = DoublesUnion.builder().setMaxK(8).build(wmem); + for (int s = 0; s < numSketches; s++) { dUnion.update(sketchArr[s]); } + dSketch = dUnion.getResult(); //result is on heap + } + + //iterates and counts errors + int hCount = hSketch.getRetainedItems(); + int dCount = dSketch.getRetainedItems(); + + assertEquals(hCount, dCount); //Retained items must be the same + + double[] heapItems = new double[hCount]; + double[] directItems = new double[dCount]; + int hErrors = 0; + int dErrors = 0; + + DoublesSketchIterator hit = hSketch.iterator(); + DoublesSketchIterator dit = dSketch.iterator(); + int i = 0; + while (hit.next() && dit.next()) { + double v = hit.getValue(); + heapItems[i] = v; + if (!set.contains(v)) { hErrors++; } + + double w = dit.getValue(); + directItems[i] = w; + if (!set.contains(w)) { dErrors++; } + i++; + assertEquals(v, w, 0); //Items must be returned in same order and be equal + } + assertTrue(hErrors == 0); + assertTrue(dErrors == 0); + + //println("HeapUnion : Values: " + hCount + ", errors: " + hErrors); + //println(hSketch.toString(true, true)); + + //println("DirectUnion: Values: " + dCount + ", errors: " + dErrors); + //println(dSketch.toString(true, true)); + } + + private static UpdateDoublesSketch buildHeapSketch(final int k, final int n, final int valueLimit, + final HashSet set) { + final UpdateDoublesSketch uSk = DoublesSketch.builder().setK(k).build(); + for (int i = 0; i < n; i++) { + final double value = DoublesSketch.rand.nextInt(valueLimit) + 1; + uSk.update(value); + set.add(value); + } + return uSk; + } + + @Test + public void printlnTest() { + println("PRINTING: " + this.getClass().getName()); + } + + /** + * @param s value to print + */ + static void println(String s) { + print(s+LS); + } + + /** + * @param s value to print + */ + static void print(String s) { + //System.out.print(s); //disable here + } + +} From 457360c3bbf24a931decbc5f21159c502ca6b01d Mon Sep 17 00:00:00 2001 From: Jon Malkin Date: Sat, 30 Mar 2019 14:47:37 -0700 Subject: [PATCH 2/2] Jon's fix plus my unit test --- .../sketches/quantiles/DoublesMergeImpl.java | 9 +- .../sketches/quantiles/DoublesSketch.java | 2 +- .../sketches/quantiles/DebugUnionTest.java | 123 ++++++++++++++++++ 3 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 src/test/java/com/yahoo/sketches/quantiles/DebugUnionTest.java diff --git a/src/main/java/com/yahoo/sketches/quantiles/DoublesMergeImpl.java b/src/main/java/com/yahoo/sketches/quantiles/DoublesMergeImpl.java index 76b7bfaaa..fbd996b93 100644 --- a/src/main/java/com/yahoo/sketches/quantiles/DoublesMergeImpl.java +++ b/src/main/java/com/yahoo/sketches/quantiles/DoublesMergeImpl.java @@ -76,21 +76,19 @@ static void mergeInto(final DoublesSketch src, final UpdateDoublesSketch tgt) { assert srcBitPattern == (srcN / (2L * srcK)); final DoublesSketchAccessor tgtSketchBuf = DoublesSketchAccessor.wrap(tgt, true); + long newTgtBitPattern = tgt.getBitPattern(); for (int srcLvl = 0; srcBitPattern != 0L; srcLvl++, srcBitPattern >>>= 1) { if ((srcBitPattern & 1L) > 0L) { - final long newTgtBitPattern = DoublesUpdateImpl.inPlacePropagateCarry( + newTgtBitPattern = DoublesUpdateImpl.inPlacePropagateCarry( srcLvl, srcSketchBuf.setLevel(srcLvl), scratch2KAcc, false, tgtK, tgtSketchBuf, - tgt.getBitPattern() + newTgtBitPattern ); - - tgt.putBitPattern(newTgtBitPattern); - // won't update tgt.n_ until the very end } } @@ -100,6 +98,7 @@ static void mergeInto(final DoublesSketch src, final UpdateDoublesSketch tgt) { } tgt.putN(nFinal); + tgt.putBitPattern(newTgtBitPattern); // no-op if direct assert (tgt.getN() / (2L * tgtK)) == tgt.getBitPattern(); // internal consistency check diff --git a/src/main/java/com/yahoo/sketches/quantiles/DoublesSketch.java b/src/main/java/com/yahoo/sketches/quantiles/DoublesSketch.java index ec48ef29c..6d5efe832 100644 --- a/src/main/java/com/yahoo/sketches/quantiles/DoublesSketch.java +++ b/src/main/java/com/yahoo/sketches/quantiles/DoublesSketch.java @@ -138,7 +138,7 @@ public abstract class DoublesSketch { * received in exactly the same order. This is only useful when performing test comparisons, * otherwise is not recommended. */ - public static final Random rand = new Random(); + public static Random rand = new Random(); DoublesSketch(final int k) { Util.checkK(k); diff --git a/src/test/java/com/yahoo/sketches/quantiles/DebugUnionTest.java b/src/test/java/com/yahoo/sketches/quantiles/DebugUnionTest.java new file mode 100644 index 000000000..9f27a3564 --- /dev/null +++ b/src/test/java/com/yahoo/sketches/quantiles/DebugUnionTest.java @@ -0,0 +1,123 @@ +/* + * Copyright 2019, Yahoo! Inc. Licensed under the terms of the + * Apache License 2.0. See LICENSE file at the project root for terms. + */ + +package com.yahoo.sketches.quantiles; + +import static com.yahoo.sketches.quantiles.Util.LS; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + +import java.util.HashSet; +import java.util.Random; + +import org.testng.annotations.Test; + +import com.yahoo.memory.WritableDirectHandle; +import com.yahoo.memory.WritableMemory; + +/** + * @author Lee Rhodes + */ +public class DebugUnionTest { + + @Test + public void test() { + final int n = 70_000; + final int valueLimit = 1000; + final int numSketches = 3; + final int sketchK = 8; + final int unionK = 8; + UpdateDoublesSketch[] sketchArr = new UpdateDoublesSketch[numSketches]; + + //builds the input sketches, all on heap + DoublesSketch.rand = new Random(1); //make deterministic for test + final HashSet set = new HashSet<>(); //holds input values + for (int s = 0; s < numSketches; s++) { + sketchArr[s] = buildHeapSketch(sketchK, n, valueLimit, set); + } + + //loads the on heap union + DoublesSketch.rand = new Random(1); //make deterministic for test + DoublesUnion hUnion = DoublesUnion.builder().setMaxK(unionK).build(); + for (int s = 0; s < numSketches; s++) { hUnion.update(sketchArr[s]); } + DoublesSketch hSketch = hUnion.getResult(); + + //loads the direct union + DoublesSketch.rand = new Random(1); //make deterministic for test + DoublesUnion dUnion; + DoublesSketch dSketch; + try ( WritableDirectHandle wdh = WritableMemory.allocateDirect(10_000_000) ) { + WritableMemory wmem = wdh.get(); + dUnion = DoublesUnion.builder().setMaxK(8).build(wmem); + for (int s = 0; s < numSketches; s++) { dUnion.update(sketchArr[s]); } + dSketch = dUnion.getResult(); //result is on heap + } + + //iterates and counts errors + int hCount = hSketch.getRetainedItems(); + int dCount = dSketch.getRetainedItems(); + + assertEquals(hCount, dCount); //Retained items must be the same + + double[] heapItems = new double[hCount]; + double[] directItems = new double[dCount]; + int hErrors = 0; + int dErrors = 0; + + DoublesSketchIterator hit = hSketch.iterator(); + DoublesSketchIterator dit = dSketch.iterator(); + int i = 0; + while (hit.next() && dit.next()) { + double v = hit.getValue(); + heapItems[i] = v; + if (!set.contains(v)) { hErrors++; } + + double w = dit.getValue(); + directItems[i] = w; + if (!set.contains(w)) { dErrors++; } + i++; + assertEquals(v, w, 0); //Items must be returned in same order and be equal + } + assertTrue(hErrors == 0); + assertTrue(dErrors == 0); + + //println("HeapUnion : Values: " + hCount + ", errors: " + hErrors); + //println(hSketch.toString(true, true)); + + //println("DirectUnion: Values: " + dCount + ", errors: " + dErrors); + //println(dSketch.toString(true, true)); + } + + private static UpdateDoublesSketch buildHeapSketch(final int k, final int n, final int valueLimit, + final HashSet set) { + final UpdateDoublesSketch uSk = DoublesSketch.builder().setK(k).build(); + for (int i = 0; i < n; i++) { + final double value = DoublesSketch.rand.nextInt(valueLimit) + 1; + uSk.update(value); + set.add(value); + } + return uSk; + } + + @Test + public void printlnTest() { + println("PRINTING: " + this.getClass().getName()); + } + + /** + * @param s value to print + */ + static void println(String s) { + print(s+LS); + } + + /** + * @param s value to print + */ + static void print(String s) { + //System.out.print(s); //disable here + } + +}