From 436828b67cc53ad9382314c4e74f3a7dea572975 Mon Sep 17 00:00:00 2001 From: Fangjin Yang Date: Wed, 12 Dec 2012 10:36:46 -0800 Subject: [PATCH] post aggs verify that they have all the fields they need --- .../java/com/metamx/druid/query/Queries.java | 50 +++++ .../query/timeseries/TimeseriesQuery.java | 4 +- .../com/metamx/druid/query/QueriesTest.java | 191 ++++++++++++++++++ .../post/ArithmeticPostAggregator.java | 11 + .../post/ConstantPostAggregator.java | 7 + .../post/FieldAccessPostAggregator.java | 13 +- .../aggregation/post/PostAggregator.java | 3 + 7 files changed, 272 insertions(+), 7 deletions(-) create mode 100644 client/src/main/java/com/metamx/druid/query/Queries.java create mode 100644 client/src/test/java/com/metamx/druid/query/QueriesTest.java diff --git a/client/src/main/java/com/metamx/druid/query/Queries.java b/client/src/main/java/com/metamx/druid/query/Queries.java new file mode 100644 index 00000000000..2fb17b2fe76 --- /dev/null +++ b/client/src/main/java/com/metamx/druid/query/Queries.java @@ -0,0 +1,50 @@ +package com.metamx.druid.query; + +import com.google.common.base.Function; +import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; +import com.google.common.collect.Sets; +import com.metamx.druid.aggregation.AggregatorFactory; +import com.metamx.druid.aggregation.post.PostAggregator; + +import javax.annotation.Nullable; +import java.util.List; +import java.util.Set; + +/** + */ +public class Queries +{ + public static void verifyAggregations( + List aggFactories, + List postAggs + ) + { + Preconditions.checkNotNull(aggFactories, "aggregations cannot be null"); + Preconditions.checkArgument(aggFactories.size() > 0, "Must have at least one AggregatorFactory"); + + if (postAggs != null && !postAggs.isEmpty()) { + Set combinedAggNames = Sets.newHashSet( + Lists.transform( + aggFactories, + new Function() + { + @Override + public String apply(@Nullable AggregatorFactory input) + { + return input.getName(); + } + } + ) + ); + + for (PostAggregator postAgg : postAggs) { + Preconditions.checkArgument( + postAgg.verifyFields(combinedAggNames), + String.format("Missing field[%s]", postAgg.getName()) + ); + combinedAggNames.add(postAgg.getName()); + } + } + } +} diff --git a/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQuery.java b/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQuery.java index 3c664402df3..490e2531edd 100644 --- a/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQuery.java +++ b/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQuery.java @@ -26,6 +26,7 @@ import com.metamx.druid.Query; import com.metamx.druid.QueryGranularity; import com.metamx.druid.aggregation.AggregatorFactory; import com.metamx.druid.aggregation.post.PostAggregator; +import com.metamx.druid.query.Queries; import com.metamx.druid.query.filter.DimFilter; import com.metamx.druid.query.segment.QuerySegmentSpec; import com.metamx.druid.result.Result; @@ -67,8 +68,7 @@ public class TimeseriesQuery extends BaseQuery> this.aggregatorSpecs = aggregatorSpecs; this.postAggregatorSpecs = postAggregatorSpecs == null ? ImmutableList.of() : postAggregatorSpecs; - Preconditions.checkNotNull(aggregatorSpecs, "aggregations cannot be null"); - Preconditions.checkArgument(aggregatorSpecs.size() > 0, "Must have at least one AggregatorFactory"); + Queries.verifyAggregations(this.aggregatorSpecs, this.postAggregatorSpecs); } @Override diff --git a/client/src/test/java/com/metamx/druid/query/QueriesTest.java b/client/src/test/java/com/metamx/druid/query/QueriesTest.java new file mode 100644 index 00000000000..46af9682826 --- /dev/null +++ b/client/src/test/java/com/metamx/druid/query/QueriesTest.java @@ -0,0 +1,191 @@ +package com.metamx.druid.query; + +import com.metamx.druid.aggregation.AggregatorFactory; +import com.metamx.druid.aggregation.CountAggregatorFactory; +import com.metamx.druid.aggregation.DoubleSumAggregatorFactory; +import com.metamx.druid.aggregation.post.ArithmeticPostAggregator; +import com.metamx.druid.aggregation.post.ConstantPostAggregator; +import com.metamx.druid.aggregation.post.FieldAccessPostAggregator; +import com.metamx.druid.aggregation.post.PostAggregator; +import junit.framework.Assert; +import org.junit.Test; + +import java.util.Arrays; +import java.util.List; + +/** + */ +public class QueriesTest +{ + @Test + public void testVerifyAggregations() throws Exception + { + List aggFactories = Arrays.asList( + new CountAggregatorFactory("count"), + new DoubleSumAggregatorFactory("idx", "index"), + new DoubleSumAggregatorFactory("rev", "revenue") + ); + + List postAggs = Arrays.asList( + new ArithmeticPostAggregator( + "addStuff", + "+", + Arrays.asList( + new FieldAccessPostAggregator("idx", "idx"), + new FieldAccessPostAggregator("count", "count") + ) + ) + ); + + boolean exceptionOccured = false; + + try { + Queries.verifyAggregations(aggFactories, postAggs); + } + catch (Exception e) { + exceptionOccured = true; + } + + Assert.assertFalse(exceptionOccured); + } + + @Test + public void testVerifyAggregationsMissingVal() throws Exception + { + List aggFactories = Arrays.asList( + new CountAggregatorFactory("count"), + new DoubleSumAggregatorFactory("idx", "index"), + new DoubleSumAggregatorFactory("rev", "revenue") + ); + + List postAggs = Arrays.asList( + new ArithmeticPostAggregator( + "addStuff", + "+", + Arrays.asList( + new FieldAccessPostAggregator("idx", "idx2"), + new FieldAccessPostAggregator("count", "count") + ) + ) + ); + + boolean exceptionOccured = false; + + try { + Queries.verifyAggregations(aggFactories, postAggs); + } + catch (Exception e) { + exceptionOccured = true; + } + + Assert.assertTrue(exceptionOccured); + } + + @Test + public void testVerifyAggregationsMultiLevel() throws Exception + { + List aggFactories = Arrays.asList( + new CountAggregatorFactory("count"), + new DoubleSumAggregatorFactory("idx", "index"), + new DoubleSumAggregatorFactory("rev", "revenue") + ); + + List postAggs = Arrays.asList( + new ArithmeticPostAggregator( + "divideStuff", + "/", + Arrays.asList( + new ArithmeticPostAggregator( + "addStuff", + "+", + Arrays.asList( + new FieldAccessPostAggregator("idx", "idx"), + new ConstantPostAggregator("const", 1) + ) + ), + new ArithmeticPostAggregator( + "subtractStuff", + "-", + Arrays.asList( + new FieldAccessPostAggregator("rev", "rev"), + new ConstantPostAggregator("const", 1) + ) + ) + ) + ), + new ArithmeticPostAggregator( + "addStuff", + "+", + Arrays.asList( + new FieldAccessPostAggregator("divideStuff", "divideStuff"), + new FieldAccessPostAggregator("count", "count") + ) + ) + ); + + boolean exceptionOccured = false; + + try { + Queries.verifyAggregations(aggFactories, postAggs); + } + catch (Exception e) { + exceptionOccured = true; + } + + Assert.assertFalse(exceptionOccured); + } + + @Test + public void testVerifyAggregationsMultiLevelMissingVal() throws Exception + { + List aggFactories = Arrays.asList( + new CountAggregatorFactory("count"), + new DoubleSumAggregatorFactory("idx", "index"), + new DoubleSumAggregatorFactory("rev", "revenue") + ); + + List postAggs = Arrays.asList( + new ArithmeticPostAggregator( + "divideStuff", + "/", + Arrays.asList( + new ArithmeticPostAggregator( + "addStuff", + "+", + Arrays.asList( + new FieldAccessPostAggregator("idx", "idx"), + new ConstantPostAggregator("const", 1) + ) + ), + new ArithmeticPostAggregator( + "subtractStuff", + "-", + Arrays.asList( + new FieldAccessPostAggregator("rev", "rev2"), + new ConstantPostAggregator("const", 1) + ) + ) + ) + ), + new ArithmeticPostAggregator( + "addStuff", + "+", + Arrays.asList( + new FieldAccessPostAggregator("divideStuff", "divideStuff"), + new FieldAccessPostAggregator("count", "count") + ) + ) + ); + + boolean exceptionOccured = false; + + try { + Queries.verifyAggregations(aggFactories, postAggs); + } + catch (Exception e) { + exceptionOccured = true; + } + + Assert.assertTrue(exceptionOccured); + } +} diff --git a/common/src/main/java/com/metamx/druid/aggregation/post/ArithmeticPostAggregator.java b/common/src/main/java/com/metamx/druid/aggregation/post/ArithmeticPostAggregator.java index bffc0f2d7bb..035d0fa6652 100644 --- a/common/src/main/java/com/metamx/druid/aggregation/post/ArithmeticPostAggregator.java +++ b/common/src/main/java/com/metamx/druid/aggregation/post/ArithmeticPostAggregator.java @@ -68,6 +68,17 @@ public class ArithmeticPostAggregator implements PostAggregator } } + @Override + public boolean verifyFields(Set fieldNames) + { + for (PostAggregator field : fields) { + if (!field.verifyFields(fieldNames)) { + return false; + } + } + return true; + } + @Override public Comparator getComparator() { diff --git a/common/src/main/java/com/metamx/druid/aggregation/post/ConstantPostAggregator.java b/common/src/main/java/com/metamx/druid/aggregation/post/ConstantPostAggregator.java index eef818db49f..645de6c9b80 100644 --- a/common/src/main/java/com/metamx/druid/aggregation/post/ConstantPostAggregator.java +++ b/common/src/main/java/com/metamx/druid/aggregation/post/ConstantPostAggregator.java @@ -24,6 +24,7 @@ import org.codehaus.jackson.annotate.JsonProperty; import java.util.Comparator; import java.util.Map; +import java.util.Set; /** */ @@ -42,6 +43,12 @@ public class ConstantPostAggregator implements PostAggregator this.constantValue = constantValue; } + @Override + public boolean verifyFields(Set fields) + { + return true; + } + @Override public Comparator getComparator() { diff --git a/common/src/main/java/com/metamx/druid/aggregation/post/FieldAccessPostAggregator.java b/common/src/main/java/com/metamx/druid/aggregation/post/FieldAccessPostAggregator.java index 42c031db01c..0a1c866d044 100644 --- a/common/src/main/java/com/metamx/druid/aggregation/post/FieldAccessPostAggregator.java +++ b/common/src/main/java/com/metamx/druid/aggregation/post/FieldAccessPostAggregator.java @@ -25,6 +25,7 @@ import org.codehaus.jackson.annotate.JsonProperty; import java.util.Comparator; import java.util.Map; +import java.util.Set; /** */ @@ -43,6 +44,12 @@ public class FieldAccessPostAggregator implements PostAggregator this.fieldName = fieldName; } + @Override + public boolean verifyFields(Set fieldNames) + { + return fieldNames.contains(fieldName); + } + @Override public Comparator getComparator() { @@ -52,11 +59,7 @@ public class FieldAccessPostAggregator implements PostAggregator @Override public Object compute(Map combinedAggregators) { - Object retVal = combinedAggregators.get(fieldName); - if (retVal == null) { - throw new ISE("Mismatch! Agg[%s] was not specified!", fieldName); - } - return retVal; + return combinedAggregators.get(fieldName); } @Override diff --git a/common/src/main/java/com/metamx/druid/aggregation/post/PostAggregator.java b/common/src/main/java/com/metamx/druid/aggregation/post/PostAggregator.java index 5e679d64868..487ac30efb3 100644 --- a/common/src/main/java/com/metamx/druid/aggregation/post/PostAggregator.java +++ b/common/src/main/java/com/metamx/druid/aggregation/post/PostAggregator.java @@ -24,6 +24,7 @@ import org.codehaus.jackson.annotate.JsonTypeInfo; import java.util.Comparator; import java.util.Map; +import java.util.Set; /** * Functionally similar to an Aggregator. See the Aggregator interface for more comments. @@ -36,6 +37,8 @@ import java.util.Map; }) public interface PostAggregator { + public boolean verifyFields(Set fieldNames); + public Comparator getComparator(); public Object compute(Map combinedAggregators);