From de5b1749bc17c8d36ae382f12553ed7c3c6e1014 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Xavier=20L=C3=A9aut=C3=A9?= Date: Fri, 25 Apr 2014 14:50:22 -0700 Subject: [PATCH] minor changes to address code review --- .../cardinality/CardinalityAggregator.java | 12 +++++------- .../cardinality/CardinalityBufferAggregator.java | 3 +-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityAggregator.java b/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityAggregator.java index cde4e643110..e1fbc806fe2 100644 --- a/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityAggregator.java @@ -19,7 +19,6 @@ package io.druid.query.aggregation.cardinality; -import com.google.common.collect.Lists; import com.google.common.hash.HashFunction; import com.google.common.hash.Hasher; import com.google.common.hash.Hashing; @@ -52,24 +51,23 @@ public class CardinalityAggregator implements Aggregator final DimensionSelector selector = selectorList.get(k); final IndexedInts row = selector.getRow(); final int size = row.size(); + // nothing to add to hasher if size == 0, only handle size == 1 and size != 0 cases. if (size == 1) { final String value = selector.lookupName(row.get(0)); hasher.putString(value != null ? value : NULL_STRING); - } else if (size == 0) { - // nothing to add to hasher - } else { + } else if (size != 0) { final String[] values = new String[size]; for (int i = 0; i < size; ++i) { final String value = selector.lookupName(row.get(i)); values[i] = value != null ? value : NULL_STRING; } + // Values need to be sorted to ensure consistent multi-value ordering across different segments Arrays.sort(values); for (int i = 0; i < size; ++i) { if(i != 0) { hasher.putChar(SEPARATOR); } - final String value = values[i]; - hasher.putString(value); + hasher.putString(values[i]); } } } @@ -95,7 +93,7 @@ public class CardinalityAggregator implements Aggregator ) { this.name = name; - this.selectorList = Lists.newArrayList(selectorList); + this.selectorList = selectorList; this.collector = HyperLogLogCollector.makeLatestCollector(); this.byRow = byRow; } diff --git a/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityBufferAggregator.java b/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityBufferAggregator.java index eb4b4e25101..84143b7870d 100644 --- a/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityBufferAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityBufferAggregator.java @@ -19,7 +19,6 @@ package io.druid.query.aggregation.cardinality; -import com.google.common.collect.Lists; import io.druid.query.aggregation.BufferAggregator; import io.druid.query.aggregation.hyperloglog.HyperLogLogCollector; import io.druid.segment.DimensionSelector; @@ -39,7 +38,7 @@ public class CardinalityBufferAggregator implements BufferAggregator boolean byRow ) { - this.selectorList = Lists.newArrayList(selectorList); + this.selectorList = selectorList; this.byRow = byRow; }