From b531ab15e1bc32702b158f14fe6aa03b8fa79386 Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Tue, 26 Apr 2016 01:45:46 +0900 Subject: [PATCH 01/13] initial commits for finalizeFieldAccess #2433 --- .../io/druid/jackson/AggregatorsModule.java | 1 + .../src/main/java/io/druid/query/Queries.java | 26 +++- .../aggregation/HasDependentAggFactories.java | 28 ++++ .../post/FinalFieldAccessPostAggregator.java | 128 ++++++++++++++++++ .../FinalFieldAccessPostAggregatorTest.java | 54 ++++++++ 5 files changed, 233 insertions(+), 4 deletions(-) create mode 100644 processing/src/main/java/io/druid/query/aggregation/HasDependentAggFactories.java create mode 100644 processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java create mode 100644 processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java diff --git a/processing/src/main/java/io/druid/jackson/AggregatorsModule.java b/processing/src/main/java/io/druid/jackson/AggregatorsModule.java index 57fc798a6f40..3c7a09664b4e 100644 --- a/processing/src/main/java/io/druid/jackson/AggregatorsModule.java +++ b/processing/src/main/java/io/druid/jackson/AggregatorsModule.java @@ -98,6 +98,7 @@ public static interface AggregatorFactoryMixin @JsonSubTypes.Type(name = "expression", value = ExpressionPostAggregator.class), @JsonSubTypes.Type(name = "arithmetic", value = ArithmeticPostAggregator.class), @JsonSubTypes.Type(name = "fieldAccess", value = FieldAccessPostAggregator.class), + @JsonSubTypes.Type(name = "finalFieldAccess", value = FieldAccessPostAggregator.class), @JsonSubTypes.Type(name = "constant", value = ConstantPostAggregator.class), @JsonSubTypes.Type(name = "javascript", value = JavaScriptPostAggregator.class), @JsonSubTypes.Type(name = "hyperUniqueCardinality", value = HyperUniqueFinalizingPostAggregator.class), diff --git a/processing/src/main/java/io/druid/query/Queries.java b/processing/src/main/java/io/druid/query/Queries.java index 26afcc1bfc8c..3f9fd7dd6436 100644 --- a/processing/src/main/java/io/druid/query/Queries.java +++ b/processing/src/main/java/io/druid/query/Queries.java @@ -20,11 +20,16 @@ package io.druid.query; import com.google.common.base.Preconditions; +import com.google.common.base.Predicate; +import com.google.common.collect.Maps; import com.google.common.collect.Sets; import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.HasDependentAggFactories; import io.druid.query.aggregation.PostAggregator; +import javax.annotation.Nullable; import java.util.List; +import java.util.Map; import java.util.Set; /** @@ -38,15 +43,16 @@ public static void verifyAggregations( { Preconditions.checkNotNull(aggFactories, "aggregations cannot be null"); - final Set aggNames = Sets.newHashSet(); + final Map aggsFactoryMap = Maps.newHashMap(); for (AggregatorFactory aggFactory : aggFactories) { - Preconditions.checkArgument(aggNames.add(aggFactory.getName()), "[%s] already defined", aggFactory.getName()); + Preconditions.checkArgument(aggsFactoryMap.containsKey(aggFactory.getName()), "[%s] already defined", aggFactory.getName()); + aggsFactoryMap.put(aggFactory.getName(), aggFactory); } if (postAggs != null && !postAggs.isEmpty()) { - final Set combinedAggNames = Sets.newHashSet(aggNames); + final Set combinedAggNames = aggsFactoryMap.keySet(); - for (PostAggregator postAgg : postAggs) { + for (final PostAggregator postAgg : postAggs) { final Set dependencies = postAgg.getDependentFields(); final Set missing = Sets.difference(dependencies, combinedAggNames); @@ -55,6 +61,18 @@ public static void verifyAggregations( "Missing fields [%s] for postAggregator [%s]", missing, postAgg.getName() ); Preconditions.checkArgument(combinedAggNames.add(postAgg.getName()), "[%s] already defined", postAgg.getName()); + + if (postAgg instanceof HasDependentAggFactories) { + HasDependentAggFactories richPostAgg = (HasDependentAggFactories)postAgg; + richPostAgg.setDependentAggFactories(Maps.filterKeys(aggsFactoryMap, new Predicate() + { + @Override + public boolean apply(@Nullable String input) + { + return postAgg.getDependentFields().contains(input); + } + })); + } } } } diff --git a/processing/src/main/java/io/druid/query/aggregation/HasDependentAggFactories.java b/processing/src/main/java/io/druid/query/aggregation/HasDependentAggFactories.java new file mode 100644 index 000000000000..ba2ac3b337ed --- /dev/null +++ b/processing/src/main/java/io/druid/query/aggregation/HasDependentAggFactories.java @@ -0,0 +1,28 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package io.druid.query.aggregation; + +import java.util.Map; + +public interface HasDependentAggFactories +{ + void setDependentAggFactories(Map aggFactoryMap); + Map getDependentAggFactories(); +} diff --git a/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java new file mode 100644 index 000000000000..452b36605fcd --- /dev/null +++ b/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java @@ -0,0 +1,128 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package io.druid.query.aggregation.post; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.collect.Sets; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.HasDependentAggFactories; +import io.druid.query.aggregation.PostAggregator; + +import java.util.Comparator; +import java.util.Map; +import java.util.Set; + +public class FinalFieldAccessPostAggregator implements PostAggregator, HasDependentAggFactories +{ + private final String name; + private final String fieldName; + Map aggFactoryMap; + + @JsonCreator + public FinalFieldAccessPostAggregator( + @JsonProperty("name") String name, + @JsonProperty("fieldName") String fieldName + ) + { + this.name = name; + this.fieldName = fieldName; + } + + @Override + public Set getDependentFields() + { + return Sets.newHashSet(fieldName); + } + + @Override + public Comparator getComparator() + { + throw new UnsupportedOperationException(); + } + + @Override + public Object compute(Map combinedAggregators) + { + if (aggFactoryMap != null) { + return aggFactoryMap.get(fieldName).finalizeComputation( + combinedAggregators.get(fieldName) + ); + } else { + return combinedAggregators.get(fieldName); + } + } + + @Override + @JsonProperty + public String getName() + { + return name; + } + + @JsonProperty + public String getFieldName() + { + return fieldName; + } + + @Override + public String toString() + { + return "FinalFieldAccessPostAggregator{" + + "name'" + name + '\'' + + ", fieldName='" + fieldName + '\'' + + '}'; + } + + @Override + public boolean equals(Object o) + { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + FinalFieldAccessPostAggregator that = (FinalFieldAccessPostAggregator)o; + + if (fieldName != null ? !fieldName.equals(that.fieldName) : that.fieldName != null) return false; + if (name != null ? !name.equals(that.name) : that.name != null) return false; + + return true; + } + + @Override + public int hashCode() + { + int result = name != null ? name.hashCode() : 0; + result = 31 * result + (fieldName != null ? fieldName.hashCode() : 0); + return result; + } + + @Override + public void setDependentAggFactories(Map aggFactoryMap) + { + this.aggFactoryMap = aggFactoryMap; + } + + @Override + public Map getDependentAggFactories() + { + return aggFactoryMap; + } +} diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java new file mode 100644 index 000000000000..1c0d7c6392b9 --- /dev/null +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java @@ -0,0 +1,54 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package io.druid.query.aggregation.post; + +import io.druid.query.aggregation.CountAggregator; +import org.junit.Assert; +import org.junit.Test; + +import java.util.HashMap; +import java.util.Map; + +public class FinalFieldAccessPostAggregatorTest +{ + @Test + public void testComputeWithoutFinalizing() + { + FinalFieldAccessPostAggregator finalFieldAccessPostAggregator + = new FinalFieldAccessPostAggregator("finalName1", "rows"); + CountAggregator agg = new CountAggregator("rows"); + Map metricValues = new HashMap(); + metricValues.put(agg.getName(), agg.get()); + Assert.assertEquals(new Long(0L), finalFieldAccessPostAggregator.compute(metricValues)); + + agg.aggregate(); + agg.aggregate(); + agg.aggregate(); + metricValues.put(agg.getName(), agg.get()); + Assert.assertEquals(new Long(3L), finalFieldAccessPostAggregator.compute(metricValues)); + } + + @Test + public void testComputedWithFinalizing() + { + FinalFieldAccessPostAggregator finalFieldAccessPostAggregator + = new FinalFieldAccessPostAggregator("finalName1", "rows"); + } +} From 77f0d2e26dbd392641e8c80ce23f91be09c1dba4 Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Tue, 26 Apr 2016 02:38:10 +0900 Subject: [PATCH 02/13] fix some bugs to run a query --- .../src/main/java/io/druid/jackson/AggregatorsModule.java | 3 ++- processing/src/main/java/io/druid/query/Queries.java | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/processing/src/main/java/io/druid/jackson/AggregatorsModule.java b/processing/src/main/java/io/druid/jackson/AggregatorsModule.java index 3c7a09664b4e..0df8117b1002 100644 --- a/processing/src/main/java/io/druid/jackson/AggregatorsModule.java +++ b/processing/src/main/java/io/druid/jackson/AggregatorsModule.java @@ -48,6 +48,7 @@ import io.druid.query.aggregation.post.DoubleGreatestPostAggregator; import io.druid.query.aggregation.post.DoubleLeastPostAggregator; import io.druid.query.aggregation.post.FieldAccessPostAggregator; +import io.druid.query.aggregation.post.FinalFieldAccessPostAggregator; import io.druid.query.aggregation.post.JavaScriptPostAggregator; import io.druid.query.aggregation.post.ExpressionPostAggregator; import io.druid.query.aggregation.post.LongGreatestPostAggregator; @@ -98,7 +99,7 @@ public static interface AggregatorFactoryMixin @JsonSubTypes.Type(name = "expression", value = ExpressionPostAggregator.class), @JsonSubTypes.Type(name = "arithmetic", value = ArithmeticPostAggregator.class), @JsonSubTypes.Type(name = "fieldAccess", value = FieldAccessPostAggregator.class), - @JsonSubTypes.Type(name = "finalFieldAccess", value = FieldAccessPostAggregator.class), + @JsonSubTypes.Type(name = "finalFieldAccess", value = FinalFieldAccessPostAggregator.class), @JsonSubTypes.Type(name = "constant", value = ConstantPostAggregator.class), @JsonSubTypes.Type(name = "javascript", value = JavaScriptPostAggregator.class), @JsonSubTypes.Type(name = "hyperUniqueCardinality", value = HyperUniqueFinalizingPostAggregator.class), diff --git a/processing/src/main/java/io/druid/query/Queries.java b/processing/src/main/java/io/druid/query/Queries.java index 3f9fd7dd6436..4727898d167c 100644 --- a/processing/src/main/java/io/druid/query/Queries.java +++ b/processing/src/main/java/io/druid/query/Queries.java @@ -45,7 +45,7 @@ public static void verifyAggregations( final Map aggsFactoryMap = Maps.newHashMap(); for (AggregatorFactory aggFactory : aggFactories) { - Preconditions.checkArgument(aggsFactoryMap.containsKey(aggFactory.getName()), "[%s] already defined", aggFactory.getName()); + Preconditions.checkArgument(!aggsFactoryMap.containsKey(aggFactory.getName()), "[%s] already defined", aggFactory.getName()); aggsFactoryMap.put(aggFactory.getName(), aggFactory); } @@ -60,7 +60,6 @@ public static void verifyAggregations( missing.isEmpty(), "Missing fields [%s] for postAggregator [%s]", missing, postAgg.getName() ); - Preconditions.checkArgument(combinedAggNames.add(postAgg.getName()), "[%s] already defined", postAgg.getName()); if (postAgg instanceof HasDependentAggFactories) { HasDependentAggFactories richPostAgg = (HasDependentAggFactories)postAgg; From 59bc8aea4a9d8fe3f345fedf23aea4c7dfe74fe7 Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Tue, 26 Apr 2016 12:00:27 +0900 Subject: [PATCH 03/13] change name of method Queries.verifyAggregations to Queries.prepareAggregations --- processing/src/main/java/io/druid/query/Queries.java | 2 +- .../aggregation/post/FinalFieldAccessPostAggregator.java | 2 +- .../main/java/io/druid/query/groupby/GroupByQuery.java | 2 +- .../java/io/druid/query/timeseries/TimeseriesQuery.java | 2 +- .../src/main/java/io/druid/query/topn/TopNQuery.java | 2 +- processing/src/test/java/io/druid/query/QueriesTest.java | 8 ++++---- .../post/FinalFieldAccessPostAggregatorTest.java | 1 + 7 files changed, 10 insertions(+), 9 deletions(-) diff --git a/processing/src/main/java/io/druid/query/Queries.java b/processing/src/main/java/io/druid/query/Queries.java index 4727898d167c..6966f616c3a3 100644 --- a/processing/src/main/java/io/druid/query/Queries.java +++ b/processing/src/main/java/io/druid/query/Queries.java @@ -36,7 +36,7 @@ */ public class Queries { - public static void verifyAggregations( + public static void prepareAggregations( List aggFactories, List postAggs ) diff --git a/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java index 452b36605fcd..bbf346862015 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java @@ -61,7 +61,7 @@ public Comparator getComparator() @Override public Object compute(Map combinedAggregators) { - if (aggFactoryMap != null) { + if (aggFactoryMap != null && aggFactoryMap.containsValue(fieldName)) { return aggFactoryMap.get(fieldName).finalizeComputation( combinedAggregators.get(fieldName) ); diff --git a/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java b/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java index c5a67d0a1781..de3a6c419563 100644 --- a/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java +++ b/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java @@ -125,7 +125,7 @@ public GroupByQuery( this.limitSpec = (limitSpec == null) ? new NoopLimitSpec() : limitSpec; Preconditions.checkNotNull(this.granularity, "Must specify a granularity"); - Queries.verifyAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); + Queries.prepareAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); // Verify no duplicate names between dimensions, aggregators, and postAggregators. // They will all end up in the same namespace in the returned Rows and we can't have them clobbering each other. diff --git a/processing/src/main/java/io/druid/query/timeseries/TimeseriesQuery.java b/processing/src/main/java/io/druid/query/timeseries/TimeseriesQuery.java index 964bf38fb9d1..68689448b9c4 100644 --- a/processing/src/main/java/io/druid/query/timeseries/TimeseriesQuery.java +++ b/processing/src/main/java/io/druid/query/timeseries/TimeseriesQuery.java @@ -65,7 +65,7 @@ public TimeseriesQuery( this.aggregatorSpecs = aggregatorSpecs == null ? ImmutableList.of() : aggregatorSpecs; this.postAggregatorSpecs = postAggregatorSpecs == null ? ImmutableList.of() : postAggregatorSpecs; - Queries.verifyAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); + Queries.prepareAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); } @Override diff --git a/processing/src/main/java/io/druid/query/topn/TopNQuery.java b/processing/src/main/java/io/druid/query/topn/TopNQuery.java index 2221e65d4863..006c896053fa 100644 --- a/processing/src/main/java/io/druid/query/topn/TopNQuery.java +++ b/processing/src/main/java/io/druid/query/topn/TopNQuery.java @@ -82,7 +82,7 @@ public TopNQuery( Preconditions.checkArgument(threshold != 0, "Threshold cannot be equal to 0."); topNMetricSpec.verifyPreconditions(this.aggregatorSpecs, this.postAggregatorSpecs); - Queries.verifyAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); + Queries.prepareAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); } @Override diff --git a/processing/src/test/java/io/druid/query/QueriesTest.java b/processing/src/test/java/io/druid/query/QueriesTest.java index 1cc8aceaeef8..ba4ddb516091 100644 --- a/processing/src/test/java/io/druid/query/QueriesTest.java +++ b/processing/src/test/java/io/druid/query/QueriesTest.java @@ -59,7 +59,7 @@ public void testVerifyAggregations() throws Exception boolean exceptionOccured = false; try { - Queries.verifyAggregations(aggFactories, postAggs); + Queries.prepareAggregations(aggFactories, postAggs); } catch (IllegalArgumentException e) { exceptionOccured = true; @@ -91,7 +91,7 @@ public void testVerifyAggregationsMissingVal() throws Exception boolean exceptionOccured = false; try { - Queries.verifyAggregations(aggFactories, postAggs); + Queries.prepareAggregations(aggFactories, postAggs); } catch (IllegalArgumentException e) { exceptionOccured = true; @@ -145,7 +145,7 @@ public void testVerifyAggregationsMultiLevel() throws Exception boolean exceptionOccured = false; try { - Queries.verifyAggregations(aggFactories, postAggs); + Queries.prepareAggregations(aggFactories, postAggs); } catch (IllegalArgumentException e) { exceptionOccured = true; @@ -199,7 +199,7 @@ public void testVerifyAggregationsMultiLevelMissingVal() throws Exception boolean exceptionOccured = false; try { - Queries.verifyAggregations(aggFactories, postAggs); + Queries.prepareAggregations(aggFactories, postAggs); } catch (IllegalArgumentException e) { exceptionOccured = true; diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java index 1c0d7c6392b9..36075568a7a6 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java @@ -50,5 +50,6 @@ public void testComputedWithFinalizing() { FinalFieldAccessPostAggregator finalFieldAccessPostAggregator = new FinalFieldAccessPostAggregator("finalName1", "rows"); + } } From 84681990ed79fb1079028f7b471c5891d3274a82 Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Tue, 26 Apr 2016 19:20:26 +0900 Subject: [PATCH 04/13] add Uts --- .../post/FinalFieldAccessPostAggregator.java | 7 +- .../FinalFieldAccessPostAggregatorTest.java | 109 ++++++++++++++++-- 2 files changed, 103 insertions(+), 13 deletions(-) diff --git a/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java index bbf346862015..14b6e1c7a7d3 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java @@ -34,7 +34,7 @@ public class FinalFieldAccessPostAggregator implements PostAggregator, HasDepend { private final String name; private final String fieldName; - Map aggFactoryMap; + private Map aggFactoryMap; @JsonCreator public FinalFieldAccessPostAggregator( @@ -61,7 +61,7 @@ public Comparator getComparator() @Override public Object compute(Map combinedAggregators) { - if (aggFactoryMap != null && aggFactoryMap.containsValue(fieldName)) { + if (aggFactoryMap != null && aggFactoryMap.containsKey(fieldName)) { return aggFactoryMap.get(fieldName).finalizeComputation( combinedAggregators.get(fieldName) ); @@ -89,6 +89,7 @@ public String toString() return "FinalFieldAccessPostAggregator{" + "name'" + name + '\'' + ", fieldName='" + fieldName + '\'' + + ", aggFactoryMap='" + aggFactoryMap + '\'' + '}'; } @@ -102,6 +103,7 @@ public boolean equals(Object o) if (fieldName != null ? !fieldName.equals(that.fieldName) : that.fieldName != null) return false; if (name != null ? !name.equals(that.name) : that.name != null) return false; + if (aggFactoryMap != null ? !aggFactoryMap.equals(that.aggFactoryMap) : that.aggFactoryMap != null) return false; return true; } @@ -111,6 +113,7 @@ public int hashCode() { int result = name != null ? name.hashCode() : 0; result = 31 * result + (fieldName != null ? fieldName.hashCode() : 0); + result = 31 * result + (aggFactoryMap != null ? aggFactoryMap.hashCode() : 0); return result; } diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java index 36075568a7a6..809a4ea5151a 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java @@ -19,37 +19,124 @@ package io.druid.query.aggregation.post; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import com.metamx.common.guava.Sequence; +import com.metamx.common.guava.Sequences; +import io.druid.data.input.MapBasedRow; +import io.druid.granularity.QueryGranularity; +import io.druid.jackson.AggregatorsModule; +import io.druid.query.aggregation.AggregationTestHelper; +import io.druid.query.aggregation.Aggregator; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.CountAggregator; +import io.druid.query.aggregation.PostAggregator; +import org.easymock.EasyMock; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; -import java.util.HashMap; +import java.io.File; import java.util.Map; public class FinalFieldAccessPostAggregatorTest { + @Rule + public final TemporaryFolder tempFoler = new TemporaryFolder(); + @Test public void testComputeWithoutFinalizing() { - FinalFieldAccessPostAggregator finalFieldAccessPostAggregator - = new FinalFieldAccessPostAggregator("finalName1", "rows"); - CountAggregator agg = new CountAggregator("rows"); - Map metricValues = new HashMap(); - metricValues.put(agg.getName(), agg.get()); - Assert.assertEquals(new Long(0L), finalFieldAccessPostAggregator.compute(metricValues)); - + Aggregator agg = new CountAggregator("rows"); agg.aggregate(); agg.aggregate(); agg.aggregate(); + + Map metricValues = Maps.newHashMap(); metricValues.put(agg.getName(), agg.get()); - Assert.assertEquals(new Long(3L), finalFieldAccessPostAggregator.compute(metricValues)); + + PostAggregator postAgg = new FinalFieldAccessPostAggregator("final_rows", "rows"); + Assert.assertEquals(new Long(3L), postAgg.compute(metricValues)); } @Test public void testComputedWithFinalizing() { - FinalFieldAccessPostAggregator finalFieldAccessPostAggregator - = new FinalFieldAccessPostAggregator("finalName1", "rows"); + AggregatorFactory aggFactory = EasyMock.createMock(AggregatorFactory.class); + EasyMock.expect(aggFactory.finalizeComputation("test")) + .andReturn(new Long(3L)) + .times(1); + EasyMock.replay(aggFactory); + + FinalFieldAccessPostAggregator postAgg = new FinalFieldAccessPostAggregator("final_billy", "billy"); + postAgg.setDependentAggFactories(ImmutableMap.of("billy", aggFactory)); + + Map metricValues = Maps.newHashMap(); + metricValues.put("billy", "test"); + + Assert.assertEquals(new Long(3L), postAgg.compute(metricValues)); + EasyMock.verify(aggFactory); + } + + @Test + public void tesstIngestAndQuery() throws Exception + { + AggregationTestHelper helper = AggregationTestHelper.createGroupByQueryAggregationTestHelper( + Lists.newArrayList(new AggregatorsModule()), + tempFoler + ); + + String metricSpec = "[{" + + "\"type\": \"hyperUnique\"," + + "\"name\": \"index_hll\"," + + "\"fieldName\": \"market\"" + + "}]"; + + String parseSpec = "{" + + "\"type\" : \"string\"," + + "\"parseSpec\" : {" + + " \"format\" : \"tsv\"," + + " \"timestampSpec\" : {" + + " \"column\" : \"timestamp\"," + + " \"format\" : \"auto\"" + + "}," + + " \"dimensionsSpec\" : {" + + " \"dimensions\": []," + + " \"dimensionExclusions\" : []," + + " \"spatialDimensions\" : []" + + " }," + + " \"columns\": [\"timestamp\", \"market\", \"quality\", \"placement\", \"placementish\", \"index\"]" + + " }" + + "}"; + + String query = "{" + + "\"queryType\": \"groupBy\"," + + "\"dataSource\": \"test_datasource\"," + + "\"granularity\": \"ALL\"," + + "\"dimensions\": []," + + "\"aggregations\": [" + + " { \"type\": \"hyperUnique\", \"name\": \"index_hll\", \"fieldName\": \"index_hll\" }" + + "]," + + "\"postAggregations\": [" + + " { \"type\": \"finalFieldAccess\", \"name\": \"index_unique_count\", \"fieldName\": \"index_hll\" }" + + "]," + + "\"intervals\": [ \"1970/2050\" ]" + + "}"; + + Sequence seq = helper.createIndexAndRunQueryOnSegment( + new File(this.getClass().getClassLoader().getResource("druid.sample.tsv").getFile()), + parseSpec, + metricSpec, + 0, + QueryGranularity.NONE, + 50000, + query + ); + MapBasedRow row = (MapBasedRow) Sequences.toList(seq, Lists.newArrayList()).get(0); + Assert.assertEquals(3.0, row.getFloatMetric("index_hll"), 0.1); + Assert.assertEquals(3.0, row.getFloatMetric("index_unique_count"), 0.1); } } From 4b07db9fe9ec12f9082a4e8b668a777656e8d3dd Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Tue, 26 Apr 2016 20:09:34 +0900 Subject: [PATCH 05/13] fix Ut failures --- processing/src/main/java/io/druid/query/Queries.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/processing/src/main/java/io/druid/query/Queries.java b/processing/src/main/java/io/druid/query/Queries.java index 6966f616c3a3..9c890d3c6ed7 100644 --- a/processing/src/main/java/io/druid/query/Queries.java +++ b/processing/src/main/java/io/druid/query/Queries.java @@ -50,7 +50,7 @@ public static void prepareAggregations( } if (postAggs != null && !postAggs.isEmpty()) { - final Set combinedAggNames = aggsFactoryMap.keySet(); + final Set combinedAggNames = Sets.newHashSet(aggsFactoryMap.keySet()); for (final PostAggregator postAgg : postAggs) { final Set dependencies = postAgg.getDependentFields(); @@ -60,6 +60,7 @@ public static void prepareAggregations( missing.isEmpty(), "Missing fields [%s] for postAggregator [%s]", missing, postAgg.getName() ); + Preconditions.checkArgument(combinedAggNames.add(postAgg.getName()), "[%s] already defined", postAgg.getName()); if (postAgg instanceof HasDependentAggFactories) { HasDependentAggFactories richPostAgg = (HasDependentAggFactories)postAgg; From d4dcd2cb1d4949c835ceacf56c31ebc5e0cab91f Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Wed, 24 Aug 2016 13:42:09 +0900 Subject: [PATCH 06/13] rebased to master --- .../aggregation/post/FinalFieldAccessPostAggregatorTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java index 809a4ea5151a..4c35382d3c22 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java @@ -25,6 +25,7 @@ import com.metamx.common.guava.Sequence; import com.metamx.common.guava.Sequences; import io.druid.data.input.MapBasedRow; +import io.druid.granularity.QueryGranularities; import io.druid.granularity.QueryGranularity; import io.druid.jackson.AggregatorsModule; import io.druid.query.aggregation.AggregationTestHelper; @@ -130,7 +131,7 @@ public void tesstIngestAndQuery() throws Exception parseSpec, metricSpec, 0, - QueryGranularity.NONE, + QueryGranularities.NONE, 50000, query ); From c3b4fe968803c28e3c56b4d8a56f8500871e07ec Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Fri, 21 Oct 2016 11:00:43 +0900 Subject: [PATCH 07/13] address comments and add a Ut for arithmetic post aggregators --- .../io/druid/jackson/AggregatorsModule.java | 4 +-- ... FinalizingFieldAccessPostAggregator.java} | 8 ++--- ...alizingFieldAccessPostAggregatorTest.java} | 34 ++++++++++++++++--- 3 files changed, 35 insertions(+), 11 deletions(-) rename processing/src/main/java/io/druid/query/aggregation/post/{FinalFieldAccessPostAggregator.java => FinalizingFieldAccessPostAggregator.java} (92%) rename processing/src/test/java/io/druid/query/aggregation/post/{FinalFieldAccessPostAggregatorTest.java => FinalizingFieldAccessPostAggregatorTest.java} (78%) diff --git a/processing/src/main/java/io/druid/jackson/AggregatorsModule.java b/processing/src/main/java/io/druid/jackson/AggregatorsModule.java index 0df8117b1002..fad8ba7eb524 100644 --- a/processing/src/main/java/io/druid/jackson/AggregatorsModule.java +++ b/processing/src/main/java/io/druid/jackson/AggregatorsModule.java @@ -48,7 +48,7 @@ import io.druid.query.aggregation.post.DoubleGreatestPostAggregator; import io.druid.query.aggregation.post.DoubleLeastPostAggregator; import io.druid.query.aggregation.post.FieldAccessPostAggregator; -import io.druid.query.aggregation.post.FinalFieldAccessPostAggregator; +import io.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator; import io.druid.query.aggregation.post.JavaScriptPostAggregator; import io.druid.query.aggregation.post.ExpressionPostAggregator; import io.druid.query.aggregation.post.LongGreatestPostAggregator; @@ -99,7 +99,7 @@ public static interface AggregatorFactoryMixin @JsonSubTypes.Type(name = "expression", value = ExpressionPostAggregator.class), @JsonSubTypes.Type(name = "arithmetic", value = ArithmeticPostAggregator.class), @JsonSubTypes.Type(name = "fieldAccess", value = FieldAccessPostAggregator.class), - @JsonSubTypes.Type(name = "finalFieldAccess", value = FinalFieldAccessPostAggregator.class), + @JsonSubTypes.Type(name = "finalizingFieldAccess", value = FinalizingFieldAccessPostAggregator.class), @JsonSubTypes.Type(name = "constant", value = ConstantPostAggregator.class), @JsonSubTypes.Type(name = "javascript", value = JavaScriptPostAggregator.class), @JsonSubTypes.Type(name = "hyperUniqueCardinality", value = HyperUniqueFinalizingPostAggregator.class), diff --git a/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java similarity index 92% rename from processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java rename to processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java index 14b6e1c7a7d3..b66309f3f8d4 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java @@ -30,14 +30,14 @@ import java.util.Map; import java.util.Set; -public class FinalFieldAccessPostAggregator implements PostAggregator, HasDependentAggFactories +public class FinalizingFieldAccessPostAggregator implements PostAggregator, HasDependentAggFactories { private final String name; private final String fieldName; private Map aggFactoryMap; @JsonCreator - public FinalFieldAccessPostAggregator( + public FinalizingFieldAccessPostAggregator( @JsonProperty("name") String name, @JsonProperty("fieldName") String fieldName ) @@ -86,7 +86,7 @@ public String getFieldName() @Override public String toString() { - return "FinalFieldAccessPostAggregator{" + + return "FinalizingFieldAccessPostAggregator{" + "name'" + name + '\'' + ", fieldName='" + fieldName + '\'' + ", aggFactoryMap='" + aggFactoryMap + '\'' + @@ -99,7 +99,7 @@ public boolean equals(Object o) if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; - FinalFieldAccessPostAggregator that = (FinalFieldAccessPostAggregator)o; + FinalizingFieldAccessPostAggregator that = (FinalizingFieldAccessPostAggregator)o; if (fieldName != null ? !fieldName.equals(that.fieldName) : that.fieldName != null) return false; if (name != null ? !name.equals(that.name) : that.name != null) return false; diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java similarity index 78% rename from processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java rename to processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java index 4c35382d3c22..6951ee332b9c 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java @@ -26,7 +26,6 @@ import com.metamx.common.guava.Sequences; import io.druid.data.input.MapBasedRow; import io.druid.granularity.QueryGranularities; -import io.druid.granularity.QueryGranularity; import io.druid.jackson.AggregatorsModule; import io.druid.query.aggregation.AggregationTestHelper; import io.druid.query.aggregation.Aggregator; @@ -40,9 +39,10 @@ import org.junit.rules.TemporaryFolder; import java.io.File; +import java.util.List; import java.util.Map; -public class FinalFieldAccessPostAggregatorTest +public class FinalizingFieldAccessPostAggregatorTest { @Rule public final TemporaryFolder tempFoler = new TemporaryFolder(); @@ -58,7 +58,7 @@ public void testComputeWithoutFinalizing() Map metricValues = Maps.newHashMap(); metricValues.put(agg.getName(), agg.get()); - PostAggregator postAgg = new FinalFieldAccessPostAggregator("final_rows", "rows"); + PostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_rows", "rows"); Assert.assertEquals(new Long(3L), postAgg.compute(metricValues)); } @@ -71,7 +71,7 @@ public void testComputedWithFinalizing() .times(1); EasyMock.replay(aggFactory); - FinalFieldAccessPostAggregator postAgg = new FinalFieldAccessPostAggregator("final_billy", "billy"); + FinalizingFieldAccessPostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_billy", "billy"); postAgg.setDependentAggFactories(ImmutableMap.of("billy", aggFactory)); Map metricValues = Maps.newHashMap(); @@ -81,6 +81,30 @@ public void testComputedWithFinalizing() EasyMock.verify(aggFactory); } + @Test + public void testComputedInArithmeticPostAggregator() + { + AggregatorFactory aggFactory = EasyMock.createMock(AggregatorFactory.class); + EasyMock.expect(aggFactory.finalizeComputation("test")) + .andReturn(new Long(3L)) + .times(1); + EasyMock.replay(aggFactory); + + FinalizingFieldAccessPostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_billy", "billy"); + postAgg.setDependentAggFactories(ImmutableMap.of("billy", aggFactory)); + + Map metricValues = Maps.newHashMap(); + metricValues.put("billy", "test"); + + List postAggsList = Lists.newArrayList( + new ConstantPostAggregator("roku", 6), postAgg); + + ArithmeticPostAggregator arithmeticPostAggregator = new ArithmeticPostAggregator("add", "+", postAggsList); + + Assert.assertEquals(new Double(9.0f), arithmeticPostAggregator.compute(metricValues)); + EasyMock.verify(); + } + @Test public void tesstIngestAndQuery() throws Exception { @@ -121,7 +145,7 @@ public void tesstIngestAndQuery() throws Exception + " { \"type\": \"hyperUnique\", \"name\": \"index_hll\", \"fieldName\": \"index_hll\" }" + "]," + "\"postAggregations\": [" - + " { \"type\": \"finalFieldAccess\", \"name\": \"index_unique_count\", \"fieldName\": \"index_hll\" }" + + " { \"type\": \"finalizingFieldAccess\", \"name\": \"index_unique_count\", \"fieldName\": \"index_hll\" }" + "]," + "\"intervals\": [ \"1970/2050\" ]" + "}"; From 7c1a4fb5ab30beb22dc805b30cdc6fbe7d05adf7 Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Sun, 25 Dec 2016 21:22:54 +0900 Subject: [PATCH 08/13] rebased to the master --- .../FinalizingFieldAccessPostAggregator.java | 20 ++++++++++++++----- ...nalizingFieldAccessPostAggregatorTest.java | 10 ++++++---- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java index b66309f3f8d4..19ee014ad5cf 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java @@ -96,14 +96,24 @@ public String toString() @Override public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } FinalizingFieldAccessPostAggregator that = (FinalizingFieldAccessPostAggregator)o; - if (fieldName != null ? !fieldName.equals(that.fieldName) : that.fieldName != null) return false; - if (name != null ? !name.equals(that.name) : that.name != null) return false; - if (aggFactoryMap != null ? !aggFactoryMap.equals(that.aggFactoryMap) : that.aggFactoryMap != null) return false; + if (fieldName != null ? !fieldName.equals(that.fieldName) : that.fieldName != null) { + return false; + } + if (name != null ? !name.equals(that.name) : that.name != null) { + return false; + } + if (aggFactoryMap != null ? !aggFactoryMap.equals(that.aggFactoryMap) : that.aggFactoryMap != null) { + return false; + } return true; } diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java index 6951ee332b9c..2ae055e7ec2a 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java @@ -22,16 +22,17 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.metamx.common.guava.Sequence; -import com.metamx.common.guava.Sequences; import io.druid.data.input.MapBasedRow; import io.druid.granularity.QueryGranularities; import io.druid.jackson.AggregatorsModule; +import io.druid.java.util.common.guava.Sequence; +import io.druid.java.util.common.guava.Sequences; import io.druid.query.aggregation.AggregationTestHelper; import io.druid.query.aggregation.Aggregator; import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.CountAggregator; import io.druid.query.aggregation.PostAggregator; +import io.druid.query.groupby.GroupByQueryRunnerTest; import org.easymock.EasyMock; import org.junit.Assert; import org.junit.Rule; @@ -50,7 +51,7 @@ public class FinalizingFieldAccessPostAggregatorTest @Test public void testComputeWithoutFinalizing() { - Aggregator agg = new CountAggregator("rows"); + Aggregator agg = new CountAggregator(); agg.aggregate(); agg.aggregate(); agg.aggregate(); @@ -106,10 +107,11 @@ public void testComputedInArithmeticPostAggregator() } @Test - public void tesstIngestAndQuery() throws Exception + public void testIngestAndQuery() throws Exception { AggregationTestHelper helper = AggregationTestHelper.createGroupByQueryAggregationTestHelper( Lists.newArrayList(new AggregatorsModule()), + GroupByQueryRunnerTest.testConfigs().get(0), tempFoler ); From e3626129df23396293da33104c7f233d650eecdb Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Sun, 25 Dec 2016 23:58:50 +0900 Subject: [PATCH 09/13] address the comment of injection within arithmetic post aggregator --- .../src/main/java/io/druid/query/Queries.java | 12 +++------- .../post/ArithmeticPostAggregator.java | 23 ++++++++++++++++++- ...nalizingFieldAccessPostAggregatorTest.java | 22 ++++++++++-------- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/processing/src/main/java/io/druid/query/Queries.java b/processing/src/main/java/io/druid/query/Queries.java index 9c890d3c6ed7..fac53206b9a4 100644 --- a/processing/src/main/java/io/druid/query/Queries.java +++ b/processing/src/main/java/io/druid/query/Queries.java @@ -24,6 +24,7 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.AggregatorUtil; import io.druid.query.aggregation.HasDependentAggFactories; import io.druid.query.aggregation.PostAggregator; @@ -63,15 +64,8 @@ public static void prepareAggregations( Preconditions.checkArgument(combinedAggNames.add(postAgg.getName()), "[%s] already defined", postAgg.getName()); if (postAgg instanceof HasDependentAggFactories) { - HasDependentAggFactories richPostAgg = (HasDependentAggFactories)postAgg; - richPostAgg.setDependentAggFactories(Maps.filterKeys(aggsFactoryMap, new Predicate() - { - @Override - public boolean apply(@Nullable String input) - { - return postAgg.getDependentFields().contains(input); - } - })); + HasDependentAggFactories richPostAgg = (HasDependentAggFactories) postAgg; + richPostAgg.setDependentAggFactories(aggsFactoryMap); } } } diff --git a/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java index 989118441cb5..33ddae4cf4d7 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java @@ -25,6 +25,8 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; import io.druid.java.util.common.IAE; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.HasDependentAggFactories; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -35,7 +37,7 @@ /** */ -public class ArithmeticPostAggregator implements PostAggregator +public class ArithmeticPostAggregator implements PostAggregator, HasDependentAggFactories { public static final Comparator DEFAULT_COMPARATOR = new Comparator() { @@ -52,6 +54,7 @@ public int compare(Object o, Object o1) private final Ops op; private final Comparator comparator; private final String ordering; + private Map aggFactoryMap; public ArithmeticPostAggregator( String name, @@ -152,6 +155,24 @@ public String toString() '}'; } + @Override + public void setDependentAggFactories(Map aggFactoryMap) + { + this.aggFactoryMap = aggFactoryMap; + for (PostAggregator postAgg : fields) { + if (postAgg instanceof HasDependentAggFactories) { + HasDependentAggFactories richPostAgg = (HasDependentAggFactories) postAgg; + richPostAgg.setDependentAggFactories(this.aggFactoryMap); + } + } + } + + @Override + public Map getDependentAggFactories() + { + return aggFactoryMap; + } + private static enum Ops { PLUS("+") diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java index 2ae055e7ec2a..a8d8342ca89b 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java @@ -107,7 +107,7 @@ public void testComputedInArithmeticPostAggregator() } @Test - public void testIngestAndQuery() throws Exception + public void testIngestAndQueryWithArithmeticPostAggregator() throws Exception { AggregationTestHelper helper = AggregationTestHelper.createGroupByQueryAggregationTestHelper( Lists.newArrayList(new AggregatorsModule()), @@ -115,11 +115,8 @@ public void testIngestAndQuery() throws Exception tempFoler ); - String metricSpec = "[{" - + "\"type\": \"hyperUnique\"," - + "\"name\": \"index_hll\"," - + "\"fieldName\": \"market\"" - + "}]"; + String metricSpec = "[{\"type\": \"hyperUnique\", \"name\": \"hll_market\", \"fieldName\": \"market\"}," + + "{\"type\": \"hyperUnique\", \"name\": \"hll_quality\", \"fieldName\": \"quality\"}]"; String parseSpec = "{" + "\"type\" : \"string\"," @@ -144,10 +141,14 @@ public void testIngestAndQuery() throws Exception + "\"granularity\": \"ALL\"," + "\"dimensions\": []," + "\"aggregations\": [" - + " { \"type\": \"hyperUnique\", \"name\": \"index_hll\", \"fieldName\": \"index_hll\" }" + + " { \"type\": \"hyperUnique\", \"name\": \"hll_market\", \"fieldName\": \"hll_market\" }," + + " { \"type\": \"hyperUnique\", \"name\": \"hll_quality\", \"fieldName\": \"hll_quality\" }" + "]," + "\"postAggregations\": [" - + " { \"type\": \"finalizingFieldAccess\", \"name\": \"index_unique_count\", \"fieldName\": \"index_hll\" }" + + " { \"type\": \"arithmetic\", \"name\": \"uniq_add\", \"fn\": \"+\", \"fields\":[" + + " { \"type\": \"finalizingFieldAccess\", \"name\": \"uniq_market\", \"fieldName\": \"hll_market\" }," + + " { \"type\": \"finalizingFieldAccess\", \"name\": \"uniq_quality\", \"fieldName\": \"hll_quality\" }]" + + " }" + "]," + "\"intervals\": [ \"1970/2050\" ]" + "}"; @@ -163,7 +164,8 @@ public void testIngestAndQuery() throws Exception ); MapBasedRow row = (MapBasedRow) Sequences.toList(seq, Lists.newArrayList()).get(0); - Assert.assertEquals(3.0, row.getFloatMetric("index_hll"), 0.1); - Assert.assertEquals(3.0, row.getFloatMetric("index_unique_count"), 0.1); + Assert.assertEquals(3.0, row.getFloatMetric("hll_market"), 0.1); + Assert.assertEquals(9.0, row.getFloatMetric("hll_quality"), 0.1); + Assert.assertEquals(12.0, row.getFloatMetric("uniq_add"), 0.1); } } From 74aa554c7e7442d4c2b0c3e3ce8eae07744f721f Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Wed, 4 Jan 2017 10:16:48 +0900 Subject: [PATCH 10/13] address comments and introduce decorate() in the PostAggregator interface. --- .../theta/SketchEstimatePostAggregator.java | 7 +++ .../theta/SketchSetPostAggregator.java | 7 +++ .../histogram/BucketsPostAggregator.java | 8 +++ .../CustomBucketsPostAggregator.java | 8 +++ .../histogram/EqualBucketsPostAggregator.java | 8 +++ .../histogram/MaxPostAggregator.java | 8 +++ .../histogram/MinPostAggregator.java | 8 +++ .../histogram/QuantilePostAggregator.java | 8 +++ .../histogram/QuantilesPostAggregator.java | 8 +++ .../StandardDeviationPostAggregator.java | 7 +++ .../src/main/java/io/druid/query/Queries.java | 32 +++++++----- .../aggregation/HasDependentAggFactories.java | 28 ----------- .../query/aggregation/PostAggregator.java | 2 + .../HyperUniqueFinalizingPostAggregator.java | 7 +++ .../post/ArithmeticPostAggregator.java | 28 +++-------- .../post/ConstantPostAggregator.java | 7 +++ .../post/DoubleGreatestPostAggregator.java | 8 +++ .../post/DoubleLeastPostAggregator.java | 8 +++ .../post/ExpressionPostAggregator.java | 7 +++ .../post/FieldAccessPostAggregator.java | 7 +++ .../FinalizingFieldAccessPostAggregator.java | 49 ++++++++++--------- .../post/JavaScriptPostAggregator.java | 7 +++ .../post/LongGreatestPostAggregator.java | 8 +++ .../post/LongLeastPostAggregator.java | 8 +++ .../io/druid/query/groupby/GroupByQuery.java | 7 ++- .../query/timeseries/TimeseriesQuery.java | 30 +++++++----- .../java/io/druid/query/topn/TopNQuery.java | 16 ++++-- ...nalizingFieldAccessPostAggregatorTest.java | 14 +++--- 28 files changed, 241 insertions(+), 109 deletions(-) delete mode 100644 processing/src/main/java/io/druid/query/aggregation/HasDependentAggFactories.java diff --git a/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchEstimatePostAggregator.java b/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchEstimatePostAggregator.java index 373f7a30cd4c..9d13f0598062 100644 --- a/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchEstimatePostAggregator.java +++ b/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchEstimatePostAggregator.java @@ -25,6 +25,7 @@ import com.google.common.collect.Ordering; import com.google.common.collect.Sets; import com.google.common.primitives.Doubles; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -95,6 +96,12 @@ public String getName() return name; } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public PostAggregator getField() { diff --git a/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchSetPostAggregator.java b/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchSetPostAggregator.java index 40849ce37a40..1aa490b5eefb 100644 --- a/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchSetPostAggregator.java +++ b/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchSetPostAggregator.java @@ -25,6 +25,7 @@ import com.yahoo.sketches.Util; import io.druid.java.util.common.IAE; import io.druid.java.util.common.logger.Logger; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -95,6 +96,12 @@ public String getName() return name; } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public String getFunc() { diff --git a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/BucketsPostAggregator.java b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/BucketsPostAggregator.java index f117f507f145..56372d357b6e 100644 --- a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/BucketsPostAggregator.java +++ b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/BucketsPostAggregator.java @@ -25,6 +25,8 @@ import com.google.common.collect.Sets; import io.druid.java.util.common.IAE; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.PostAggregator; import java.util.Map; import java.util.Set; @@ -67,6 +69,12 @@ public Object compute(Map values) return ah.toHistogram(bucketSize, offset); } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public float getBucketSize() { diff --git a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/CustomBucketsPostAggregator.java b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/CustomBucketsPostAggregator.java index aa958601a2ad..f312369ff2ae 100644 --- a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/CustomBucketsPostAggregator.java +++ b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/CustomBucketsPostAggregator.java @@ -23,6 +23,8 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonTypeName; import com.google.common.collect.Sets; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.PostAggregator; import java.util.Arrays; import java.util.Map; @@ -59,6 +61,12 @@ public Object compute(Map values) return ah.toHistogram(breaks); } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public float[] getBreaks() { diff --git a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/EqualBucketsPostAggregator.java b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/EqualBucketsPostAggregator.java index 60e9de60e81a..0afe3b7383a0 100644 --- a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/EqualBucketsPostAggregator.java +++ b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/EqualBucketsPostAggregator.java @@ -25,6 +25,8 @@ import com.google.common.collect.Sets; import io.druid.java.util.common.IAE; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.PostAggregator; import java.util.Map; import java.util.Set; @@ -63,6 +65,12 @@ public Object compute(Map values) return ah.toHistogram(numBuckets); } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public int getNumBuckets() { diff --git a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/MaxPostAggregator.java b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/MaxPostAggregator.java index 120f4bb9f088..a0bdb6623469 100644 --- a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/MaxPostAggregator.java +++ b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/MaxPostAggregator.java @@ -23,6 +23,8 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonTypeName; import com.google.common.collect.Sets; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; import java.util.Map; @@ -71,6 +73,12 @@ public Object compute(Map values) return ah.getMax(); } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @Override public String toString() { diff --git a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/MinPostAggregator.java b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/MinPostAggregator.java index fbf030c6a65a..35f090895030 100644 --- a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/MinPostAggregator.java +++ b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/MinPostAggregator.java @@ -23,6 +23,8 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonTypeName; import com.google.common.collect.Sets; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; import java.util.Map; @@ -71,6 +73,12 @@ public Object compute(Map values) return ah.getMin(); } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @Override public String toString() { diff --git a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/QuantilePostAggregator.java b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/QuantilePostAggregator.java index 7eefe04ff59f..1406379b77de 100644 --- a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/QuantilePostAggregator.java +++ b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/QuantilePostAggregator.java @@ -25,6 +25,8 @@ import com.google.common.collect.Sets; import io.druid.java.util.common.IAE; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; import java.util.Map; @@ -80,6 +82,12 @@ public Object compute(Map values) return ah.getQuantiles(new float[]{this.getProbability()})[0]; } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public float getProbability() { diff --git a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/QuantilesPostAggregator.java b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/QuantilesPostAggregator.java index 7999a45f341a..1387d97aa0ef 100644 --- a/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/QuantilesPostAggregator.java +++ b/extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/QuantilesPostAggregator.java @@ -25,6 +25,8 @@ import com.google.common.collect.Sets; import io.druid.java.util.common.IAE; +import io.druid.query.aggregation.AggregatorFactory; +import io.druid.query.aggregation.PostAggregator; import java.util.Arrays; import java.util.Comparator; @@ -75,6 +77,12 @@ public Object compute(Map values) return new Quantiles(this.getProbabilities(), ah.getQuantiles(this.getProbabilities()), ah.getMin(), ah.getMax()); } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public float[] getProbabilities() { diff --git a/extensions-core/stats/src/main/java/io/druid/query/aggregation/variance/StandardDeviationPostAggregator.java b/extensions-core/stats/src/main/java/io/druid/query/aggregation/variance/StandardDeviationPostAggregator.java index 2bdcb0da9c91..e4ea6838ecde 100644 --- a/extensions-core/stats/src/main/java/io/druid/query/aggregation/variance/StandardDeviationPostAggregator.java +++ b/extensions-core/stats/src/main/java/io/druid/query/aggregation/variance/StandardDeviationPostAggregator.java @@ -24,6 +24,7 @@ import com.fasterxml.jackson.annotation.JsonTypeName; import com.google.common.base.Preconditions; import com.google.common.collect.Sets; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import io.druid.query.aggregation.post.ArithmeticPostAggregator; @@ -80,6 +81,12 @@ public String getName() return name; } + @Override + public PostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty("fieldName") public String getFieldName() { diff --git a/processing/src/main/java/io/druid/query/Queries.java b/processing/src/main/java/io/druid/query/Queries.java index fac53206b9a4..c48a876c1d3f 100644 --- a/processing/src/main/java/io/druid/query/Queries.java +++ b/processing/src/main/java/io/druid/query/Queries.java @@ -20,15 +20,12 @@ package io.druid.query; import com.google.common.base.Preconditions; -import com.google.common.base.Predicate; +import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; import io.druid.query.aggregation.AggregatorFactory; -import io.druid.query.aggregation.AggregatorUtil; -import io.druid.query.aggregation.HasDependentAggFactories; import io.druid.query.aggregation.PostAggregator; -import javax.annotation.Nullable; import java.util.List; import java.util.Map; import java.util.Set; @@ -37,7 +34,17 @@ */ public class Queries { - public static void prepareAggregations( + public static List decorate(List postAggs, + Map aggFactories) + { + List decorated = Lists.newArrayListWithExpectedSize(postAggs.size()); + for (PostAggregator aggregator : postAggs) { + decorated.add(aggregator.decorate(aggFactories)); + } + return decorated; + } + + public static List prepareAggregations( List aggFactories, List postAggs ) @@ -46,13 +53,15 @@ public static void prepareAggregations( final Map aggsFactoryMap = Maps.newHashMap(); for (AggregatorFactory aggFactory : aggFactories) { - Preconditions.checkArgument(!aggsFactoryMap.containsKey(aggFactory.getName()), "[%s] already defined", aggFactory.getName()); + Preconditions.checkArgument(!aggsFactoryMap.containsKey(aggFactory.getName()), + "[%s] already defined", aggFactory.getName()); aggsFactoryMap.put(aggFactory.getName(), aggFactory); } if (postAggs != null && !postAggs.isEmpty()) { final Set combinedAggNames = Sets.newHashSet(aggsFactoryMap.keySet()); + List decorated = Lists.newArrayListWithExpectedSize(postAggs.size()); for (final PostAggregator postAgg : postAggs) { final Set dependencies = postAgg.getDependentFields(); final Set missing = Sets.difference(dependencies, combinedAggNames); @@ -61,13 +70,14 @@ public static void prepareAggregations( missing.isEmpty(), "Missing fields [%s] for postAggregator [%s]", missing, postAgg.getName() ); - Preconditions.checkArgument(combinedAggNames.add(postAgg.getName()), "[%s] already defined", postAgg.getName()); + Preconditions.checkArgument(combinedAggNames.add(postAgg.getName()), + "[%s] already defined", postAgg.getName()); - if (postAgg instanceof HasDependentAggFactories) { - HasDependentAggFactories richPostAgg = (HasDependentAggFactories) postAgg; - richPostAgg.setDependentAggFactories(aggsFactoryMap); - } + decorated.add(postAgg.decorate(aggsFactoryMap)); } + return decorated; } + + return postAggs; } } diff --git a/processing/src/main/java/io/druid/query/aggregation/HasDependentAggFactories.java b/processing/src/main/java/io/druid/query/aggregation/HasDependentAggFactories.java deleted file mode 100644 index ba2ac3b337ed..000000000000 --- a/processing/src/main/java/io/druid/query/aggregation/HasDependentAggFactories.java +++ /dev/null @@ -1,28 +0,0 @@ -/* - * Licensed to Metamarkets Group Inc. (Metamarkets) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. Metamarkets licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package io.druid.query.aggregation; - -import java.util.Map; - -public interface HasDependentAggFactories -{ - void setDependentAggFactories(Map aggFactoryMap); - Map getDependentAggFactories(); -} diff --git a/processing/src/main/java/io/druid/query/aggregation/PostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/PostAggregator.java index d3037e9ec2e9..d1ce4ad113b7 100644 --- a/processing/src/main/java/io/druid/query/aggregation/PostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/PostAggregator.java @@ -35,4 +35,6 @@ public interface PostAggregator public Object compute(Map combinedAggregators); public String getName(); + + public PostAggregator decorate(Map aggregators); } diff --git a/processing/src/main/java/io/druid/query/aggregation/hyperloglog/HyperUniqueFinalizingPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/hyperloglog/HyperUniqueFinalizingPostAggregator.java index e84bb5ab5fcf..0bbbae1f7c97 100644 --- a/processing/src/main/java/io/druid/query/aggregation/hyperloglog/HyperUniqueFinalizingPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/hyperloglog/HyperUniqueFinalizingPostAggregator.java @@ -24,6 +24,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Ordering; import com.google.common.collect.Sets; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -84,6 +85,12 @@ public String getName() return name; } + @Override + public HyperUniqueFinalizingPostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty("fieldName") public String getFieldName() { diff --git a/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java index 33ddae4cf4d7..873dce4c117d 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java @@ -25,8 +25,8 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; import io.druid.java.util.common.IAE; +import io.druid.query.Queries; import io.druid.query.aggregation.AggregatorFactory; -import io.druid.query.aggregation.HasDependentAggFactories; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -37,7 +37,7 @@ /** */ -public class ArithmeticPostAggregator implements PostAggregator, HasDependentAggFactories +public class ArithmeticPostAggregator implements PostAggregator { public static final Comparator DEFAULT_COMPARATOR = new Comparator() { @@ -126,6 +126,12 @@ public String getName() return name; } + @Override + public ArithmeticPostAggregator decorate(Map aggregators) + { + return new ArithmeticPostAggregator(name, fnName, Queries.decorate(fields, aggregators), ordering); + } + @JsonProperty("fn") public String getFnName() { @@ -155,24 +161,6 @@ public String toString() '}'; } - @Override - public void setDependentAggFactories(Map aggFactoryMap) - { - this.aggFactoryMap = aggFactoryMap; - for (PostAggregator postAgg : fields) { - if (postAgg instanceof HasDependentAggFactories) { - HasDependentAggFactories richPostAgg = (HasDependentAggFactories) postAgg; - richPostAgg.setDependentAggFactories(this.aggFactoryMap); - } - } - } - - @Override - public Map getDependentAggFactories() - { - return aggFactoryMap; - } - private static enum Ops { PLUS("+") diff --git a/processing/src/main/java/io/druid/query/aggregation/post/ConstantPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/ConstantPostAggregator.java index d0a482c9bce7..1f6323a7b966 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/ConstantPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/ConstantPostAggregator.java @@ -23,6 +23,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; import com.google.common.collect.Sets; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -79,6 +80,12 @@ public String getName() return name; } + @Override + public ConstantPostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty("value") public Number getConstantValue() { diff --git a/processing/src/main/java/io/druid/query/aggregation/post/DoubleGreatestPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/DoubleGreatestPostAggregator.java index 8a238847d163..b9e25aab7d14 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/DoubleGreatestPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/DoubleGreatestPostAggregator.java @@ -23,6 +23,8 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; import com.google.common.collect.Sets; +import io.druid.query.Queries; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -98,6 +100,12 @@ public String getName() return name; } + @Override + public DoubleGreatestPostAggregator decorate(Map aggregators) + { + return new DoubleGreatestPostAggregator(name, Queries.decorate(fields, aggregators)); + } + @JsonProperty public List getFields() { diff --git a/processing/src/main/java/io/druid/query/aggregation/post/DoubleLeastPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/DoubleLeastPostAggregator.java index 88e4f765fed0..cd2308dd0bb5 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/DoubleLeastPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/DoubleLeastPostAggregator.java @@ -23,6 +23,8 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; import com.google.common.collect.Sets; +import io.druid.query.Queries; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -98,6 +100,12 @@ public String getName() return name; } + @Override + public DoubleLeastPostAggregator decorate(Map aggregators) + { + return new DoubleLeastPostAggregator(name, Queries.decorate(fields, aggregators)); + } + @JsonProperty public List getFields() { diff --git a/processing/src/main/java/io/druid/query/aggregation/post/ExpressionPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/ExpressionPostAggregator.java index b917869cd525..5fd394fd7694 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/ExpressionPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/ExpressionPostAggregator.java @@ -25,6 +25,7 @@ import com.google.common.collect.Sets; import io.druid.math.expr.Expr; import io.druid.math.expr.Parser; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -105,6 +106,12 @@ public String getName() return name; } + @Override + public ExpressionPostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty("expression") public String getExpression() { diff --git a/processing/src/main/java/io/druid/query/aggregation/post/FieldAccessPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/FieldAccessPostAggregator.java index 64b369366498..138b9b82ac7e 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/FieldAccessPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/FieldAccessPostAggregator.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.collect.Sets; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -70,6 +71,12 @@ public String getName() return name; } + @Override + public FieldAccessPostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public String getFieldName() { diff --git a/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java index 19ee014ad5cf..d7ef6f551e86 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java @@ -23,18 +23,16 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.collect.Sets; import io.druid.query.aggregation.AggregatorFactory; -import io.druid.query.aggregation.HasDependentAggFactories; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; import java.util.Map; import java.util.Set; -public class FinalizingFieldAccessPostAggregator implements PostAggregator, HasDependentAggFactories +public class FinalizingFieldAccessPostAggregator implements PostAggregator { private final String name; private final String fieldName; - private Map aggFactoryMap; @JsonCreator public FinalizingFieldAccessPostAggregator( @@ -61,13 +59,7 @@ public Comparator getComparator() @Override public Object compute(Map combinedAggregators) { - if (aggFactoryMap != null && aggFactoryMap.containsKey(fieldName)) { - return aggFactoryMap.get(fieldName).finalizeComputation( - combinedAggregators.get(fieldName) - ); - } else { - return combinedAggregators.get(fieldName); - } + throw new UnsupportedOperationException("No decorated"); } @Override @@ -77,6 +69,24 @@ public String getName() return name; } + @Override + public FinalizingFieldAccessPostAggregator decorate(final Map aggregators) + { + return new FinalizingFieldAccessPostAggregator(name, fieldName) { + @Override + public Object compute(Map combinedAggregators) + { + if (aggregators != null && aggregators.containsKey(fieldName)) { + return aggregators.get(fieldName).finalizeComputation( + combinedAggregators.get(fieldName) + ); + } else { + return combinedAggregators.get(fieldName); + } + } + }; + } + @JsonProperty public String getFieldName() { @@ -89,7 +99,6 @@ public String toString() return "FinalizingFieldAccessPostAggregator{" + "name'" + name + '\'' + ", fieldName='" + fieldName + '\'' + - ", aggFactoryMap='" + aggFactoryMap + '\'' + '}'; } @@ -111,9 +120,6 @@ public boolean equals(Object o) if (name != null ? !name.equals(that.name) : that.name != null) { return false; } - if (aggFactoryMap != null ? !aggFactoryMap.equals(that.aggFactoryMap) : that.aggFactoryMap != null) { - return false; - } return true; } @@ -123,19 +129,14 @@ public int hashCode() { int result = name != null ? name.hashCode() : 0; result = 31 * result + (fieldName != null ? fieldName.hashCode() : 0); - result = 31 * result + (aggFactoryMap != null ? aggFactoryMap.hashCode() : 0); return result; } - @Override - public void setDependentAggFactories(Map aggFactoryMap) - { - this.aggFactoryMap = aggFactoryMap; - } - - @Override - public Map getDependentAggFactories() + public static FinalizingFieldAccessPostAggregator buildDecorated(String name, + String fieldName, + Map aggregators) { - return aggFactoryMap; + FinalizingFieldAccessPostAggregator ret = new FinalizingFieldAccessPostAggregator(name, fieldName); + return ret.decorate(aggregators); } } diff --git a/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java index 866bb835b653..e775e527cd8d 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java @@ -26,6 +26,7 @@ import com.google.common.collect.Sets; import io.druid.java.util.common.ISE; import io.druid.js.JavaScriptConfig; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import org.mozilla.javascript.Context; import org.mozilla.javascript.ContextFactory; @@ -143,6 +144,12 @@ public String getName() return name; } + @Override + public JavaScriptPostAggregator decorate(Map aggregators) + { + return this; + } + @JsonProperty public List getFieldNames() { diff --git a/processing/src/main/java/io/druid/query/aggregation/post/LongGreatestPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/LongGreatestPostAggregator.java index 8f9e5cdf31be..bb58430aeff2 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/LongGreatestPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/LongGreatestPostAggregator.java @@ -24,6 +24,8 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Sets; import com.google.common.primitives.Longs; +import io.druid.query.Queries; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -99,6 +101,12 @@ public String getName() return name; } + @Override + public LongGreatestPostAggregator decorate(Map aggregators) + { + return new LongGreatestPostAggregator(name, Queries.decorate(fields, aggregators)); + } + @JsonProperty public List getFields() { diff --git a/processing/src/main/java/io/druid/query/aggregation/post/LongLeastPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/LongLeastPostAggregator.java index 80f4325857cd..70a91881c2dd 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/LongLeastPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/LongLeastPostAggregator.java @@ -24,6 +24,8 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Sets; import com.google.common.primitives.Longs; +import io.druid.query.Queries; +import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; import java.util.Comparator; @@ -99,6 +101,12 @@ public String getName() return name; } + @Override + public LongLeastPostAggregator decorate(Map aggregators) + { + return new LongLeastPostAggregator(name, Queries.decorate(fields, aggregators)); + } + @JsonProperty public List getFields() { diff --git a/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java b/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java index de3a6c419563..742f578fc919 100644 --- a/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java +++ b/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java @@ -120,12 +120,15 @@ public GroupByQuery( Preconditions.checkArgument(spec != null, "dimensions has null DimensionSpec"); } this.aggregatorSpecs = aggregatorSpecs == null ? ImmutableList.of() : aggregatorSpecs; - this.postAggregatorSpecs = postAggregatorSpecs == null ? ImmutableList.of() : postAggregatorSpecs; + this.postAggregatorSpecs = Queries.prepareAggregations( + this.aggregatorSpecs, + postAggregatorSpecs == null ? ImmutableList.of() : postAggregatorSpecs + ); this.havingSpec = havingSpec; this.limitSpec = (limitSpec == null) ? new NoopLimitSpec() : limitSpec; Preconditions.checkNotNull(this.granularity, "Must specify a granularity"); - Queries.prepareAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); + // Verify no duplicate names between dimensions, aggregators, and postAggregators. // They will all end up in the same namespace in the returned Rows and we can't have them clobbering each other. diff --git a/processing/src/main/java/io/druid/query/timeseries/TimeseriesQuery.java b/processing/src/main/java/io/druid/query/timeseries/TimeseriesQuery.java index 68689448b9c4..dbf29dcfaf49 100644 --- a/processing/src/main/java/io/druid/query/timeseries/TimeseriesQuery.java +++ b/processing/src/main/java/io/druid/query/timeseries/TimeseriesQuery.java @@ -63,9 +63,11 @@ public TimeseriesQuery( this.dimFilter = dimFilter; this.granularity = granularity; this.aggregatorSpecs = aggregatorSpecs == null ? ImmutableList.of() : aggregatorSpecs; - this.postAggregatorSpecs = postAggregatorSpecs == null ? ImmutableList.of() : postAggregatorSpecs; - - Queries.prepareAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); + this.postAggregatorSpecs = Queries.prepareAggregations(this.aggregatorSpecs, + postAggregatorSpecs == null + ? ImmutableList.of() + : postAggregatorSpecs + ); } @Override @@ -176,15 +178,15 @@ public TimeseriesQuery withDimFilter(DimFilter dimFilter) public String toString() { return "TimeseriesQuery{" + - "dataSource='" + getDataSource() + '\'' + - ", querySegmentSpec=" + getQuerySegmentSpec() + - ", descending=" + isDescending() + - ", dimFilter=" + dimFilter + - ", granularity='" + granularity + '\'' + - ", aggregatorSpecs=" + aggregatorSpecs + - ", postAggregatorSpecs=" + postAggregatorSpecs + - ", context=" + getContext() + - '}'; + "dataSource='" + getDataSource() + '\'' + + ", querySegmentSpec=" + getQuerySegmentSpec() + + ", descending=" + isDescending() + + ", dimFilter=" + dimFilter + + ", granularity='" + granularity + '\'' + + ", aggregatorSpecs=" + aggregatorSpecs + + ", postAggregatorSpecs=" + postAggregatorSpecs + + ", context=" + getContext() + + '}'; } @Override @@ -211,7 +213,9 @@ public boolean equals(Object o) if (granularity != null ? !granularity.equals(that.granularity) : that.granularity != null) { return false; } - if (postAggregatorSpecs != null ? !postAggregatorSpecs.equals(that.postAggregatorSpecs) : that.postAggregatorSpecs != null) { + if (postAggregatorSpecs != null + ? !postAggregatorSpecs.equals(that.postAggregatorSpecs) + : that.postAggregatorSpecs != null) { return false; } diff --git a/processing/src/main/java/io/druid/query/topn/TopNQuery.java b/processing/src/main/java/io/druid/query/topn/TopNQuery.java index 006c896053fa..9a75517852c5 100644 --- a/processing/src/main/java/io/druid/query/topn/TopNQuery.java +++ b/processing/src/main/java/io/druid/query/topn/TopNQuery.java @@ -74,15 +74,17 @@ public TopNQuery( this.dimFilter = dimFilter; this.granularity = granularity; this.aggregatorSpecs = aggregatorSpecs == null ? ImmutableList.of() : aggregatorSpecs; - this.postAggregatorSpecs = postAggregatorSpecs == null ? ImmutableList.of() : postAggregatorSpecs; + this.postAggregatorSpecs = Queries.prepareAggregations(this.aggregatorSpecs, + postAggregatorSpecs == null + ? ImmutableList.of() + : postAggregatorSpecs + ); Preconditions.checkNotNull(dimensionSpec, "dimensionSpec can't be null"); Preconditions.checkNotNull(topNMetricSpec, "must specify a metric"); Preconditions.checkArgument(threshold != 0, "Threshold cannot be equal to 0."); topNMetricSpec.verifyPreconditions(this.aggregatorSpecs, this.postAggregatorSpecs); - - Queries.prepareAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); } @Override @@ -316,7 +318,9 @@ public boolean equals(Object o) if (threshold != topNQuery.threshold) { return false; } - if (aggregatorSpecs != null ? !aggregatorSpecs.equals(topNQuery.aggregatorSpecs) : topNQuery.aggregatorSpecs != null) { + if (aggregatorSpecs != null + ? !aggregatorSpecs.equals(topNQuery.aggregatorSpecs) + : topNQuery.aggregatorSpecs != null) { return false; } if (dimFilter != null ? !dimFilter.equals(topNQuery.dimFilter) : topNQuery.dimFilter != null) { @@ -328,7 +332,9 @@ public boolean equals(Object o) if (granularity != null ? !granularity.equals(topNQuery.granularity) : topNQuery.granularity != null) { return false; } - if (postAggregatorSpecs != null ? !postAggregatorSpecs.equals(topNQuery.postAggregatorSpecs) : topNQuery.postAggregatorSpecs != null) { + if (postAggregatorSpecs != null + ? !postAggregatorSpecs.equals(topNQuery.postAggregatorSpecs) + : topNQuery.postAggregatorSpecs != null) { return false; } if (topNMetricSpec != null ? !topNMetricSpec.equals(topNQuery.topNMetricSpec) : topNQuery.topNMetricSpec != null) { diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java index a8d8342ca89b..8caec0061257 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java @@ -48,7 +48,7 @@ public class FinalizingFieldAccessPostAggregatorTest @Rule public final TemporaryFolder tempFoler = new TemporaryFolder(); - @Test + @Test(expected = UnsupportedOperationException.class) public void testComputeWithoutFinalizing() { Aggregator agg = new CountAggregator(); @@ -59,7 +59,7 @@ public void testComputeWithoutFinalizing() Map metricValues = Maps.newHashMap(); metricValues.put(agg.getName(), agg.get()); - PostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_rows", "rows"); + FinalizingFieldAccessPostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_rows", "rows"); Assert.assertEquals(new Long(3L), postAgg.compute(metricValues)); } @@ -72,8 +72,9 @@ public void testComputedWithFinalizing() .times(1); EasyMock.replay(aggFactory); - FinalizingFieldAccessPostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_billy", "billy"); - postAgg.setDependentAggFactories(ImmutableMap.of("billy", aggFactory)); + FinalizingFieldAccessPostAggregator postAgg = FinalizingFieldAccessPostAggregator.buildDecorated( + "final_billy", "billy", ImmutableMap.of("billy", aggFactory) + ); Map metricValues = Maps.newHashMap(); metricValues.put("billy", "test"); @@ -91,8 +92,9 @@ public void testComputedInArithmeticPostAggregator() .times(1); EasyMock.replay(aggFactory); - FinalizingFieldAccessPostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_billy", "billy"); - postAgg.setDependentAggFactories(ImmutableMap.of("billy", aggFactory)); + FinalizingFieldAccessPostAggregator postAgg = FinalizingFieldAccessPostAggregator.buildDecorated( + "final_billy", "billy", ImmutableMap.of("billy", aggFactory) + ); Map metricValues = Maps.newHashMap(); metricValues.put("billy", "test"); From 4e9aa9e24dffd25192ccaf54ad048374de0e84f8 Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Sat, 11 Feb 2017 01:05:28 +0900 Subject: [PATCH 11/13] Address comments. 1. Implements getComparator in FinalizingFieldAccessPostAggregator and add Uts for it 2. Some minor changes like renaming a method name. --- .../src/main/java/io/druid/query/Queries.java | 4 +- .../query/aggregation/PostAggregator.java | 7 ++ .../post/ArithmeticPostAggregator.java | 2 +- .../post/DoubleGreatestPostAggregator.java | 2 +- .../post/DoubleLeastPostAggregator.java | 2 +- .../FinalizingFieldAccessPostAggregator.java | 22 +++++-- .../post/LongGreatestPostAggregator.java | 2 +- .../post/LongLeastPostAggregator.java | 2 +- ...nalizingFieldAccessPostAggregatorTest.java | 65 +++++++++++++++++++ 9 files changed, 97 insertions(+), 11 deletions(-) diff --git a/processing/src/main/java/io/druid/query/Queries.java b/processing/src/main/java/io/druid/query/Queries.java index c48a876c1d3f..d62050965b42 100644 --- a/processing/src/main/java/io/druid/query/Queries.java +++ b/processing/src/main/java/io/druid/query/Queries.java @@ -34,8 +34,8 @@ */ public class Queries { - public static List decorate(List postAggs, - Map aggFactories) + public static List decoratePostAggregators(List postAggs, + Map aggFactories) { List decorated = Lists.newArrayListWithExpectedSize(postAggs.size()); for (PostAggregator aggregator : postAggs) { diff --git a/processing/src/main/java/io/druid/query/aggregation/PostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/PostAggregator.java index d1ce4ad113b7..e4af91bdd034 100644 --- a/processing/src/main/java/io/druid/query/aggregation/PostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/PostAggregator.java @@ -36,5 +36,12 @@ public interface PostAggregator public String getName(); + /** + * Returns a richer post aggregator which are built from the given aggregators with their names and some accessible + * environmental variables such as ones in the object scope. + * + * @param aggregators A map of aggregator factories with their names. + * + */ public PostAggregator decorate(Map aggregators); } diff --git a/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java index 873dce4c117d..25ab4456989d 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/ArithmeticPostAggregator.java @@ -129,7 +129,7 @@ public String getName() @Override public ArithmeticPostAggregator decorate(Map aggregators) { - return new ArithmeticPostAggregator(name, fnName, Queries.decorate(fields, aggregators), ordering); + return new ArithmeticPostAggregator(name, fnName, Queries.decoratePostAggregators(fields, aggregators), ordering); } @JsonProperty("fn") diff --git a/processing/src/main/java/io/druid/query/aggregation/post/DoubleGreatestPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/DoubleGreatestPostAggregator.java index b9e25aab7d14..7d4d0cb1e3c4 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/DoubleGreatestPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/DoubleGreatestPostAggregator.java @@ -103,7 +103,7 @@ public String getName() @Override public DoubleGreatestPostAggregator decorate(Map aggregators) { - return new DoubleGreatestPostAggregator(name, Queries.decorate(fields, aggregators)); + return new DoubleGreatestPostAggregator(name, Queries.decoratePostAggregators(fields, aggregators)); } @JsonProperty diff --git a/processing/src/main/java/io/druid/query/aggregation/post/DoubleLeastPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/DoubleLeastPostAggregator.java index cd2308dd0bb5..a49cc643135d 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/DoubleLeastPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/DoubleLeastPostAggregator.java @@ -103,7 +103,7 @@ public String getName() @Override public DoubleLeastPostAggregator decorate(Map aggregators) { - return new DoubleLeastPostAggregator(name, Queries.decorate(fields, aggregators)); + return new DoubleLeastPostAggregator(name, Queries.decoratePostAggregators(fields, aggregators)); } @JsonProperty diff --git a/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java index d7ef6f551e86..8c2fcd63f140 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregator.java @@ -21,6 +21,8 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.Ordering; import com.google.common.collect.Sets; import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.PostAggregator; @@ -73,6 +75,17 @@ public String getName() public FinalizingFieldAccessPostAggregator decorate(final Map aggregators) { return new FinalizingFieldAccessPostAggregator(name, fieldName) { + + @Override + public Comparator getComparator() + { + if (aggregators != null && aggregators.containsKey(fieldName)) { + return aggregators.get(fieldName).getComparator(); + } else { + return Ordering.natural().nullsFirst(); + } + } + @Override public Object compute(Map combinedAggregators) { @@ -97,7 +110,7 @@ public String getFieldName() public String toString() { return "FinalizingFieldAccessPostAggregator{" + - "name'" + name + '\'' + + "name='" + name + '\'' + ", fieldName='" + fieldName + '\'' + '}'; } @@ -132,9 +145,10 @@ public int hashCode() return result; } - public static FinalizingFieldAccessPostAggregator buildDecorated(String name, - String fieldName, - Map aggregators) + @VisibleForTesting + static FinalizingFieldAccessPostAggregator buildDecorated(String name, + String fieldName, + Map aggregators) { FinalizingFieldAccessPostAggregator ret = new FinalizingFieldAccessPostAggregator(name, fieldName); return ret.decorate(aggregators); diff --git a/processing/src/main/java/io/druid/query/aggregation/post/LongGreatestPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/LongGreatestPostAggregator.java index bb58430aeff2..b906911c4feb 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/LongGreatestPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/LongGreatestPostAggregator.java @@ -104,7 +104,7 @@ public String getName() @Override public LongGreatestPostAggregator decorate(Map aggregators) { - return new LongGreatestPostAggregator(name, Queries.decorate(fields, aggregators)); + return new LongGreatestPostAggregator(name, Queries.decoratePostAggregators(fields, aggregators)); } @JsonProperty diff --git a/processing/src/main/java/io/druid/query/aggregation/post/LongLeastPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/LongLeastPostAggregator.java index 70a91881c2dd..30932a706daa 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/LongLeastPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/LongLeastPostAggregator.java @@ -104,7 +104,7 @@ public String getName() @Override public LongLeastPostAggregator decorate(Map aggregators) { - return new LongLeastPostAggregator(name, Queries.decorate(fields, aggregators)); + return new LongLeastPostAggregator(name, Queries.decoratePostAggregators(fields, aggregators)); } @JsonProperty diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java index 8caec0061257..c519b9386904 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java @@ -19,9 +19,11 @@ package io.druid.query.aggregation.post; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Ordering; import io.druid.data.input.MapBasedRow; import io.druid.granularity.QueryGranularities; import io.druid.jackson.AggregatorsModule; @@ -40,6 +42,7 @@ import org.junit.rules.TemporaryFolder; import java.io.File; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -108,6 +111,68 @@ public void testComputedInArithmeticPostAggregator() EasyMock.verify(); } + @Test + public void testComparatorsWithFinalizing() throws Exception + { + AggregatorFactory aggFactory = EasyMock.createMock(AggregatorFactory.class); + EasyMock.expect(aggFactory.finalizeComputation("test_val1")) + .andReturn(new Long(10L)) + .times(1); + EasyMock.expect(aggFactory.finalizeComputation("test_val2")) + .andReturn(new Long(21)) + .times(1); + EasyMock.expect(aggFactory.finalizeComputation("test_val3")) + .andReturn(new Long(3)) + .times(1); + EasyMock.expect(aggFactory.finalizeComputation("test_val4")) + .andReturn(null) + .times(1); + EasyMock.expect(aggFactory.getComparator()) + .andReturn(Ordering.natural().nullsLast()) + .times(1); + EasyMock.replay(aggFactory); + + FinalizingFieldAccessPostAggregator postAgg = FinalizingFieldAccessPostAggregator.buildDecorated( + "final_billy", "billy", ImmutableMap.of("billy", aggFactory) + ); + + List computedValues = Lists.newArrayList(); + computedValues.add(postAgg.compute(ImmutableMap.of("billy", (Object)"test_val1"))); + computedValues.add(postAgg.compute(ImmutableMap.of("billy", (Object)"test_val2"))); + computedValues.add(postAgg.compute(ImmutableMap.of("billy", (Object)"test_val3"))); + computedValues.add(postAgg.compute(ImmutableMap.of("billy", (Object)"test_val4"))); + + Collections.sort(computedValues, postAgg.getComparator()); + Assert.assertArrayEquals(new Object[]{3L, 10L, 21L, null}, computedValues.toArray(new Object[]{})); + EasyMock.verify(); + } + + @Test + public void testComparatorsWithFinalizingAndComparatorNull() throws Exception + { + AggregatorFactory aggFactory = EasyMock.createMock(AggregatorFactory.class); + EasyMock.expect(aggFactory.getComparator()) + .andReturn(null) + .times(1); + EasyMock.replay(aggFactory); + + FinalizingFieldAccessPostAggregator postAgg = FinalizingFieldAccessPostAggregator.buildDecorated( + "final_billy", "joe", ImmutableMap.of("billy", aggFactory)); + + List computedValues = Lists.newArrayList(); + Map forNull = Maps.newHashMap(); + forNull.put("joe", (Object)null); // guava does not allow the value to be null. + computedValues.add(postAgg.compute(ImmutableMap.of("joe", (Object)"test_val1"))); + computedValues.add(postAgg.compute(ImmutableMap.of("joe", (Object)"test_val2"))); + computedValues.add(postAgg.compute(forNull)); + computedValues.add(postAgg.compute(ImmutableMap.of("joe", (Object)"test_val4"))); + Collections.sort(computedValues, postAgg.getComparator()); + + Assert.assertArrayEquals(new Object[]{null, "test_val1", "test_val2", "test_val4"}, computedValues.toArray(new Object[]{})); + + EasyMock.verify(); + } + @Test public void testIngestAndQueryWithArithmeticPostAggregator() throws Exception { From d773990823801347bb6e40ccb40677370df946f3 Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Sat, 11 Feb 2017 01:13:00 +0900 Subject: [PATCH 12/13] Fix a code style mismatch. --- .../post/FinalizingFieldAccessPostAggregatorTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java index c519b9386904..c4f63c0b189d 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java @@ -19,7 +19,6 @@ package io.druid.query.aggregation.post; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; From 2f755f685c254ca038f2e1a1dff917185965ec0e Mon Sep 17 00:00:00 2001 From: Jaehong Choi Date: Sat, 11 Feb 2017 01:30:19 +0900 Subject: [PATCH 13/13] Rebased to the master --- ...nalizingFieldAccessPostAggregatorTest.java | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java index c4f63c0b189d..d32170d6a338 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/FinalizingFieldAccessPostAggregatorTest.java @@ -53,21 +53,23 @@ public class FinalizingFieldAccessPostAggregatorTest @Test(expected = UnsupportedOperationException.class) public void testComputeWithoutFinalizing() { + String aggName = "rows"; Aggregator agg = new CountAggregator(); agg.aggregate(); agg.aggregate(); agg.aggregate(); Map metricValues = Maps.newHashMap(); - metricValues.put(agg.getName(), agg.get()); + metricValues.put(aggName, agg.get()); - FinalizingFieldAccessPostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_rows", "rows"); + FinalizingFieldAccessPostAggregator postAgg = new FinalizingFieldAccessPostAggregator("final_rows", aggName); Assert.assertEquals(new Long(3L), postAgg.compute(metricValues)); } @Test public void testComputedWithFinalizing() { + String aggName = "biily"; AggregatorFactory aggFactory = EasyMock.createMock(AggregatorFactory.class); EasyMock.expect(aggFactory.finalizeComputation("test")) .andReturn(new Long(3L)) @@ -75,11 +77,11 @@ public void testComputedWithFinalizing() EasyMock.replay(aggFactory); FinalizingFieldAccessPostAggregator postAgg = FinalizingFieldAccessPostAggregator.buildDecorated( - "final_billy", "billy", ImmutableMap.of("billy", aggFactory) + "final_billy", aggName, ImmutableMap.of(aggName, aggFactory) ); Map metricValues = Maps.newHashMap(); - metricValues.put("billy", "test"); + metricValues.put(aggName, "test"); Assert.assertEquals(new Long(3L), postAgg.compute(metricValues)); EasyMock.verify(aggFactory); @@ -88,6 +90,7 @@ public void testComputedWithFinalizing() @Test public void testComputedInArithmeticPostAggregator() { + String aggName = "billy"; AggregatorFactory aggFactory = EasyMock.createMock(AggregatorFactory.class); EasyMock.expect(aggFactory.finalizeComputation("test")) .andReturn(new Long(3L)) @@ -95,11 +98,11 @@ public void testComputedInArithmeticPostAggregator() EasyMock.replay(aggFactory); FinalizingFieldAccessPostAggregator postAgg = FinalizingFieldAccessPostAggregator.buildDecorated( - "final_billy", "billy", ImmutableMap.of("billy", aggFactory) + "final_billy", aggName, ImmutableMap.of(aggName, aggFactory) ); Map metricValues = Maps.newHashMap(); - metricValues.put("billy", "test"); + metricValues.put(aggName, "test"); List postAggsList = Lists.newArrayList( new ConstantPostAggregator("roku", 6), postAgg); @@ -113,6 +116,7 @@ public void testComputedInArithmeticPostAggregator() @Test public void testComparatorsWithFinalizing() throws Exception { + String aggName = "billy"; AggregatorFactory aggFactory = EasyMock.createMock(AggregatorFactory.class); EasyMock.expect(aggFactory.finalizeComputation("test_val1")) .andReturn(new Long(10L)) @@ -132,14 +136,14 @@ public void testComparatorsWithFinalizing() throws Exception EasyMock.replay(aggFactory); FinalizingFieldAccessPostAggregator postAgg = FinalizingFieldAccessPostAggregator.buildDecorated( - "final_billy", "billy", ImmutableMap.of("billy", aggFactory) + "final_billy", aggName, ImmutableMap.of(aggName, aggFactory) ); List computedValues = Lists.newArrayList(); - computedValues.add(postAgg.compute(ImmutableMap.of("billy", (Object)"test_val1"))); - computedValues.add(postAgg.compute(ImmutableMap.of("billy", (Object)"test_val2"))); - computedValues.add(postAgg.compute(ImmutableMap.of("billy", (Object)"test_val3"))); - computedValues.add(postAgg.compute(ImmutableMap.of("billy", (Object)"test_val4"))); + computedValues.add(postAgg.compute(ImmutableMap.of(aggName, (Object)"test_val1"))); + computedValues.add(postAgg.compute(ImmutableMap.of(aggName, (Object)"test_val2"))); + computedValues.add(postAgg.compute(ImmutableMap.of(aggName, (Object)"test_val3"))); + computedValues.add(postAgg.compute(ImmutableMap.of(aggName, (Object)"test_val4"))); Collections.sort(computedValues, postAgg.getComparator()); Assert.assertArrayEquals(new Object[]{3L, 10L, 21L, null}, computedValues.toArray(new Object[]{})); @@ -149,6 +153,7 @@ public void testComparatorsWithFinalizing() throws Exception @Test public void testComparatorsWithFinalizingAndComparatorNull() throws Exception { + String aggName = "billy"; AggregatorFactory aggFactory = EasyMock.createMock(AggregatorFactory.class); EasyMock.expect(aggFactory.getComparator()) .andReturn(null) @@ -156,7 +161,7 @@ public void testComparatorsWithFinalizingAndComparatorNull() throws Exception EasyMock.replay(aggFactory); FinalizingFieldAccessPostAggregator postAgg = FinalizingFieldAccessPostAggregator.buildDecorated( - "final_billy", "joe", ImmutableMap.of("billy", aggFactory)); + "final_billy", "joe", ImmutableMap.of(aggName, aggFactory)); List computedValues = Lists.newArrayList(); Map forNull = Maps.newHashMap();