diff --git a/extensions/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogramAggregatorFactory.java b/extensions/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogramAggregatorFactory.java index a2742c194ad..bef74feda6a 100644 --- a/extensions/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogramAggregatorFactory.java +++ b/extensions/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogramAggregatorFactory.java @@ -114,6 +114,21 @@ public class ApproximateHistogramAggregatorFactory implements AggregatorFactory return new ApproximateHistogramFoldingAggregatorFactory(name, name, resolution, numBuckets, lowerLimit, upperLimit); } + @Override + public List getRequiredColumns() + { + return Arrays.asList( + new ApproximateHistogramAggregatorFactory( + fieldName, + fieldName, + resolution, + numBuckets, + lowerLimit, + upperLimit + ) + ); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/AggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/AggregatorFactory.java index 0a9de24039a..0f90a0a8493 100644 --- a/processing/src/main/java/io/druid/query/aggregation/AggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/AggregatorFactory.java @@ -62,6 +62,13 @@ public interface AggregatorFactory */ public AggregatorFactory getCombiningFactory(); + /** + * Gets a list of all columns that this AggregatorFactory will scan + * + * @return AggregatorFactories for the columns to scan of the parent AggregatorFactory + */ + public List getRequiredColumns(); + /** * A method that knows how to "deserialize" the object from whatever form it might have been put into * in order to transfer via JSON. diff --git a/processing/src/main/java/io/druid/query/aggregation/CountAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/CountAggregatorFactory.java index 2ab435a8834..ae4243d5e4f 100644 --- a/processing/src/main/java/io/druid/query/aggregation/CountAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/CountAggregatorFactory.java @@ -75,6 +75,12 @@ public class CountAggregatorFactory implements AggregatorFactory return new LongSumAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new CountAggregatorFactory(name)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/DoubleMaxAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/DoubleMaxAggregatorFactory.java index 86ffa240000..74acf283b74 100644 --- a/processing/src/main/java/io/druid/query/aggregation/DoubleMaxAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/DoubleMaxAggregatorFactory.java @@ -81,6 +81,12 @@ public class DoubleMaxAggregatorFactory implements AggregatorFactory return new DoubleMaxAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new DoubleMaxAggregatorFactory(fieldName, fieldName)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/DoubleMinAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/DoubleMinAggregatorFactory.java index 3bb991c1d5e..a246a4ccfc8 100644 --- a/processing/src/main/java/io/druid/query/aggregation/DoubleMinAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/DoubleMinAggregatorFactory.java @@ -81,6 +81,12 @@ public class DoubleMinAggregatorFactory implements AggregatorFactory return new DoubleMinAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new DoubleMinAggregatorFactory(fieldName, fieldName)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/DoubleSumAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/DoubleSumAggregatorFactory.java index 12f504533bd..87f98b223b1 100644 --- a/processing/src/main/java/io/druid/query/aggregation/DoubleSumAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/DoubleSumAggregatorFactory.java @@ -84,6 +84,12 @@ public class DoubleSumAggregatorFactory implements AggregatorFactory return new DoubleSumAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new DoubleSumAggregatorFactory(fieldName, fieldName)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/FilteredAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/FilteredAggregatorFactory.java index 5805535b521..74cf62b5cfa 100644 --- a/processing/src/main/java/io/druid/query/aggregation/FilteredAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/FilteredAggregatorFactory.java @@ -153,6 +153,12 @@ public class FilteredAggregatorFactory implements AggregatorFactory return filter; } + @Override + public List getRequiredColumns() + { + return delegate.getRequiredColumns(); + } + @Override public String toString() { diff --git a/processing/src/main/java/io/druid/query/aggregation/HistogramAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/HistogramAggregatorFactory.java index dc4b444cddd..9f3c4ccfecf 100644 --- a/processing/src/main/java/io/druid/query/aggregation/HistogramAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/HistogramAggregatorFactory.java @@ -98,6 +98,12 @@ public class HistogramAggregatorFactory implements AggregatorFactory return new HistogramAggregatorFactory(name, name, breaksList); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new HistogramAggregatorFactory(fieldName, fieldName, breaksList)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java index 76ce9ba57cc..bbc4506ec3c 100644 --- a/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -135,6 +135,22 @@ public class JavaScriptAggregatorFactory implements AggregatorFactory return new JavaScriptAggregatorFactory(name, Lists.newArrayList(name), fnCombine, fnReset, fnCombine); } + @Override + public List getRequiredColumns() + { + return Lists.transform( + fieldNames, + new com.google.common.base.Function() + { + @Override + public AggregatorFactory apply(String input) + { + return new JavaScriptAggregatorFactory(input, fieldNames, fnAggregate, fnReset, fnCombine); + } + } + ); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/LongMaxAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/LongMaxAggregatorFactory.java index 6d96fff3a8b..c71baf9a3dc 100644 --- a/processing/src/main/java/io/druid/query/aggregation/LongMaxAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/LongMaxAggregatorFactory.java @@ -83,6 +83,12 @@ public class LongMaxAggregatorFactory implements AggregatorFactory return new LongMaxAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new LongMaxAggregatorFactory(fieldName, fieldName)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/LongMinAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/LongMinAggregatorFactory.java index 85e1c006e25..7f7eca7989f 100644 --- a/processing/src/main/java/io/druid/query/aggregation/LongMinAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/LongMinAggregatorFactory.java @@ -83,6 +83,12 @@ public class LongMinAggregatorFactory implements AggregatorFactory return new LongMinAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new LongMinAggregatorFactory(fieldName, fieldName)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/LongSumAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/LongSumAggregatorFactory.java index d6d27ef870f..48a598d9cf6 100644 --- a/processing/src/main/java/io/druid/query/aggregation/LongSumAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/LongSumAggregatorFactory.java @@ -84,6 +84,12 @@ public class LongSumAggregatorFactory implements AggregatorFactory return new LongSumAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new LongSumAggregatorFactory(fieldName, fieldName)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorFactory.java index b2594b48a8c..289120cf976 100644 --- a/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorFactory.java @@ -144,6 +144,22 @@ public class CardinalityAggregatorFactory implements AggregatorFactory return new HyperUniquesAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Lists.transform( + fieldNames, + new Function() + { + @Override + public AggregatorFactory apply(String input) + { + return new CardinalityAggregatorFactory(input, fieldNames, byRow); + } + } + ); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java index 8962f821a35..cbf64c879d5 100644 --- a/processing/src/main/java/io/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/hyperloglog/HyperUniquesAggregatorFactory.java @@ -137,6 +137,12 @@ public class HyperUniquesAggregatorFactory implements AggregatorFactory return new HyperUniquesAggregatorFactory(name, name); } + @Override + public List getRequiredColumns() + { + return Arrays.asList(new HyperUniquesAggregatorFactory(fieldName, fieldName)); + } + @Override public Object deserialize(Object object) { diff --git a/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java index fe7a4cd1b98..8b6c0538f5d 100644 --- a/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -30,7 +30,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Ordering; -import com.google.common.collect.Sets; import com.google.inject.Inject; import com.metamx.common.ISE; import com.metamx.common.Pair; @@ -57,7 +56,6 @@ import io.druid.query.QueryRunner; import io.druid.query.QueryToolChest; import io.druid.query.SubqueryQueryRunner; import io.druid.query.aggregation.AggregatorFactory; -import io.druid.query.aggregation.DoubleSumAggregatorFactory; import io.druid.query.aggregation.MetricManipulationFn; import io.druid.query.aggregation.PostAggregator; import io.druid.query.dimension.DefaultDimensionSpec; @@ -160,36 +158,12 @@ public class GroupByQueryQueryToolChest extends QueryToolChest subqueryResult = mergeGroupByResults(subquery, runner, context); - - // check that all fieldName parameters in the outer query match up with a name parameter in the inner query - // for an aggregator or a post aggregator - Set innerFieldNames = Sets.newHashSet(); - for (AggregatorFactory innerAggregator : subquery.getAggregatorSpecs()) { - innerFieldNames.add(innerAggregator.getName()); - } - for (PostAggregator innerPostAggregator : subquery.getPostAggregatorSpecs()) { - innerFieldNames.add(innerPostAggregator.getName()); - } - - for (AggregatorFactory outerAggregator : query.getAggregatorSpecs()) { - for (final String fieldName : outerAggregator.requiredFields()) { - if (!innerFieldNames.contains(fieldName)) { - throw new IllegalArgumentException( - String.format("Subquery must have an aggregator or post aggregator with name '%s'", fieldName) - ); - } - } + final List aggs = Lists.newArrayList(); + for (AggregatorFactory aggregatorFactory : query.getAggregatorSpecs()) { + aggs.addAll(aggregatorFactory.getRequiredColumns()); } // We need the inner incremental index to have all the columns required by the outer query - final List aggs = Lists.newArrayList(subquery.getAggregatorSpecs()); - for (PostAggregator postAgg : subquery.getPostAggregatorSpecs()) { - // This causes the post aggregators from the inner query to be copied to the incremental index so that they are - // available as columns for the outer query. The data isn't modified by the aggregator since it has already - // been fully grouped by the inner query. Somewhat of a hack to get this working with an incremental index. - aggs.add(new DoubleSumAggregatorFactory(postAgg.getName(), postAgg.getName())); - } - final GroupByQuery innerQuery = new GroupByQuery.Builder(subquery) .setAggregatorSpecs(aggs) .setInterval(subquery.getIntervals()) diff --git a/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java index 97ad17488b7..ae22708742f 100644 --- a/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java @@ -51,7 +51,6 @@ import io.druid.query.aggregation.FilteredAggregatorFactory; import io.druid.query.aggregation.JavaScriptAggregatorFactory; import io.druid.query.aggregation.LongSumAggregatorFactory; import io.druid.query.aggregation.PostAggregator; -import io.druid.query.aggregation.cardinality.CardinalityAggregatorFactory; import io.druid.query.aggregation.hyperloglog.HyperUniqueFinalizingPostAggregator; import io.druid.query.aggregation.hyperloglog.HyperUniquesAggregatorFactory; import io.druid.query.aggregation.post.ArithmeticPostAggregator; @@ -2437,68 +2436,6 @@ public class GroupByQueryRunnerTest TestHelper.assertExpectedObjects(expectedResults, results, ""); } - @Test - public void testDifferentGroupingSubqueryMultipleAggregatorsOnSameField() - { - GroupByQuery subquery = GroupByQuery - .builder() - .setDataSource(QueryRunnerTestHelper.dataSource) - .setQuerySegmentSpec(QueryRunnerTestHelper.firstToThird) - .setDimensions(Lists.newArrayList(new DefaultDimensionSpec("quality", "alias"))) - .setAggregatorSpecs( - Arrays.asList( - QueryRunnerTestHelper.rowsCount, - new LongSumAggregatorFactory("idx", "index") - ) - ) - .setPostAggregatorSpecs( - Lists.newArrayList( - new ArithmeticPostAggregator( - "post_agg", - "+", - Lists.newArrayList( - new FieldAccessPostAggregator("idx", "idx"), - new FieldAccessPostAggregator("idx", "idx") - ) - ), - new ArithmeticPostAggregator( - "post_agg2", - "quotient", - Lists.newArrayList( - new FieldAccessPostAggregator("idx", "idx"), - new ConstantPostAggregator("constant", 1.23) - ) - ) - ) - ) - .setGranularity(QueryRunnerTestHelper.dayGran) - .build(); - - GroupByQuery query = GroupByQuery - .builder() - .setDataSource(subquery) - .setQuerySegmentSpec(QueryRunnerTestHelper.firstToThird) - .setAggregatorSpecs( - Arrays.asList( - new DoubleMaxAggregatorFactory("idx1", "idx"), - new DoubleMaxAggregatorFactory("idx2", "idx"), - new DoubleMaxAggregatorFactory("idx3", "post_agg"), - new DoubleMaxAggregatorFactory("idx4", "post_agg2") - ) - ) - .setGranularity(QueryRunnerTestHelper.dayGran) - .build(); - - List expectedResults = Arrays.asList( - GroupByQueryRunnerTestHelper.createExpectedRow("2011-04-01", "idx1", 2900.0, "idx2", 2900.0, - "idx3", 5800.0, "idx4", 2357.7236328125), - GroupByQueryRunnerTestHelper.createExpectedRow("2011-04-02", "idx1", 2505.0, "idx2", 2505.0, - "idx3", 5010.0, "idx4", 2036.5853271484375) - ); - - Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); - TestHelper.assertExpectedObjects(expectedResults, results, ""); - } @Test public void testDifferentGroupingSubqueryWithFilter()