[kylin] 35/44: KYLIN-4327 TOPN Comparator may violate its general contract

[nic]

This is an automated email from the ASF dual-hosted git repository.

nic pushed a commit to branch 3.0.x
in repository https://gitbox.apache.org/repos/asf/kylin.git

commit 7ea101db55d9524260ad8d9e63f2020f9ab32f71
Author: Yifei.Wu <vafuler...@gmail.com>
AuthorDate: Mon Jan 6 11:28:47 2020 +0800

    KYLIN-4327 TOPN Comparator may violate its general contract
---
 .../org/apache/kylin/measure/topn/TopNCounter.java |  8 ++--
 .../kylin/measure/topn/TopNCounterBasicTest.java   | 47 ++++++++++++++++++++++
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git 
a/core-metadata/src/main/java/org/apache/kylin/measure/topn/TopNCounter.java 
b/core-metadata/src/main/java/org/apache/kylin/measure/topn/TopNCounter.java
index 8037eac..754fd55 100644
--- a/core-metadata/src/main/java/org/apache/kylin/measure/topn/TopNCounter.java
+++ b/core-metadata/src/main/java/org/apache/kylin/measure/topn/TopNCounter.java
@@ -238,18 +238,18 @@ public class TopNCounter<T> implements 
Iterable<Counter<T>>, java.io.Serializabl
         }
     }
 
-    private static final Comparator ASC_COMPARATOR = new Comparator<Counter>() 
{
+    static final Comparator ASC_COMPARATOR = new Comparator<Counter>() {
         @Override
         public int compare(Counter o1, Counter o2) {
-            return o1.getCount() > o2.getCount() ? 1 : o1.getCount() == 
o2.getCount() ? 0 : -1;
+            return Double.compare(o1.getCount(), o2.getCount());
         }
 
     };
 
-    private static final Comparator DESC_COMPARATOR = new 
Comparator<Counter>() {
+    static final Comparator DESC_COMPARATOR = new Comparator<Counter>() {
         @Override
         public int compare(Counter o1, Counter o2) {
-            return o1.getCount() > o2.getCount() ? -1 : o1.getCount() == 
o2.getCount() ? 0 : 1;
+            return Double.compare(o2.getCount(), o1.getCount());
         }
 
     };
diff --git 
a/core-metadata/src/test/java/org/apache/kylin/measure/topn/TopNCounterBasicTest.java
 
b/core-metadata/src/test/java/org/apache/kylin/measure/topn/TopNCounterBasicTest.java
index 9d034fe..506ecf3 100644
--- 
a/core-metadata/src/test/java/org/apache/kylin/measure/topn/TopNCounterBasicTest.java
+++ 
b/core-metadata/src/test/java/org/apache/kylin/measure/topn/TopNCounterBasicTest.java
@@ -24,8 +24,11 @@ import static org.junit.Assert.assertTrue;
 import java.util.Arrays;
 import java.util.List;
 
+import org.junit.Assert;
 import org.junit.Test;
 
+import com.google.common.collect.Lists;
+
 public class TopNCounterBasicTest {
 
     @Test
@@ -130,4 +133,48 @@ public class TopNCounterBasicTest {
         }
 
     }
+
+    @Test
+    public void testComparatorSymmetry() {
+        List<Counter> counters = Lists.newArrayList(new Counter<>("item", 0d), 
new Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 3d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 1d), new 
Counter<>("item", 1d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 1d),
+                new Counter<>("item", 0d), new Counter<>("item", 1d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 1d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 1d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 0d), new Counter<>("item", 2d), new 
Counter<>("item", 1d),
+                new Counter<>("item", 0d), new Counter<>("item", 0d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 2d), new Counter<>("item", 4d), new 
Counter<>("item", 0d),
+                new Counter<>("item", 3d));
+        counters.sort(TopNCounter.ASC_COMPARATOR);
+        List<Double> expectedCounts = Lists.newArrayList(0d, 0d, 0d, 0d, 0d, 
0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d,
+                0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 
0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d,
+                0d, 0d, 0d, 0d, 0d, 0d, 1d, 1d, 1d, 1d, 1d, 1d, 1d, 2d, 2d, 
3d, 3d, 4d);
+        List<Double> originCounts = Lists.newArrayList();
+        counters.stream().forEach(counter -> {
+            originCounts.add(counter.getCount());
+        });
+        Assert.assertArrayEquals(expectedCounts.toArray(), 
originCounts.toArray());
+
+        counters.sort(TopNCounter.DESC_COMPARATOR);
+        List<Double> expectedDescCounts = Lists.newArrayList(4d, 3d, 3d, 2d, 
2d, 1d, 1d, 1d, 1d, 1d, 1d, 1d, 0d, 0d, 0d,
+                0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 
0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d,
+                0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 0d, 
0d, 0d, 0d, 0d);
+        List<Double> originDescCounts = Lists.newArrayList();
+        counters.stream().forEach(counter -> {
+            originDescCounts.add(counter.getCount());
+        });
+        Assert.assertArrayEquals(expectedDescCounts.toArray(), 
originDescCounts.toArray());
+    }
 }