diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java index 7a3d3fee8ac4a..359c8dd571e25 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java @@ -28,7 +28,7 @@ import org.elasticsearch.search.internal.SearchContext; import java.io.IOException; -import java.util.List; +import java.util.Collection; import java.util.Map; /** @@ -79,12 +79,12 @@ public String getName() { public abstract AggregationBuilder subAggregation(PipelineAggregationBuilder aggregation); /** Return the configured set of subaggregations **/ - public List getSubAggregations() { + public Collection getSubAggregations() { return factoriesBuilder.getAggregatorFactories(); } /** Return the configured set of pipeline aggregations **/ - public List getPipelineAggregations() { + public Collection getPipelineAggregations() { return factoriesBuilder.getPipelineAggregatorFactories(); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java index 9198582411fb3..5bfb575bee892 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -38,9 +38,11 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -237,8 +239,11 @@ public int countPipelineAggregators() { public static class Builder implements Writeable, ToXContentObject { private final Set names = new HashSet<>(); - private final List aggregationBuilders = new ArrayList<>(); - private final List pipelineAggregatorBuilders = new ArrayList<>(); + + // Using LinkedHashSets to preserve the order of insertion, that makes the results + // ordered nicely, although technically order does not matter + private final Collection aggregationBuilders = new LinkedHashSet<>(); + private final Collection pipelineAggregatorBuilders = new LinkedHashSet<>(); private boolean skipResolveOrder; /** @@ -322,29 +327,32 @@ public AggregatorFactories build(SearchContext context, AggregatorFactory par parent); } AggregatorFactory[] aggFactories = new AggregatorFactory[aggregationBuilders.size()]; - for (int i = 0; i < aggregationBuilders.size(); i++) { - aggFactories[i] = aggregationBuilders.get(i).build(context, parent); + + int i = 0; + for (AggregationBuilder agg : aggregationBuilders) { + aggFactories[i] = agg.build(context, parent); + ++i; } return new AggregatorFactories(aggFactories, orderedpipelineAggregators); } private List resolvePipelineAggregatorOrder( - List pipelineAggregatorBuilders, List aggBuilders, + Collection pipelineAggregatorBuilders, Collection aggregationBuilders, AggregatorFactory parent) { Map pipelineAggregatorBuildersMap = new HashMap<>(); for (PipelineAggregationBuilder builder : pipelineAggregatorBuilders) { pipelineAggregatorBuildersMap.put(builder.getName(), builder); } Map aggBuildersMap = new HashMap<>(); - for (AggregationBuilder aggBuilder : aggBuilders) { + for (AggregationBuilder aggBuilder : aggregationBuilders) { aggBuildersMap.put(aggBuilder.name, aggBuilder); } List orderedPipelineAggregatorrs = new LinkedList<>(); List unmarkedBuilders = new ArrayList<>(pipelineAggregatorBuilders); - Set temporarilyMarked = new HashSet<>(); + Collection temporarilyMarked = new HashSet<>(); while (!unmarkedBuilders.isEmpty()) { PipelineAggregationBuilder builder = unmarkedBuilders.get(0); - builder.validate(parent, aggBuilders, pipelineAggregatorBuilders); + builder.validate(parent, aggregationBuilders, pipelineAggregatorBuilders); resolvePipelineAggregatorOrder(aggBuildersMap, pipelineAggregatorBuildersMap, orderedPipelineAggregatorrs, unmarkedBuilders, temporarilyMarked, builder); } @@ -354,7 +362,7 @@ private List resolvePipelineAggregatorOrder( private void resolvePipelineAggregatorOrder(Map aggBuildersMap, Map pipelineAggregatorBuildersMap, List orderedPipelineAggregators, List unmarkedBuilders, - Set temporarilyMarked, PipelineAggregationBuilder builder) { + Collection temporarilyMarked, PipelineAggregationBuilder builder) { if (temporarilyMarked.contains(builder)) { throw new IllegalArgumentException("Cyclical dependency found with pipeline aggregator [" + builder.getName() + "]"); } else if (unmarkedBuilders.contains(builder)) { @@ -375,7 +383,7 @@ private void resolvePipelineAggregatorOrder(Map aggB } else { // Check the non-pipeline sub-aggregator // factories - List subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders; + Collection subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders; boolean foundSubBuilder = false; for (AggregationBuilder subBuilder : subBuilders) { if (aggName.equals(subBuilder.name)) { @@ -386,7 +394,7 @@ private void resolvePipelineAggregatorOrder(Map aggB } // Check the pipeline sub-aggregator factories if (!foundSubBuilder && (i == bucketsPathElements.size() - 1)) { - List subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders; + Collection subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders; for (PipelineAggregationBuilder subFactory : subPipelineBuilders) { if (aggName.equals(subFactory.getName())) { foundSubBuilder = true; @@ -417,12 +425,12 @@ private void resolvePipelineAggregatorOrder(Map aggB } } - public List getAggregatorFactories() { - return Collections.unmodifiableList(aggregationBuilders); + public Collection getAggregatorFactories() { + return Collections.unmodifiableCollection(aggregationBuilders); } - public List getPipelineAggregatorFactories() { - return Collections.unmodifiableList(pipelineAggregatorBuilders); + public Collection getPipelineAggregatorFactories() { + return Collections.unmodifiableCollection(pipelineAggregatorBuilders); } public int count() { @@ -463,6 +471,7 @@ public boolean equals(Object obj) { if (getClass() != obj.getClass()) return false; Builder other = (Builder) obj; + if (!Objects.equals(aggregationBuilders, other.aggregationBuilders)) return false; if (!Objects.equals(pipelineAggregatorBuilders, other.pipelineAggregatorBuilders)) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java index 1b751d8c6847d..8b66738848fda 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java @@ -25,7 +25,7 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; -import java.util.List; +import java.util.Collection; import java.util.Map; /** @@ -68,8 +68,8 @@ public final String[] getBucketsPaths() { * Internal: Validates the state of this factory (makes sure the factory is properly * configured) */ - protected abstract void validate(AggregatorFactory parent, List factories, - List pipelineAggregatorFactories); + protected abstract void validate(AggregatorFactory parent, Collection aggregationBuilders, + Collection pipelineAggregatorBuilders); /** * Creates the pipeline aggregator diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java index 33c68aff26d56..cbe9cc2b89589 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java @@ -28,7 +28,7 @@ import java.io.IOException; import java.util.Arrays; -import java.util.List; +import java.util.Collection; import java.util.Map; import java.util.Objects; @@ -81,8 +81,8 @@ public String type() { * configured) */ @Override - public final void validate(AggregatorFactory parent, List factories, - List pipelineAggregatorFactories) { + public final void validate(AggregatorFactory parent, Collection factories, + Collection pipelineAggregatorFactories) { doValidate(parent, factories, pipelineAggregatorFactories); } @@ -99,8 +99,8 @@ public final PipelineAggregator create() throws IOException { return aggregator; } - public void doValidate(AggregatorFactory parent, List factories, - List pipelineAggregatorFactories) { + public void doValidate(AggregatorFactory parent, Collection factories, + Collection pipelineAggregatorFactories) { } @SuppressWarnings("unchecked") diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregationBuilder.java index 56db4310c9497..c77922eff2a5e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregationBuilder.java @@ -32,7 +32,7 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; -import java.util.List; +import java.util.Collection; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -109,8 +109,8 @@ public GapPolicy gapPolicy() { protected abstract PipelineAggregator createInternal(Map metaData) throws IOException; @Override - public void doValidate(AggregatorFactory parent, List aggBuilders, - List pipelineAggregatorFactories) { + public void doValidate(AggregatorFactory parent, Collection aggBuilders, + Collection pipelineAggregatorFactories) { if (bucketsPaths.length != 1) { throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must contain a single entry for aggregation [" + name + "]"); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/percentile/PercentilesBucketPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/percentile/PercentilesBucketPipelineAggregationBuilder.java index 0870ef0e18725..56dd0d3e786fc 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/percentile/PercentilesBucketPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/percentile/PercentilesBucketPipelineAggregationBuilder.java @@ -35,7 +35,7 @@ import java.io.IOException; import java.util.Arrays; -import java.util.List; +import java.util.Collection; import java.util.Map; import java.util.Objects; @@ -95,8 +95,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, List aggFactories, - List pipelineAggregatorFactories) { + public void doValidate(AggregatorFactory parent, Collection aggFactories, + Collection pipelineAggregatorFactories) { super.doValidate(parent, aggFactories, pipelineAggregatorFactories); for (Double p : percents) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/stats/extended/ExtendedStatsBucketPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/stats/extended/ExtendedStatsBucketPipelineAggregationBuilder.java index 00db3fabaa69d..84dcb03fbe9a7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/stats/extended/ExtendedStatsBucketPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/stats/extended/ExtendedStatsBucketPipelineAggregationBuilder.java @@ -29,7 +29,7 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.BucketMetricsPipelineAggregationBuilder; import java.io.IOException; -import java.util.List; +import java.util.Collection; import java.util.Map; import java.util.Objects; @@ -82,8 +82,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, List aggBuilders, - List pipelineAggregatorFactories) { + public void doValidate(AggregatorFactory parent, Collection aggBuilders, + Collection pipelineAggregatorFactories) { super.doValidate(parent, aggBuilders, pipelineAggregatorFactories); if (sigma < 0.0 ) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketsort/BucketSortPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketsort/BucketSortPipelineAggregationBuilder.java index 7f148c8e365b7..15c37061cd9ee 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketsort/BucketSortPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketsort/BucketSortPipelineAggregationBuilder.java @@ -38,6 +38,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Locale; @@ -145,8 +146,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, List aggFactories, - List pipelineAggregatoractories) { + public void doValidate(AggregatorFactory parent, Collection aggFactories, + Collection pipelineAggregatoractories) { if (sorts.isEmpty() && size == null && from == 0) { throw new IllegalStateException("[" + name + "] is configured to perform nothing. Please set either of " + Arrays.asList(SearchSourceBuilder.SORT_FIELD.getPreferredName(), SIZE.getPreferredName(), FROM.getPreferredName()) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregationBuilder.java index dbbb7fa534a57..209af3c03a7fc 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregationBuilder.java @@ -36,6 +36,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -97,8 +98,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, List aggFactories, - List pipelineAggregatorFactories) { + public void doValidate(AggregatorFactory parent, Collection aggFactories, + Collection pipelineAggregatorFactories) { if (bucketsPaths.length != 1) { throw new IllegalStateException(BUCKETS_PATH.getPreferredName() + " must contain a single entry for aggregation [" + name + "]"); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregationBuilder.java index ba7a2a2c03f7f..5fac90b094816 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregationBuilder.java @@ -42,6 +42,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -156,8 +157,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, List aggFactories, - List pipelineAggregatoractories) { + public void doValidate(AggregatorFactory parent, Collection aggFactories, + Collection pipelineAggregatoractories) { if (bucketsPaths.length != 1) { throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must contain a single entry for aggregation [" + name + "]"); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregationBuilder.java index 074ea1c25892b..1f36d5395b2da 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/movavg/MovAvgPipelineAggregationBuilder.java @@ -44,6 +44,7 @@ import java.io.IOException; import java.text.ParseException; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -260,8 +261,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, List aggFactories, - List pipelineAggregatoractories) { + public void doValidate(AggregatorFactory parent, Collection aggFactories, + Collection pipelineAggregatoractories) { if (minimize != null && minimize && !model.canBeMinimized()) { // If the user asks to minimize, but this model doesn't support // it, throw exception diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/movfn/MovFnPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/movfn/MovFnPipelineAggregationBuilder.java index 185e1c63b98b8..375125dbefc55 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/movfn/MovFnPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/movfn/MovFnPipelineAggregationBuilder.java @@ -39,7 +39,7 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; -import java.util.List; +import java.util.Collection; import java.util.Locale; import java.util.Map; import java.util.Objects; @@ -173,8 +173,8 @@ public void setWindow(int window) { } @Override - public void doValidate(AggregatorFactory parent, List aggFactories, - List pipelineAggregatoractories) { + public void doValidate(AggregatorFactory parent, Collection aggFactories, + Collection pipelineAggregatoractories) { if (window <= 0) { throw new IllegalArgumentException("[" + WINDOW.getPreferredName() + "] must be a positive, non-zero integer."); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java new file mode 100644 index 0000000000000..d8d7f416d2d84 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java @@ -0,0 +1,162 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch 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 org.elasticsearch.search.aggregations; + +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.io.stream.Writeable.Reader; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.search.SearchModule; +import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; +import org.elasticsearch.search.aggregations.pipeline.cumulativesum.CumulativeSumPipelineAggregationBuilder; +import org.elasticsearch.test.AbstractSerializingTestCase; +import org.junit.Before; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import static java.util.Collections.emptyList; +import static org.hamcrest.Matchers.equalTo; + +public class AggregatorFactoriesBuilderTests extends AbstractSerializingTestCase { + + private NamedWriteableRegistry namedWriteableRegistry; + private NamedXContentRegistry namedXContentRegistry; + + @Before + public void setUp() throws Exception { + super.setUp(); + + // register aggregations as NamedWriteable + SearchModule searchModule = new SearchModule(Settings.EMPTY, false, emptyList()); + namedWriteableRegistry = new NamedWriteableRegistry(searchModule.getNamedWriteables()); + namedXContentRegistry = new NamedXContentRegistry(searchModule.getNamedXContents()); + } + + @Override + protected NamedWriteableRegistry getNamedWriteableRegistry() { + return namedWriteableRegistry; + } + + @Override + protected NamedXContentRegistry xContentRegistry() { + return namedXContentRegistry; + } + + @Override + protected Builder doParseInstance(XContentParser parser) throws IOException { + // parseAggregators expects to be already inside the xcontent object + assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT)); + AggregatorFactories.Builder builder = AggregatorFactories.parseAggregators(parser); + return builder; + } + + @Override + protected Builder createTestInstance() { + AggregatorFactories.Builder builder = new AggregatorFactories.Builder(); + + // ensure that the unlikely does not happen: 2 aggs share the same name + Set names = new HashSet<>(); + for (int i = 0; i < randomIntBetween(1, 20); ++i) { + AggregationBuilder aggBuilder = getRandomAggregation(); + if (names.add(aggBuilder.getName())) { + builder.addAggregator(aggBuilder); + } + } + + for (int i = 0; i < randomIntBetween(0, 20); ++i) { + PipelineAggregationBuilder aggBuilder = getRandomPipelineAggregation(); + if (names.add(aggBuilder.getName())) { + builder.addPipelineAggregator(aggBuilder); + } + } + + return builder; + } + + @Override + protected Reader instanceReader() { + return AggregatorFactories.Builder::new; + } + + public void testUnorderedEqualsSubSet() { + Set names = new HashSet<>(); + List aggBuilders = new ArrayList<>(); + + while (names.size() < 2) { + AggregationBuilder aggBuilder = getRandomAggregation(); + + if (names.add(aggBuilder.getName())) { + aggBuilders.add(aggBuilder); + } + } + + AggregatorFactories.Builder builder1 = new AggregatorFactories.Builder(); + AggregatorFactories.Builder builder2 = new AggregatorFactories.Builder(); + + builder1.addAggregator(aggBuilders.get(0)); + builder1.addAggregator(aggBuilders.get(1)); + builder2.addAggregator(aggBuilders.get(1)); + + assertFalse(builder1.equals(builder2)); + assertFalse(builder2.equals(builder1)); + assertNotEquals(builder1.hashCode(), builder2.hashCode()); + + builder2.addAggregator(aggBuilders.get(0)); + assertTrue(builder1.equals(builder2)); + assertTrue(builder2.equals(builder1)); + assertEquals(builder1.hashCode(), builder2.hashCode()); + + builder1.addPipelineAggregator(getRandomPipelineAggregation()); + assertFalse(builder1.equals(builder2)); + assertFalse(builder2.equals(builder1)); + assertNotEquals(builder1.hashCode(), builder2.hashCode()); + } + + private static AggregationBuilder getRandomAggregation() { + // just a couple of aggregations, sufficient for the purpose of this test + final int randomAggregatorPoolSize = 4; + switch (randomIntBetween(1, randomAggregatorPoolSize)) { + case 1: + return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)); + case 2: + return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)); + case 3: + return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)); + case 4: + return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)); + } + + // never reached + return null; + } + + private static PipelineAggregationBuilder getRandomPipelineAggregation() { + // just 1 type of pipeline agg, sufficient for the purpose of this test + String name = randomAlphaOfLengthBetween(3, 20); + String bucketsPath = randomAlphaOfLengthBetween(3, 20); + PipelineAggregationBuilder builder = new CumulativeSumPipelineAggregationBuilder(name, bucketsPath); + return builder; + } +} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java index 642092507fed9..731156457a4f7 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java @@ -41,7 +41,7 @@ import org.elasticsearch.test.AbstractQueryTestCase; import org.elasticsearch.test.ESTestCase; -import java.util.List; +import java.util.Collection; import java.util.Random; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -74,7 +74,7 @@ public void setUp() throws Exception { public void testGetAggregatorFactories_returnsUnmodifiableList() { AggregatorFactories.Builder builder = new AggregatorFactories.Builder().addAggregator(AggregationBuilders.avg("foo")); - List aggregatorFactories = builder.getAggregatorFactories(); + Collection aggregatorFactories = builder.getAggregatorFactories(); assertThat(aggregatorFactories.size(), equalTo(1)); expectThrows(UnsupportedOperationException.class, () -> aggregatorFactories.add(AggregationBuilders.avg("bar"))); } @@ -82,7 +82,7 @@ public void testGetAggregatorFactories_returnsUnmodifiableList() { public void testGetPipelineAggregatorFactories_returnsUnmodifiableList() { AggregatorFactories.Builder builder = new AggregatorFactories.Builder().addPipelineAggregator( PipelineAggregatorBuilders.avgBucket("foo", "path1")); - List pipelineAggregatorFactories = builder.getPipelineAggregatorFactories(); + Collection pipelineAggregatorFactories = builder.getPipelineAggregatorFactories(); assertThat(pipelineAggregatorFactories.size(), equalTo(1)); expectThrows(UnsupportedOperationException.class, () -> pipelineAggregatorFactories.add(PipelineAggregatorBuilders.avgBucket("bar", "path2"))); @@ -269,10 +269,10 @@ public void testRewrite() throws Exception { AggregatorFactories.Builder rewritten = builder .rewrite(new QueryRewriteContext(xContentRegistry, null, null, () -> 0L)); assertNotSame(builder, rewritten); - List aggregatorFactories = rewritten.getAggregatorFactories(); + Collection aggregatorFactories = rewritten.getAggregatorFactories(); assertEquals(1, aggregatorFactories.size()); - assertThat(aggregatorFactories.get(0), instanceOf(FilterAggregationBuilder.class)); - FilterAggregationBuilder rewrittenFilterAggBuilder = (FilterAggregationBuilder) aggregatorFactories.get(0); + assertThat(aggregatorFactories.iterator().next(), instanceOf(FilterAggregationBuilder.class)); + FilterAggregationBuilder rewrittenFilterAggBuilder = (FilterAggregationBuilder) aggregatorFactories.iterator().next(); assertNotSame(filterAggBuilder, rewrittenFilterAggBuilder); assertNotEquals(filterAggBuilder, rewrittenFilterAggBuilder); // Check the filter was rewritten from a wrapper query to a terms query diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java index c7bbcfc147780..ea25e86b3a284 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java @@ -121,7 +121,7 @@ protected PipelineAggregationBuilder parse(XContentParser parser) throws IOExcep AggregatorFactories.Builder parsed = AggregatorFactories.parseAggregators(parser); assertThat(parsed.getAggregatorFactories(), hasSize(0)); assertThat(parsed.getPipelineAggregatorFactories(), hasSize(1)); - PipelineAggregationBuilder newAgg = parsed.getPipelineAggregatorFactories().get(0); + PipelineAggregationBuilder newAgg = parsed.getPipelineAggregatorFactories().iterator().next(); assertNull(parser.nextToken()); assertNotNull(newAgg); return newAgg; diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeTests.java index 08ae503102e86..1c198fd3ca5d6 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeTests.java @@ -36,7 +36,7 @@ public class DateRangeTests extends BaseAggregationTestCase 0L))); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceRangeTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceRangeTests.java index fcdbc81c0c63e..fc0a749f064eb 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceRangeTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceRangeTests.java @@ -41,7 +41,7 @@ public class GeoDistanceRangeTests extends BaseAggregationTestCase @Override protected RangeAggregationBuilder createTestAggregatorBuilder() { int numRanges = randomIntBetween(1, 10); - RangeAggregationBuilder factory = new RangeAggregationBuilder("foo"); + RangeAggregationBuilder factory = new RangeAggregationBuilder(randomAlphaOfLengthBetween(3, 10)); for (int i = 0; i < numRanges; i++) { String key = null; if (randomBoolean()) { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/SamplerTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/SamplerTests.java index e4de490f6b24e..5a9a28baf8231 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/SamplerTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/SamplerTests.java @@ -26,7 +26,7 @@ public class SamplerTests extends BaseAggregationTestCase { @@ -40,27 +40,27 @@ protected AvgBucketPipelineAggregationBuilder doCreateTestAggregatorFactory(Stri public void testValidate() { AggregationBuilder singleBucketAgg = new GlobalAggregationBuilder("global"); AggregationBuilder multiBucketAgg = new TermsAggregationBuilder("terms", ValueType.STRING); - final List aggBuilders = new ArrayList<>(); + final Set aggBuilders = new HashSet<>(); aggBuilders.add(singleBucketAgg); aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg final AvgBucketPipelineAggregationBuilder builder = new AvgBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptyList())); + () -> builder.validate(null, aggBuilders, Collections.emptySet())); assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); // Now try to point to a single bucket agg AvgBucketPipelineAggregationBuilder builder2 = new AvgBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptyList())); + ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() + " for buckets path: global>metric", ex.getMessage()); // Now try to point to a valid multi-bucket agg (no exception should be thrown) AvgBucketPipelineAggregationBuilder builder3 = new AvgBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptyList()); + builder3.validate(null, aggBuilders, Collections.emptySet()); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/ExtendedStatsBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/ExtendedStatsBucketTests.java index d177568700022..43303205b463e 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/ExtendedStatsBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/ExtendedStatsBucketTests.java @@ -28,9 +28,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.stats.extended.ExtendedStatsBucketPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; +import java.util.HashSet; +import java.util.Set; import static org.hamcrest.Matchers.equalTo; @@ -65,7 +65,7 @@ public void testSigmaFromInt() throws Exception { public void testValidate() { AggregationBuilder singleBucketAgg = new GlobalAggregationBuilder("global"); AggregationBuilder multiBucketAgg = new TermsAggregationBuilder("terms", ValueType.STRING); - final List aggBuilders = new ArrayList<>(); + final Set aggBuilders = new HashSet<>(); aggBuilders.add(singleBucketAgg); aggBuilders.add(multiBucketAgg); @@ -73,13 +73,13 @@ public void testValidate() { final ExtendedStatsBucketPipelineAggregationBuilder builder = new ExtendedStatsBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptyList())); + () -> builder.validate(null, aggBuilders, Collections.emptySet())); assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); // Now try to point to a single bucket agg ExtendedStatsBucketPipelineAggregationBuilder builder2 = new ExtendedStatsBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptyList())); + ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() + " for buckets path: global>metric", ex.getMessage()); @@ -87,6 +87,6 @@ public void testValidate() { // Now try to point to a valid multi-bucket agg (no exception should be // thrown) ExtendedStatsBucketPipelineAggregationBuilder builder3 = new ExtendedStatsBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptyList()); + builder3.validate(null, aggBuilders, Collections.emptySet()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/MaxBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/MaxBucketTests.java index a8e78a31f954e..cbf31130d38dc 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/MaxBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/MaxBucketTests.java @@ -26,9 +26,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.max.MaxBucketPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; +import java.util.HashSet; +import java.util.Set; public class MaxBucketTests extends AbstractBucketMetricsTestCase { @@ -40,20 +40,20 @@ protected MaxBucketPipelineAggregationBuilder doCreateTestAggregatorFactory(Stri public void testValidate() { AggregationBuilder singleBucketAgg = new GlobalAggregationBuilder("global"); AggregationBuilder multiBucketAgg = new TermsAggregationBuilder("terms", ValueType.STRING); - final List aggBuilders = new ArrayList<>(); + final Set aggBuilders = new HashSet<>(); aggBuilders.add(singleBucketAgg); aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg final MaxBucketPipelineAggregationBuilder builder = new MaxBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptyList())); + () -> builder.validate(null, aggBuilders, Collections.emptySet())); assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); // Now try to point to a single bucket agg MaxBucketPipelineAggregationBuilder builder2 = new MaxBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptyList())); + ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() + " for buckets path: global>metric", ex.getMessage()); @@ -61,7 +61,7 @@ public void testValidate() { // Now try to point to a valid multi-bucket agg (no exception should be // thrown) MaxBucketPipelineAggregationBuilder builder3 = new MaxBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptyList()); + builder3.validate(null, aggBuilders, Collections.emptySet()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/MinBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/MinBucketTests.java index 21efed4a5cff9..eca1db24ff7ff 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/MinBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/MinBucketTests.java @@ -26,9 +26,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.min.MinBucketPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; +import java.util.HashSet; +import java.util.Set; public class MinBucketTests extends AbstractBucketMetricsTestCase { @@ -40,20 +40,20 @@ protected MinBucketPipelineAggregationBuilder doCreateTestAggregatorFactory(Stri public void testValidate() { AggregationBuilder singleBucketAgg = new GlobalAggregationBuilder("global"); AggregationBuilder multiBucketAgg = new TermsAggregationBuilder("terms", ValueType.STRING); - final List aggBuilders = new ArrayList<>(); + final Set aggBuilders = new HashSet<>(); aggBuilders.add(singleBucketAgg); aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg final MinBucketPipelineAggregationBuilder builder = new MinBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptyList())); + () -> builder.validate(null, aggBuilders, Collections.emptySet())); assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); // Now try to point to a single bucket agg MinBucketPipelineAggregationBuilder builder2 = new MinBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptyList())); + ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() + " for buckets path: global>metric", ex.getMessage()); @@ -61,7 +61,7 @@ public void testValidate() { // Now try to point to a valid multi-bucket agg (no exception should be // thrown) MinBucketPipelineAggregationBuilder builder3 = new MinBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptyList()); + builder3.validate(null, aggBuilders, Collections.emptySet()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java index 4851c96972231..a6040aaf9f67c 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java @@ -28,9 +28,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.percentile.PercentilesBucketPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; +import java.util.HashSet; +import java.util.Set; import static org.hamcrest.Matchers.equalTo; @@ -69,7 +69,7 @@ public void testPercentsFromMixedArray() throws Exception { public void testValidate() { AggregationBuilder singleBucketAgg = new GlobalAggregationBuilder("global"); AggregationBuilder multiBucketAgg = new TermsAggregationBuilder("terms", ValueType.STRING); - final List aggBuilders = new ArrayList<>(); + final Set aggBuilders = new HashSet<>(); aggBuilders.add(singleBucketAgg); aggBuilders.add(multiBucketAgg); @@ -77,13 +77,13 @@ public void testValidate() { final PercentilesBucketPipelineAggregationBuilder builder = new PercentilesBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptyList())); + () -> builder.validate(null, aggBuilders, Collections.emptySet())); assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); // Now try to point to a single bucket agg PercentilesBucketPipelineAggregationBuilder builder2 = new PercentilesBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptyList())); + ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() + " for buckets path: global>metric", ex.getMessage()); @@ -91,6 +91,6 @@ public void testValidate() { // Now try to point to a valid multi-bucket agg (no exception should be // thrown) PercentilesBucketPipelineAggregationBuilder builder3 = new PercentilesBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptyList()); + builder3.validate(null, aggBuilders, Collections.emptySet()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/StatsBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/StatsBucketTests.java index 7611d7b07b3ae..bcd90778136bc 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/StatsBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/StatsBucketTests.java @@ -26,9 +26,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.stats.StatsBucketPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; +import java.util.HashSet; +import java.util.Set; public class StatsBucketTests extends AbstractBucketMetricsTestCase { @@ -41,20 +41,20 @@ protected StatsBucketPipelineAggregationBuilder doCreateTestAggregatorFactory(St public void testValidate() { AggregationBuilder singleBucketAgg = new GlobalAggregationBuilder("global"); AggregationBuilder multiBucketAgg = new TermsAggregationBuilder("terms", ValueType.STRING); - final List aggBuilders = new ArrayList<>(); + final Set aggBuilders = new HashSet<>(); aggBuilders.add(singleBucketAgg); aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg final StatsBucketPipelineAggregationBuilder builder = new StatsBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptyList())); + () -> builder.validate(null, aggBuilders, Collections.emptySet())); assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); // Now try to point to a single bucket agg StatsBucketPipelineAggregationBuilder builder2 = new StatsBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptyList())); + ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() + " for buckets path: global>metric", ex.getMessage()); @@ -62,7 +62,7 @@ public void testValidate() { // Now try to point to a valid multi-bucket agg (no exception should be // thrown) StatsBucketPipelineAggregationBuilder builder3 = new StatsBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptyList()); + builder3.validate(null, aggBuilders, Collections.emptySet()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/SumBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/SumBucketTests.java index 62fc1f977970f..be6c7f9234230 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/SumBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/SumBucketTests.java @@ -26,9 +26,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.sum.SumBucketPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; +import java.util.HashSet; +import java.util.Set; public class SumBucketTests extends AbstractBucketMetricsTestCase { @@ -40,20 +40,20 @@ protected SumBucketPipelineAggregationBuilder doCreateTestAggregatorFactory(Stri public void testValidate() { AggregationBuilder singleBucketAgg = new GlobalAggregationBuilder("global"); AggregationBuilder multiBucketAgg = new TermsAggregationBuilder("terms", ValueType.STRING); - final List aggBuilders = new ArrayList<>(); + final Set aggBuilders = new HashSet<>(); aggBuilders.add(singleBucketAgg); aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg final SumBucketPipelineAggregationBuilder builder = new SumBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptyList())); + () -> builder.validate(null, aggBuilders, Collections.emptySet())); assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); // Now try to point to a single bucket agg SumBucketPipelineAggregationBuilder builder2 = new SumBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptyList())); + ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() + " for buckets path: global>metric", ex.getMessage()); @@ -61,7 +61,7 @@ public void testValidate() { // Now try to point to a valid multi-bucket agg (no exception should be // thrown) SumBucketPipelineAggregationBuilder builder3 = new SumBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptyList()); + builder3.validate(null, aggBuilders, Collections.emptySet()); } } diff --git a/server/src/test/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java b/server/src/test/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java index 51bc5cc4e24bc..92bf4d6acad2c 100644 --- a/server/src/test/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java +++ b/server/src/test/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java @@ -31,6 +31,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; @@ -360,10 +361,12 @@ public void testComplexProfile() { assertThat(histoBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(histoAggResult.getProfiledChildren().size(), equalTo(2)); - ProfileResult tagsAggResult = histoAggResult.getProfiledChildren().get(0); + Map histoAggResultSubAggregations = histoAggResult.getProfiledChildren().stream() + .collect(Collectors.toMap(ProfileResult::getLuceneDescription, s -> s)); + + ProfileResult tagsAggResult = histoAggResultSubAggregations.get("tags"); assertThat(tagsAggResult, notNullValue()); assertThat(tagsAggResult.getQueryName(), equalTo(GlobalOrdinalsStringTermsAggregator.class.getSimpleName())); - assertThat(tagsAggResult.getLuceneDescription(), equalTo("tags")); assertThat(tagsAggResult.getTime(), greaterThan(0L)); Map tagsBreakdown = tagsAggResult.getTimeBreakdown(); assertThat(tagsBreakdown, notNullValue()); @@ -377,10 +380,12 @@ public void testComplexProfile() { assertThat(tagsBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(tagsAggResult.getProfiledChildren().size(), equalTo(2)); - ProfileResult avgAggResult = tagsAggResult.getProfiledChildren().get(0); + Map tagsAggResultSubAggregations = tagsAggResult.getProfiledChildren().stream() + .collect(Collectors.toMap(ProfileResult::getLuceneDescription, s -> s)); + + ProfileResult avgAggResult = tagsAggResultSubAggregations.get("avg"); assertThat(avgAggResult, notNullValue()); assertThat(avgAggResult.getQueryName(), equalTo("AvgAggregator")); - assertThat(avgAggResult.getLuceneDescription(), equalTo("avg")); assertThat(avgAggResult.getTime(), greaterThan(0L)); Map avgBreakdown = tagsAggResult.getTimeBreakdown(); assertThat(avgBreakdown, notNullValue()); @@ -394,10 +399,9 @@ public void testComplexProfile() { assertThat(avgBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(avgAggResult.getProfiledChildren().size(), equalTo(0)); - ProfileResult maxAggResult = tagsAggResult.getProfiledChildren().get(1); + ProfileResult maxAggResult = tagsAggResultSubAggregations.get("max"); assertThat(maxAggResult, notNullValue()); assertThat(maxAggResult.getQueryName(), equalTo("MaxAggregator")); - assertThat(maxAggResult.getLuceneDescription(), equalTo("max")); assertThat(maxAggResult.getTime(), greaterThan(0L)); Map maxBreakdown = tagsAggResult.getTimeBreakdown(); assertThat(maxBreakdown, notNullValue()); @@ -411,10 +415,9 @@ public void testComplexProfile() { assertThat(maxBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(maxAggResult.getProfiledChildren().size(), equalTo(0)); - ProfileResult stringsAggResult = histoAggResult.getProfiledChildren().get(1); + ProfileResult stringsAggResult = histoAggResultSubAggregations.get("strings"); assertThat(stringsAggResult, notNullValue()); assertThat(stringsAggResult.getQueryName(), equalTo(GlobalOrdinalsStringTermsAggregator.class.getSimpleName())); - assertThat(stringsAggResult.getLuceneDescription(), equalTo("strings")); assertThat(stringsAggResult.getTime(), greaterThan(0L)); Map stringsBreakdown = stringsAggResult.getTimeBreakdown(); assertThat(stringsBreakdown, notNullValue()); @@ -428,10 +431,12 @@ public void testComplexProfile() { assertThat(stringsBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(stringsAggResult.getProfiledChildren().size(), equalTo(3)); - avgAggResult = stringsAggResult.getProfiledChildren().get(0); + Map stringsAggResultSubAggregations = stringsAggResult.getProfiledChildren().stream() + .collect(Collectors.toMap(ProfileResult::getLuceneDescription, s -> s)); + + avgAggResult = stringsAggResultSubAggregations.get("avg"); assertThat(avgAggResult, notNullValue()); assertThat(avgAggResult.getQueryName(), equalTo("AvgAggregator")); - assertThat(avgAggResult.getLuceneDescription(), equalTo("avg")); assertThat(avgAggResult.getTime(), greaterThan(0L)); avgBreakdown = stringsAggResult.getTimeBreakdown(); assertThat(avgBreakdown, notNullValue()); @@ -445,10 +450,9 @@ public void testComplexProfile() { assertThat(avgBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(avgAggResult.getProfiledChildren().size(), equalTo(0)); - maxAggResult = stringsAggResult.getProfiledChildren().get(1); + maxAggResult = stringsAggResultSubAggregations.get("max"); assertThat(maxAggResult, notNullValue()); assertThat(maxAggResult.getQueryName(), equalTo("MaxAggregator")); - assertThat(maxAggResult.getLuceneDescription(), equalTo("max")); assertThat(maxAggResult.getTime(), greaterThan(0L)); maxBreakdown = stringsAggResult.getTimeBreakdown(); assertThat(maxBreakdown, notNullValue()); @@ -462,7 +466,7 @@ public void testComplexProfile() { assertThat(maxBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(maxAggResult.getProfiledChildren().size(), equalTo(0)); - tagsAggResult = stringsAggResult.getProfiledChildren().get(2); + tagsAggResult = stringsAggResultSubAggregations.get("tags"); assertThat(tagsAggResult, notNullValue()); assertThat(tagsAggResult.getQueryName(), equalTo(GlobalOrdinalsStringTermsAggregator.class.getSimpleName())); assertThat(tagsAggResult.getLuceneDescription(), equalTo("tags")); @@ -479,10 +483,12 @@ public void testComplexProfile() { assertThat(tagsBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(tagsAggResult.getProfiledChildren().size(), equalTo(2)); - avgAggResult = tagsAggResult.getProfiledChildren().get(0); + tagsAggResultSubAggregations = tagsAggResult.getProfiledChildren().stream() + .collect(Collectors.toMap(ProfileResult::getLuceneDescription, s -> s)); + + avgAggResult = tagsAggResultSubAggregations.get("avg"); assertThat(avgAggResult, notNullValue()); assertThat(avgAggResult.getQueryName(), equalTo("AvgAggregator")); - assertThat(avgAggResult.getLuceneDescription(), equalTo("avg")); assertThat(avgAggResult.getTime(), greaterThan(0L)); avgBreakdown = tagsAggResult.getTimeBreakdown(); assertThat(avgBreakdown, notNullValue()); @@ -496,10 +502,9 @@ public void testComplexProfile() { assertThat(avgBreakdown.get(AggregationTimingType.REDUCE.toString()), equalTo(0L)); assertThat(avgAggResult.getProfiledChildren().size(), equalTo(0)); - maxAggResult = tagsAggResult.getProfiledChildren().get(1); + maxAggResult = tagsAggResultSubAggregations.get("max"); assertThat(maxAggResult, notNullValue()); assertThat(maxAggResult.getQueryName(), equalTo("MaxAggregator")); - assertThat(maxAggResult.getLuceneDescription(), equalTo("max")); assertThat(maxAggResult.getTime(), greaterThan(0L)); maxBreakdown = tagsAggResult.getTimeBreakdown(); assertThat(maxBreakdown, notNullValue()); diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java index 4a6e17005499f..ffd19d8e94d6e 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java @@ -32,6 +32,10 @@ import org.elasticsearch.test.AbstractBuilderTestCase; import java.io.IOException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; import static org.hamcrest.Matchers.hasSize; @@ -63,6 +67,58 @@ public void testFromXContent() throws IOException { assertEquals(testAgg.hashCode(), newAgg.hashCode()); } + /** + * Create at least 2 aggregations and test equality and hash + */ + public void testFromXContentMulti() throws IOException { + AggregatorFactories.Builder factoriesBuilder = AggregatorFactories.builder(); + List testAggs = createTestAggregatorBuilders(); + + for (AB testAgg : testAggs) { + factoriesBuilder.addAggregator(testAgg); + } + + XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values())); + if (randomBoolean()) { + builder.prettyPrint(); + } + factoriesBuilder.toXContent(builder, ToXContent.EMPTY_PARAMS); + XContentBuilder shuffled = shuffleXContent(builder); + XContentParser parser = createParser(shuffled); + + assertSame(XContentParser.Token.START_OBJECT, parser.nextToken()); + AggregatorFactories.Builder parsed = AggregatorFactories.parseAggregators(parser); + + assertThat(parsed.getAggregatorFactories(), hasSize(testAggs.size())); + assertThat(parsed.getPipelineAggregatorFactories(), hasSize(0)); + assertEquals(factoriesBuilder, parsed); + assertEquals(factoriesBuilder.hashCode(), parsed.hashCode()); + } + + /** + * Create at least 2 aggregations and test equality and hash + */ + public void testSerializationMulti() throws IOException { + AggregatorFactories.Builder builder = AggregatorFactories.builder(); + List testAggs = createTestAggregatorBuilders(); + + for (AB testAgg : testAggs) { + builder.addAggregator(testAgg); + } + + try (BytesStreamOutput output = new BytesStreamOutput()) { + builder.writeTo(output); + + try (StreamInput in = new NamedWriteableAwareStreamInput(output.bytes().streamInput(), namedWriteableRegistry())) { + AggregatorFactories.Builder newBuilder = new AggregatorFactories.Builder(in); + + assertEquals(builder, newBuilder); + assertEquals(builder.hashCode(), newBuilder.hashCode()); + assertNotSame(builder, newBuilder); + } + } + } + /** * Generic test that checks that the toString method renders the XContent * correctly. @@ -82,7 +138,7 @@ protected AggregationBuilder parse(XContentParser parser) throws IOException { AggregatorFactories.Builder parsed = AggregatorFactories.parseAggregators(parser); assertThat(parsed.getAggregatorFactories(), hasSize(1)); assertThat(parsed.getPipelineAggregatorFactories(), hasSize(0)); - AggregationBuilder newAgg = parsed.getAggregatorFactories().get(0); + AggregationBuilder newAgg = parsed.getAggregatorFactories().iterator().next(); assertNull(parser.nextToken()); assertNotNull(newAgg); return newAgg; @@ -160,4 +216,21 @@ protected void randomFieldOrScript(ValuesSourceAggregationBuilder factory, throw new AssertionError("Unknow random operation [" + choice + "]"); } } + + private List createTestAggregatorBuilders() { + int numberOfAggregatorBuilders = randomIntBetween(2, 10); + + // ensure that we do not create 2 aggregations with the same name + Set names = new HashSet<>(); + List aggBuilders = new ArrayList<>(); + + while (names.size() < numberOfAggregatorBuilders) { + AB aggBuilder = createTestAggregatorBuilder(); + + if (names.add(aggBuilder.getName())) { + aggBuilders.add(aggBuilder); + } + } + return aggBuilders; + } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java index 03b58732a378a..9cad992327e25 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java @@ -35,6 +35,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -549,7 +550,7 @@ void validateAggregations() { throw ExceptionsHelper.badRequestException( Messages.getMessage(Messages.DATAFEED_CONFIG_CANNOT_USE_SCRIPT_FIELDS_WITH_AGGS)); } - List aggregatorFactories = aggregations.getAggregatorFactories(); + Collection aggregatorFactories = aggregations.getAggregatorFactories(); if (aggregatorFactories.isEmpty()) { throw ExceptionsHelper.badRequestException(Messages.DATAFEED_AGGREGATIONS_REQUIRES_DATE_HISTOGRAM); } @@ -560,7 +561,7 @@ void validateAggregations() { checkHistogramIntervalIsPositive(histogramAggregation); } - private static void checkNoMoreHistogramAggregations(List aggregations) { + private static void checkNoMoreHistogramAggregations(Collection aggregations) { for (AggregationBuilder agg : aggregations) { if (ExtractorUtils.isHistogram(agg)) { throw ExceptionsHelper.badRequestException(Messages.DATAFEED_AGGREGATIONS_MAX_ONE_DATE_HISTOGRAM); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/extractor/ExtractorUtils.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/extractor/ExtractorUtils.java index b0794adae4a69..6d9312654facb 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/extractor/ExtractorUtils.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/extractor/ExtractorUtils.java @@ -26,7 +26,7 @@ import java.io.IOException; import java.util.Arrays; -import java.util.List; +import java.util.Collection; import java.util.concurrent.TimeUnit; /** @@ -83,7 +83,7 @@ public static long getHistogramIntervalMillis(AggregatorFactories.Builder aggFac * @param aggregations List of aggregations * @return A {@link HistogramAggregationBuilder} or a {@link DateHistogramAggregationBuilder} */ - public static AggregationBuilder getHistogramAggregation(List aggregations) { + public static AggregationBuilder getHistogramAggregation(Collection aggregations) { if (aggregations.isEmpty()) { throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.DATAFEED_AGGREGATIONS_REQUIRES_DATE_HISTOGRAM)); } @@ -91,7 +91,7 @@ public static AggregationBuilder getHistogramAggregation(List afterKey = composite.afterKey(); // a null after-key means done if (afterKey != null) { - AggregationBuilder aggBuilder = next.aggregations().getAggregatorFactories().get(0); + AggregationBuilder aggBuilder = next.aggregations().getAggregatorFactories().iterator().next(); // update after-key with the new value if (aggBuilder instanceof CompositeAggregationBuilder) { CompositeAggregationBuilder comp = (CompositeAggregationBuilder) aggBuilder; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java index 43c72b5583d11..3d84f852bc870 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java @@ -65,7 +65,7 @@ public void testLimit() { SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, null, size); Builder aggBuilder = sourceBuilder.aggregations(); assertEquals(1, aggBuilder.count()); - CompositeAggregationBuilder composite = (CompositeAggregationBuilder) aggBuilder.getAggregatorFactories().get(0); + CompositeAggregationBuilder composite = (CompositeAggregationBuilder) aggBuilder.getAggregatorFactories().iterator().next(); assertEquals(size, composite.size()); }