KYLIN-1832 code review
Project: http://git-wip-us.apache.org/repos/asf/kylin/repo Commit: http://git-wip-us.apache.org/repos/asf/kylin/commit/78a59179 Tree: http://git-wip-us.apache.org/repos/asf/kylin/tree/78a59179 Diff: http://git-wip-us.apache.org/repos/asf/kylin/diff/78a59179 Branch: refs/heads/master Commit: 78a59179825012daa94a117babbf0c3aea15ec89 Parents: a39d078 Author: Li Yang <liy...@apache.org> Authored: Thu Dec 22 14:31:07 2016 +0800 Committer: Li Yang <liy...@apache.org> Committed: Thu Dec 22 14:31:07 2016 +0800 ---------------------------------------------------------------------- .../kylin/measure/hllc/DenseRegister.java | 8 ++-- .../apache/kylin/measure/hllc/HLLCounter.java | 22 ++++++----- .../kylin/measure/hllc/SingleValueRegister.java | 7 +--- .../kylin/measure/hllc/SparseRegister.java | 8 ++-- .../kylin/measure/hllc/HLLCounterTest.java | 15 ++++---- .../hllc/NewHyperLogLogBenchmarkTest.java | 40 +++++++++----------- 6 files changed, 48 insertions(+), 52 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kylin/blob/78a59179/core-metadata/src/main/java/org/apache/kylin/measure/hllc/DenseRegister.java ---------------------------------------------------------------------- diff --git a/core-metadata/src/main/java/org/apache/kylin/measure/hllc/DenseRegister.java b/core-metadata/src/main/java/org/apache/kylin/measure/hllc/DenseRegister.java index 0dea535..5b929b2 100644 --- a/core-metadata/src/main/java/org/apache/kylin/measure/hllc/DenseRegister.java +++ b/core-metadata/src/main/java/org/apache/kylin/measure/hllc/DenseRegister.java @@ -52,16 +52,16 @@ public class DenseRegister implements Register { if (dr.register[i] > register[i]) register[i] = dr.register[i]; } - } else if(another.getRegisterType() == RegisterType.SPARSE){ + } else if (another.getRegisterType() == RegisterType.SPARSE) { SparseRegister sr = (SparseRegister) another; Collection<Map.Entry<Integer, Byte>> allValue = sr.getAllValue(); for (Map.Entry<Integer, Byte> entry : allValue) { if (entry.getValue() > register[entry.getKey()]) register[entry.getKey()] = entry.getValue(); } - }else{ - SingleValueRegister sr = (SingleValueRegister)another; - if(sr.getSize() > 0 && sr.getValue() > register[sr.getSingleValuePos()]){ + } else { + SingleValueRegister sr = (SingleValueRegister) another; + if (sr.getSize() > 0 && sr.getValue() > register[sr.getSingleValuePos()]) { register[sr.getSingleValuePos()] = sr.getValue(); } } http://git-wip-us.apache.org/repos/asf/kylin/blob/78a59179/core-metadata/src/main/java/org/apache/kylin/measure/hllc/HLLCounter.java ---------------------------------------------------------------------- diff --git a/core-metadata/src/main/java/org/apache/kylin/measure/hllc/HLLCounter.java b/core-metadata/src/main/java/org/apache/kylin/measure/hllc/HLLCounter.java index 6325651..21f3a0e 100644 --- a/core-metadata/src/main/java/org/apache/kylin/measure/hllc/HLLCounter.java +++ b/core-metadata/src/main/java/org/apache/kylin/measure/hllc/HLLCounter.java @@ -119,8 +119,8 @@ public class HLLCounter implements Serializable, Comparable<HLLCounter> { } private void setIfBigger(Register register, int pos, byte value) { - Byte b = register.get(pos); - if (b == null || value > b) { + byte b = register.get(pos); + if (value > b) { register.set(pos, value); } } @@ -145,6 +145,7 @@ public class HLLCounter implements Serializable, Comparable<HLLCounter> { } else if (register.getSize() == 0 && another.register.getSize() > 0) { SingleValueRegister sr = (SingleValueRegister) another.register; register.set(sr.getSingleValuePos(), sr.getValue()); + return; } break; case SPARSE: @@ -267,7 +268,8 @@ public class HLLCounter implements Serializable, Comparable<HLLCounter> { // decide output scheme -- map (3*size bytes) or array (2^p bytes) byte scheme; - if (register instanceof SingleValueRegister || register instanceof SparseRegister || 5 + (indexLen + 1) * size < m) { + if (register instanceof SingleValueRegister || register instanceof SparseRegister // + || 5 + (indexLen + 1) * size < m) { scheme = 0; // map } else { scheme = 1; // array @@ -275,18 +277,18 @@ public class HLLCounter implements Serializable, Comparable<HLLCounter> { out.put(scheme); if (scheme == 0) { // map scheme BytesUtil.writeVInt(size, out); - if (register.getRegisterType() == RegisterType.SPARSE) { //sparse register - Collection<Map.Entry<Integer, Byte>> allValue = ((SparseRegister) register).getAllValue(); - for (Map.Entry<Integer, Byte> entry : allValue) { - writeUnsigned(entry.getKey(), indexLen, out); - out.put(entry.getValue()); - } - } else if (register.getRegisterType() == RegisterType.SINGLE_VALUE) { + if (register.getRegisterType() == RegisterType.SINGLE_VALUE) { //single value register if (size > 0) { SingleValueRegister sr = (SingleValueRegister) register; writeUnsigned(sr.getSingleValuePos(), indexLen, out); out.put(sr.getValue()); } + } else if (register.getRegisterType() == RegisterType.SPARSE) { //sparse register + Collection<Map.Entry<Integer, Byte>> allValue = ((SparseRegister) register).getAllValue(); + for (Map.Entry<Integer, Byte> entry : allValue) { + writeUnsigned(entry.getKey(), indexLen, out); + out.put(entry.getValue()); + } } else { //dense register byte[] registers = ((DenseRegister) register).getRawRegister(); for (int i = 0; i < m; i++) { http://git-wip-us.apache.org/repos/asf/kylin/blob/78a59179/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SingleValueRegister.java ---------------------------------------------------------------------- diff --git a/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SingleValueRegister.java b/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SingleValueRegister.java index 5b2f7c8..7f612e2 100644 --- a/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SingleValueRegister.java +++ b/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SingleValueRegister.java @@ -32,6 +32,7 @@ public class SingleValueRegister implements Register { @Override public void set(int pos, byte value) { + assert this.singleValuePos < 0 || this.singleValuePos == pos; this.singleValuePos = pos; this.value = value; } @@ -43,13 +44,9 @@ public class SingleValueRegister implements Register { return value; } - /* - this method should not be used in single value register - */ - @Deprecated @Override public void merge(Register another) { - return; + throw new IllegalStateException(); } @Override http://git-wip-us.apache.org/repos/asf/kylin/blob/78a59179/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SparseRegister.java ---------------------------------------------------------------------- diff --git a/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SparseRegister.java b/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SparseRegister.java index bb550e1..dd7d7c8 100644 --- a/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SparseRegister.java +++ b/core-metadata/src/main/java/org/apache/kylin/measure/hllc/SparseRegister.java @@ -54,16 +54,16 @@ public class SparseRegister implements Register { @Override public void merge(Register another) { assert another.getRegisterType() != RegisterType.DENSE; - if(another.getRegisterType() == RegisterType.SPARSE) { + if (another.getRegisterType() == RegisterType.SPARSE) { SparseRegister sr = (SparseRegister) another; for (Map.Entry<Integer, Byte> entry : sr.sparseRegister.entrySet()) { byte v = get(entry.getKey()); if (entry.getValue() > v) sparseRegister.put(entry.getKey(), entry.getValue()); } - }else if(another.getRegisterType() == RegisterType.SINGLE_VALUE){ - SingleValueRegister sr = (SingleValueRegister)another; - if(sr.getSize() > 0){ + } else if (another.getRegisterType() == RegisterType.SINGLE_VALUE) { + SingleValueRegister sr = (SingleValueRegister) another; + if (sr.getSize() > 0) { byte v = get(sr.getSingleValuePos()); if (sr.getValue() > v) sparseRegister.put(sr.getSingleValuePos(), sr.getValue()); http://git-wip-us.apache.org/repos/asf/kylin/blob/78a59179/core-metadata/src/test/java/org/apache/kylin/measure/hllc/HLLCounterTest.java ---------------------------------------------------------------------- diff --git a/core-metadata/src/test/java/org/apache/kylin/measure/hllc/HLLCounterTest.java b/core-metadata/src/test/java/org/apache/kylin/measure/hllc/HLLCounterTest.java index 1b603a7..4a95bd4 100644 --- a/core-metadata/src/test/java/org/apache/kylin/measure/hllc/HLLCounterTest.java +++ b/core-metadata/src/test/java/org/apache/kylin/measure/hllc/HLLCounterTest.java @@ -53,8 +53,8 @@ public class HLLCounterTest { for (int i = 0; i < 1000000; i++) { one.clear(); one.add(i); - //System.out.println(hllc.getCountEstimate()); hllc.merge(one); + assertTrue(one.getRegisterType() == RegisterType.SINGLE_VALUE); } System.out.println(hllc.getCountEstimate()); System.out.println(hllc.getRegister().getRegisterType()); @@ -112,9 +112,10 @@ public class HLLCounterTest { HLLCounterOld oldCounter = new HLLCounterOld(p); HLLCounter newCounter = new HLLCounter(p); HLLCounter newCounter2 = new HLLCounter(p); - newCounter.add(1); - oldCounter.add(1); - assertEquals(RegisterType.SINGLE_VALUE,newCounter.getRegisterType()); + int rr = rand1.nextInt(); + newCounter.add(rr); + oldCounter.add(rr); + assertEquals(RegisterType.SINGLE_VALUE, newCounter.getRegisterType()); assertEquals(oldCounter.getCountEstimate(), newCounter.getCountEstimate()); buf.clear(); oldCounter.writeRegisters(buf); @@ -133,7 +134,6 @@ public class HLLCounterTest { } assertEquals(RegisterType.SPARSE, newCounter.getRegisterType()); assertEquals(oldCounter.getCountEstimate(), newCounter.getCountEstimate()); - buf.clear(); oldCounter.writeRegisters(buf); buf.flip(); @@ -141,6 +141,8 @@ public class HLLCounterTest { assertEquals(oldCounter.getCountEstimate(), newCounter2.getCountEstimate()); //compare dense + oldCounter.clear(); + newCounter.clear(); for (int i = 0; i < m / 2; i++) { int r = rand1.nextInt(); oldCounter.add(r); @@ -148,7 +150,6 @@ public class HLLCounterTest { } assertEquals(RegisterType.DENSE, newCounter.getRegisterType()); assertEquals(oldCounter.getCountEstimate(), newCounter.getCountEstimate()); - buf.clear(); oldCounter.writeRegisters(buf); buf.flip(); @@ -203,7 +204,7 @@ public class HLLCounterTest { int m = 1 << p; double over = HLLCounter.OVERFLOW_FACTOR * m; int overFlow = (int) over + 1000; - for (int i = 0; i < overFlow; i++){ + for (int i = 0; i < overFlow; i++) { int k = rand1.nextInt(); ha.add(k); hb.add(k); http://git-wip-us.apache.org/repos/asf/kylin/blob/78a59179/core-metadata/src/test/java/org/apache/kylin/measure/hllc/NewHyperLogLogBenchmarkTest.java ---------------------------------------------------------------------- diff --git a/core-metadata/src/test/java/org/apache/kylin/measure/hllc/NewHyperLogLogBenchmarkTest.java b/core-metadata/src/test/java/org/apache/kylin/measure/hllc/NewHyperLogLogBenchmarkTest.java index 26f45d6..ee82f9b 100644 --- a/core-metadata/src/test/java/org/apache/kylin/measure/hllc/NewHyperLogLogBenchmarkTest.java +++ b/core-metadata/src/test/java/org/apache/kylin/measure/hllc/NewHyperLogLogBenchmarkTest.java @@ -17,17 +17,13 @@ */ package org.apache.kylin.measure.hllc; -import org.apache.kylin.measure.hllc.HLLCounterOld; -import org.apache.kylin.measure.hllc.HLLCounter; -import org.apache.kylin.measure.hllc.RegisterType; -import org.junit.Ignore; -import org.junit.Test; +import static org.junit.Assert.assertEquals; import java.nio.ByteBuffer; import java.util.Random; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import org.junit.Ignore; +import org.junit.Test; /** * Created by xiefan on 16-12-12. @@ -48,7 +44,7 @@ public class NewHyperLogLogBenchmarkTest { System.out.println("denseToDenseRegisterMergeBenchmark(), m : " + m); double oldFactor = HLLCounter.OVERFLOW_FACTOR; HLLCounter.OVERFLOW_FACTOR = 1.1; //keep sparse - for (int cardinality : new int[]{m/10,m/5,m/2,m}) { + for (int cardinality : new int[] { m / 10, m / 5, m / 2, m }) { final HLLCounterOld oldCounter = new HLLCounterOld(p); final HLLCounterOld oldCounter2 = getRandOldCounter(p, cardinality); long oldTime = runTestCase(new TestCase() { @@ -101,7 +97,7 @@ public class NewHyperLogLogBenchmarkTest { } } }); - final HLLCounter newCounter = new HLLCounter(p,RegisterType.SPARSE); + final HLLCounter newCounter = new HLLCounter(p, RegisterType.SPARSE); final HLLCounter newCounter2 = getRandNewCounter(p, cardinality); long newTime = runTestCase(new TestCase() { @Override @@ -112,10 +108,10 @@ public class NewHyperLogLogBenchmarkTest { } }); assertEquals(RegisterType.SPARSE, newCounter.getRegisterType()); - if(cardinality == 1){ - assertEquals(RegisterType.SINGLE_VALUE,newCounter2.getRegisterType()); - }else{ - assertEquals(RegisterType.SPARSE,newCounter2.getRegisterType()); + if (cardinality == 1) { + assertEquals(RegisterType.SINGLE_VALUE, newCounter2.getRegisterType()); + } else { + assertEquals(RegisterType.SPARSE, newCounter2.getRegisterType()); } System.out.println("----------------------------"); System.out.println("cardinality : " + cardinality); @@ -156,10 +152,10 @@ public class NewHyperLogLogBenchmarkTest { } }); assertEquals(RegisterType.DENSE, newCounter.getRegisterType()); - if(cardinality == 1){ - assertEquals(RegisterType.SINGLE_VALUE,newCounter2.getRegisterType()); - }else{ - assertEquals(RegisterType.SPARSE,newCounter2.getRegisterType()); + if (cardinality == 1) { + assertEquals(RegisterType.SINGLE_VALUE, newCounter2.getRegisterType()); + } else { + assertEquals(RegisterType.SPARSE, newCounter2.getRegisterType()); } System.out.println("old time : " + oldTime); System.out.println("new time : " + newTime); @@ -209,10 +205,10 @@ public class NewHyperLogLogBenchmarkTest { System.out.println("new serialize bytes : " + totalBytes / testTimes + "B"); } }); - if(cardinality == 1){ - assertEquals(RegisterType.SINGLE_VALUE,newCounter.getRegisterType()); - }else{ - assertEquals(RegisterType.SPARSE,newCounter.getRegisterType()); + if (cardinality == 1) { + assertEquals(RegisterType.SINGLE_VALUE, newCounter.getRegisterType()); + } else { + assertEquals(RegisterType.SPARSE, newCounter.getRegisterType()); } System.out.println("old serialize time : " + oldTime); System.out.println("new serialize time : " + newTime); @@ -301,6 +297,6 @@ public class NewHyperLogLogBenchmarkTest { } public static int[] getTestDataDivide(int m) { - return new int[] { 1, 5, 10, 100, m / 200, m / 100, m / 50, m / 20, m / 10}; + return new int[] { 1, 5, 10, 100, m / 200, m / 100, m / 50, m / 20, m / 10 }; } }
