From cc1aff5892f1dbd4aa75727237eb68c9217a9e8a Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Tue, 23 Jun 2020 11:36:15 -1000 Subject: [PATCH 1/3] fix return type from HyperUniquesAggregator/HyperUniquesVectorAggregator --- .../main/java/org/apache/druid/query/QueryContexts.java | 2 +- .../hyperloglog/HyperUniquesAggregatorFactory.java | 7 ++++++- .../druid/query/timeseries/TimeseriesQueryRunnerTest.java | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/QueryContexts.java b/processing/src/main/java/org/apache/druid/query/QueryContexts.java index b5a7be0b7dd8..36675935fc82 100644 --- a/processing/src/main/java/org/apache/druid/query/QueryContexts.java +++ b/processing/src/main/java/org/apache/druid/query/QueryContexts.java @@ -59,7 +59,7 @@ public class QueryContexts public static final boolean DEFAULT_USE_CACHE = true; public static final boolean DEFAULT_POPULATE_RESULTLEVEL_CACHE = true; public static final boolean DEFAULT_USE_RESULTLEVEL_CACHE = true; - public static final Vectorize DEFAULT_VECTORIZE = Vectorize.FALSE; + public static final Vectorize DEFAULT_VECTORIZE = Vectorize.TRUE; public static final int DEFAULT_PRIORITY = 0; public static final int DEFAULT_UNCOVERED_INTERVALS_LIMIT = 0; public static final long DEFAULT_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(5); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java index ceb82d5247e5..794825009ae5 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java @@ -59,7 +59,12 @@ public class HyperUniquesAggregatorFactory extends AggregatorFactory public static Object estimateCardinality(@Nullable Object object, boolean round) { if (object == null) { - return 0; + if (round) { + return 0L; + } else { + return 0d; + + } } final HyperLogLogCollector collector = (HyperLogLogCollector) object; diff --git a/processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java index 5d5af015b5c8..99989c01d8b4 100644 --- a/processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java @@ -2568,7 +2568,7 @@ public void testTimeseriesWithTimestampResultFieldContextForArrayResponse() ); Assert.assertEquals( 0.0D, - NullHandling.sqlCompatible() ? (Double) result[4] : (Integer) result[4], + (Double) result[4], 0.02 ); Assert.assertEquals( @@ -2581,7 +2581,7 @@ public void testTimeseriesWithTimestampResultFieldContextForArrayResponse() result[3] ); Assert.assertEquals( - (Integer) result[4], + (Double) result[4], 0.0, 0.02 ); From f3f16f29e16ec08a18cdc4905ca2b80ccd14d1a8 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Tue, 23 Jun 2020 13:20:00 -1000 Subject: [PATCH 2/3] address comments --- .../org/apache/druid/query/QueryContexts.java | 2 +- .../HyperUniquesAggregatorFactory.java | 15 +++-------- .../HyperUniquesAggregatorFactoryTest.java | 25 +++++++++++++++++++ .../druid/query/topn/TopNQueryRunnerTest.java | 6 ++--- 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/QueryContexts.java b/processing/src/main/java/org/apache/druid/query/QueryContexts.java index 36675935fc82..b5a7be0b7dd8 100644 --- a/processing/src/main/java/org/apache/druid/query/QueryContexts.java +++ b/processing/src/main/java/org/apache/druid/query/QueryContexts.java @@ -59,7 +59,7 @@ public class QueryContexts public static final boolean DEFAULT_USE_CACHE = true; public static final boolean DEFAULT_POPULATE_RESULTLEVEL_CACHE = true; public static final boolean DEFAULT_USE_RESULTLEVEL_CACHE = true; - public static final Vectorize DEFAULT_VECTORIZE = Vectorize.TRUE; + public static final Vectorize DEFAULT_VECTORIZE = Vectorize.FALSE; public static final int DEFAULT_PRIORITY = 0; public static final int DEFAULT_UNCOVERED_INTERVALS_LIMIT = 0; public static final long DEFAULT_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(5); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java index 794825009ae5..e0b7686ed1f5 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java @@ -58,22 +58,13 @@ public class HyperUniquesAggregatorFactory extends AggregatorFactory { public static Object estimateCardinality(@Nullable Object object, boolean round) { - if (object == null) { - if (round) { - return 0L; - } else { - return 0d; - - } - } - final HyperLogLogCollector collector = (HyperLogLogCollector) object; - // Avoid ternary, it causes estimateCardinalityRound to be cast to double. + // Avoid ternary for round check as it causes estimateCardinalityRound to be cast to double. if (round) { - return collector.estimateCardinalityRound(); + return collector == null ? 0L : collector.estimateCardinalityRound(); } else { - return collector.estimateCardinality(); + return collector == null ? 0d : collector.estimateCardinality(); } } diff --git a/processing/src/test/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactoryTest.java b/processing/src/test/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactoryTest.java index f9151923413d..421a457999d9 100644 --- a/processing/src/test/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactoryTest.java +++ b/processing/src/test/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactoryTest.java @@ -30,6 +30,7 @@ import org.junit.Assert; import org.junit.Test; +import java.nio.ByteBuffer; import java.util.Comparator; import java.util.Random; @@ -173,6 +174,30 @@ public void testCompareToShouldBehaveConsistentlyWithEstimatedCardinalitiesEvenI } } + @Test + public void testEstimateCardinalityForZeroCardinality() + { + HyperLogLogCollector emptyHyperLogLogCollector = HyperUniquesBufferAggregator.doGet( + ByteBuffer.allocate(HyperLogLogCollector.getLatestNumBytesForDenseStorage()), + 0 + ); + + Assert.assertEquals(0L, HyperUniquesAggregatorFactory.estimateCardinality(null, true)); + Assert.assertEquals(0d, HyperUniquesAggregatorFactory.estimateCardinality(null, false)); + + Assert.assertEquals(0L, HyperUniquesAggregatorFactory.estimateCardinality(emptyHyperLogLogCollector, true)); + Assert.assertEquals(0d, HyperUniquesAggregatorFactory.estimateCardinality(emptyHyperLogLogCollector, false)); + + Assert.assertEquals( + HyperUniquesAggregatorFactory.estimateCardinality(emptyHyperLogLogCollector, true).getClass(), + HyperUniquesAggregatorFactory.estimateCardinality(null, true).getClass() + ); + Assert.assertEquals( + HyperUniquesAggregatorFactory.estimateCardinality(emptyHyperLogLogCollector, false).getClass(), + HyperUniquesAggregatorFactory.estimateCardinality(null, false).getClass() + ); + } + @Test public void testSerde() throws Exception { diff --git a/processing/src/test/java/org/apache/druid/query/topn/TopNQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/topn/TopNQueryRunnerTest.java index 3f5b41db0a15..f98160a1a390 100644 --- a/processing/src/test/java/org/apache/druid/query/topn/TopNQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/topn/TopNQueryRunnerTest.java @@ -647,15 +647,15 @@ public void testTopNOverMissingUniques() Arrays.>asList( ImmutableMap.builder() .put("market", "spot") - .put("uniques", 0) + .put("uniques", 0d) .build(), ImmutableMap.builder() .put("market", "total_market") - .put("uniques", 0) + .put("uniques", 0d) .build(), ImmutableMap.builder() .put("market", "upfront") - .put("uniques", 0) + .put("uniques", 0d) .build() ) ) From e5792453f24ff427e006ee76d19e38aecebd3e19 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Tue, 23 Jun 2020 13:21:36 -1000 Subject: [PATCH 3/3] address comments --- .../test/java/org/apache/druid/query/SchemaEvolutionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/processing/src/test/java/org/apache/druid/query/SchemaEvolutionTest.java b/processing/src/test/java/org/apache/druid/query/SchemaEvolutionTest.java index 7e829e00a89a..ce17b38ba46b 100644 --- a/processing/src/test/java/org/apache/druid/query/SchemaEvolutionTest.java +++ b/processing/src/test/java/org/apache/druid/query/SchemaEvolutionTest.java @@ -241,7 +241,7 @@ public void testHyperUniqueEvolutionTimeseries(boolean doVectorize) // index1 has no "uniques" column Assert.assertEquals( - timeseriesResult(ImmutableMap.of("uniques", 0)), + timeseriesResult(ImmutableMap.of("uniques", 0d)), runQuery(query, factory, ImmutableList.of(index1)) );