From 33cdd8e30cda2724525d5d1b99e9d833ec304223 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Fri, 21 Sep 2018 14:36:04 +0200 Subject: [PATCH 01/12] Fixes the equals and hash function to ignore the order of aggregations. Mainly for testing purposes. --- .../aggregations/AggregatorFactories.java | 10 +- .../AggregatorFactoriesBuilderTests.java | 126 ++++++++++++++++++ 2 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java 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..dfccda9766c2e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -453,7 +453,9 @@ public String toString() { @Override public int hashCode() { - return Objects.hash(aggregationBuilders, pipelineAggregatorBuilders); + // implementation of an order independent hash: take hash of every element and XOR it + return aggregationBuilders.stream().mapToInt(Object::hashCode).reduce(0, (left, right) -> left ^ right) + ^ pipelineAggregatorBuilders.stream().mapToInt(Object::hashCode).reduce(0, (left, right) -> left ^ right); } @Override @@ -463,9 +465,11 @@ public boolean equals(Object obj) { if (getClass() != obj.getClass()) return false; Builder other = (Builder) obj; - if (!Objects.equals(aggregationBuilders, other.aggregationBuilders)) + + // compare aggregations independent of their order + if (!aggregationBuilders.containsAll(other.aggregationBuilders)) return false; - if (!Objects.equals(pipelineAggregatorBuilders, other.pipelineAggregatorBuilders)) + if (!pipelineAggregatorBuilders.containsAll(other.pipelineAggregatorBuilders)) return false; return true; } 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..130202f775599 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java @@ -0,0 +1,126 @@ +/* + * 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.HashSet; +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 = 1; i < randomIntBetween(1, 20); ++i) { + AggregationBuilder aggBuilder = getRandomAggregation(); + if (names.add(aggBuilder.getName())) { + builder.addAggregator(aggBuilder); + } + } + + for (int i = 1; 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; + } + + 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; + } +} From 96fe03ebc4c73e90886139c2d1b226561fc053f2 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Fri, 21 Sep 2018 15:21:54 +0200 Subject: [PATCH 02/12] check size of aggregator lists --- .../aggregations/AggregatorFactories.java | 3 ++ .../AggregatorFactoriesBuilderTests.java | 33 +++++++++++++++++++ 2 files changed, 36 insertions(+) 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 dfccda9766c2e..594a54fcbf04d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -467,6 +467,9 @@ public boolean equals(Object obj) { Builder other = (Builder) obj; // compare aggregations independent of their order + if (aggregationBuilders.size() != other.aggregationBuilders.size() + || pipelineAggregatorBuilders.size() != other.pipelineAggregatorBuilders.size()) + return false; if (!aggregationBuilders.containsAll(other.aggregationBuilders)) return false; if (!pipelineAggregatorBuilders.containsAll(other.pipelineAggregatorBuilders)) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java index 130202f775599..e18305a657b86 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java @@ -31,7 +31,9 @@ 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; @@ -98,6 +100,37 @@ 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)); + + builder2.addAggregator(aggBuilders.get(0)); + assertTrue(builder1.equals(builder2)); + assertTrue(builder2.equals(builder1)); + + builder1.addPipelineAggregator(getRandomPipelineAggregation()); + assertFalse(builder1.equals(builder2)); + assertFalse(builder2.equals(builder1)); + } + private static AggregationBuilder getRandomAggregation() { // just a couple of aggregations, sufficient for the purpose of this test final int randomAggregatorPoolSize = 4; From 4bca3c35d876566066cdf832f78b9b3a5ac93f36 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Fri, 21 Sep 2018 15:24:23 +0200 Subject: [PATCH 03/12] add tests for hashCode as well --- .../search/aggregations/AggregatorFactoriesBuilderTests.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java index e18305a657b86..755bfcc59a2fb 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java @@ -121,14 +121,17 @@ public void testUnorderedEqualsSubSet() { 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() { From fc69f096427fb77ce565dd7754257c100691705b Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Fri, 21 Sep 2018 15:39:49 +0200 Subject: [PATCH 04/12] remove unused --- .../elasticsearch/search/aggregations/AggregatorFactories.java | 1 - 1 file changed, 1 deletion(-) 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 594a54fcbf04d..41eaee55db495 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -44,7 +44,6 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; From a61dcbced85a5380a797f2cf6fd4b4090d398b96 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Mon, 24 Sep 2018 17:18:46 +0200 Subject: [PATCH 05/12] refactor to use Set instead of List --- .../aggregations/AggregationBuilder.java | 6 +-- .../aggregations/AggregatorFactories.java | 46 +++++++++---------- .../PipelineAggregationBuilder.java | 6 +-- .../AbstractPipelineAggregationBuilder.java | 10 ++-- ...cketMetricsPipelineAggregationBuilder.java | 6 +-- ...tilesBucketPipelineAggregationBuilder.java | 6 +-- ...StatsBucketPipelineAggregationBuilder.java | 6 +-- .../BucketSortPipelineAggregationBuilder.java | 5 +- ...mulativeSumPipelineAggregationBuilder.java | 5 +- .../DerivativePipelineAggregationBuilder.java | 5 +- .../MovAvgPipelineAggregationBuilder.java | 5 +- .../MovFnPipelineAggregationBuilder.java | 6 +-- .../AggregatorFactoriesTests.java | 12 ++--- .../BasePipelineAggregationTestCase.java | 2 +- .../aggregations/bucket/FiltersTests.java | 4 +- .../bucketmetrics/AvgBucketTests.java | 12 ++--- .../ExtendedStatsBucketTests.java | 12 ++--- .../bucketmetrics/MaxBucketTests.java | 12 ++--- .../bucketmetrics/MinBucketTests.java | 12 ++--- .../bucketmetrics/PercentilesBucketTests.java | 12 ++--- .../bucketmetrics/StatsBucketTests.java | 12 ++--- .../bucketmetrics/SumBucketTests.java | 12 ++--- .../aggregations/BaseAggregationTestCase.java | 2 +- .../core/ml/datafeed/DatafeedConfig.java | 5 +- .../ml/datafeed/extractor/ExtractorUtils.java | 6 +-- .../rollup/action/SearchActionTests.java | 8 ++-- .../job/RollupIndexerIndexingTests.java | 5 +- .../search/CompositeAggregationCursor.java | 2 +- .../search/SourceGeneratorTests.java | 2 +- 29 files changed, 124 insertions(+), 120 deletions(-) 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..1c2f2b6cbe72a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java @@ -28,8 +28,8 @@ import org.elasticsearch.search.internal.SearchContext; import java.io.IOException; -import java.util.List; import java.util.Map; +import java.util.Set; /** * A factory that knows how to create an {@link Aggregator} of a specific type. @@ -79,12 +79,12 @@ public String getName() { public abstract AggregationBuilder subAggregation(PipelineAggregationBuilder aggregation); /** Return the configured set of subaggregations **/ - public List getSubAggregations() { + public Set getSubAggregations() { return factoriesBuilder.getAggregatorFactories(); } /** Return the configured set of pipeline aggregations **/ - public List getPipelineAggregations() { + public Set 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 41eaee55db495..df725877634a0 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -44,6 +44,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -236,8 +237,8 @@ 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<>(); + private final Set aggregationBuilders = new HashSet<>(); + private final Set pipelineAggregatorBuilders = new HashSet<>(); private boolean skipResolveOrder; /** @@ -321,29 +322,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, + Set pipelineAggregatorBuilders2, Set aggregationBuilders2, AggregatorFactory parent) { Map pipelineAggregatorBuildersMap = new HashMap<>(); - for (PipelineAggregationBuilder builder : pipelineAggregatorBuilders) { + for (PipelineAggregationBuilder builder : pipelineAggregatorBuilders2) { pipelineAggregatorBuildersMap.put(builder.getName(), builder); } Map aggBuildersMap = new HashMap<>(); - for (AggregationBuilder aggBuilder : aggBuilders) { + for (AggregationBuilder aggBuilder : aggregationBuilders2) { aggBuildersMap.put(aggBuilder.name, aggBuilder); } List orderedPipelineAggregatorrs = new LinkedList<>(); - List unmarkedBuilders = new ArrayList<>(pipelineAggregatorBuilders); + List unmarkedBuilders = new ArrayList<>(pipelineAggregatorBuilders2); Set temporarilyMarked = new HashSet<>(); while (!unmarkedBuilders.isEmpty()) { PipelineAggregationBuilder builder = unmarkedBuilders.get(0); - builder.validate(parent, aggBuilders, pipelineAggregatorBuilders); + builder.validate(parent, aggregationBuilders2, pipelineAggregatorBuilders2); resolvePipelineAggregatorOrder(aggBuildersMap, pipelineAggregatorBuildersMap, orderedPipelineAggregatorrs, unmarkedBuilders, temporarilyMarked, builder); } @@ -374,7 +378,7 @@ private void resolvePipelineAggregatorOrder(Map aggB } else { // Check the non-pipeline sub-aggregator // factories - List subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders; + Set subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders; boolean foundSubBuilder = false; for (AggregationBuilder subBuilder : subBuilders) { if (aggName.equals(subBuilder.name)) { @@ -385,7 +389,7 @@ private void resolvePipelineAggregatorOrder(Map aggB } // Check the pipeline sub-aggregator factories if (!foundSubBuilder && (i == bucketsPathElements.size() - 1)) { - List subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders; + Set subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders; for (PipelineAggregationBuilder subFactory : subPipelineBuilders) { if (aggName.equals(subFactory.getName())) { foundSubBuilder = true; @@ -416,12 +420,12 @@ private void resolvePipelineAggregatorOrder(Map aggB } } - public List getAggregatorFactories() { - return Collections.unmodifiableList(aggregationBuilders); + public Set getAggregatorFactories() { + return Collections.unmodifiableSet(aggregationBuilders); } - public List getPipelineAggregatorFactories() { - return Collections.unmodifiableList(pipelineAggregatorBuilders); + public Set getPipelineAggregatorFactories() { + return Collections.unmodifiableSet(pipelineAggregatorBuilders); } public int count() { @@ -452,9 +456,7 @@ public String toString() { @Override public int hashCode() { - // implementation of an order independent hash: take hash of every element and XOR it - return aggregationBuilders.stream().mapToInt(Object::hashCode).reduce(0, (left, right) -> left ^ right) - ^ pipelineAggregatorBuilders.stream().mapToInt(Object::hashCode).reduce(0, (left, right) -> left ^ right); + return Objects.hash(aggregationBuilders, pipelineAggregatorBuilders); } @Override @@ -465,13 +467,9 @@ public boolean equals(Object obj) { return false; Builder other = (Builder) obj; - // compare aggregations independent of their order - if (aggregationBuilders.size() != other.aggregationBuilders.size() - || pipelineAggregatorBuilders.size() != other.pipelineAggregatorBuilders.size()) - return false; - if (!aggregationBuilders.containsAll(other.aggregationBuilders)) + if (!Objects.equals(aggregationBuilders, other.aggregationBuilders)) return false; - if (!pipelineAggregatorBuilders.containsAll(other.pipelineAggregatorBuilders)) + if (!Objects.equals(pipelineAggregatorBuilders, other.pipelineAggregatorBuilders)) return false; return true; } 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..20f22ba081af5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java @@ -25,8 +25,8 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; -import java.util.List; import java.util.Map; +import java.util.Set; /** * A factory that knows how to create an {@link PipelineAggregator} of a @@ -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, Set aggregationBuilders2, + Set pipelineAggregatorBuilders2); /** * 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..52fda6cc18d26 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,9 +28,9 @@ import java.io.IOException; import java.util.Arrays; -import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; /** * Base implementation of a {@link PipelineAggregationBuilder}. @@ -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, Set factories, + Set 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, Set factories, + Set 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..cce600ae2c95a 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,10 +32,10 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; public abstract class BucketMetricsPipelineAggregationBuilder> extends AbstractPipelineAggregationBuilder { @@ -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, Set aggBuilders, + Set 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..2c2e8d5a36609 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,9 +35,9 @@ import java.io.IOException; import java.util.Arrays; -import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; public class PercentilesBucketPipelineAggregationBuilder extends BucketMetricsPipelineAggregationBuilder { @@ -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, Set aggFactories, + Set 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..c1bc172836cb9 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,9 +29,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.BucketMetricsPipelineAggregationBuilder; import java.io.IOException; -import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; public class ExtendedStatsBucketPipelineAggregationBuilder extends BucketMetricsPipelineAggregationBuilder { @@ -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, Set aggBuilders, + Set 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..6c76804911241 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 @@ -43,6 +43,7 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.Set; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.GAP_POLICY; @@ -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, Set aggFactories, + Set 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..28c0d17d62466 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 @@ -39,6 +39,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT; @@ -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, Set aggFactories, + Set 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..0c3258397c4f2 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 @@ -45,6 +45,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; public class DerivativePipelineAggregationBuilder extends AbstractPipelineAggregationBuilder { public static final String NAME = "derivative"; @@ -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, Set aggFactories, + Set 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..3bb7b21ae4d33 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 @@ -47,6 +47,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT; @@ -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, Set aggFactories, + Set 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..20974e1d9d4c1 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,10 +39,10 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; -import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.function.Function; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; @@ -173,8 +173,8 @@ public void setWindow(int window) { } @Override - public void doValidate(AggregatorFactory parent, List aggFactories, - List pipelineAggregatoractories) { + public void doValidate(AggregatorFactory parent, Set aggFactories, + Set 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/AggregatorFactoriesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java index 642092507fed9..c5e7d85ded784 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesTests.java @@ -41,8 +41,8 @@ import org.elasticsearch.test.AbstractQueryTestCase; import org.elasticsearch.test.ESTestCase; -import java.util.List; import java.util.Random; +import java.util.Set; 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(); + Set 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(); + Set 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(); + Set 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/FiltersTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java index bdfdd4d028f0f..4c7fdccb64b00 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/FiltersTests.java @@ -171,9 +171,9 @@ public void testRewrite() throws IOException { assertNotEquals(original, rewritten); assertThat(rewritten, instanceOf(TermsAggregationBuilder.class)); assertThat(rewritten.getSubAggregations().size(), equalTo(1)); - AggregationBuilder subAgg = rewritten.getSubAggregations().get(0); + AggregationBuilder subAgg = rewritten.getSubAggregations().iterator().next(); assertThat(subAgg, instanceOf(FiltersAggregationBuilder.class)); - assertNotSame(original.getSubAggregations().get(0), subAgg); + assertNotSame(original.getSubAggregations().iterator().next(), subAgg); assertEquals("my-agg", subAgg.getName()); assertSame(rewritten, rewritten.rewrite(new QueryRewriteContext(xContentRegistry(), null, null, () -> 0L))); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/AvgBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/AvgBucketTests.java index 223cbb231ea8d..c504aa3f46183 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/AvgBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/AvgBucketTests.java @@ -26,9 +26,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.avg.AvgBucketPipelineAggregationBuilder; 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 AvgBucketTests extends AbstractBucketMetricsTestCase { @@ -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/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..437039aaa6c87 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 @@ -82,7 +82,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; 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..b659de4be07f9 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 @@ -41,6 +41,7 @@ import java.util.Map; import java.util.Objects; import java.util.Random; +import java.util.Set; import java.util.concurrent.TimeUnit; /** @@ -549,7 +550,7 @@ void validateAggregations() { throw ExceptionsHelper.badRequestException( Messages.getMessage(Messages.DATAFEED_CONFIG_CANNOT_USE_SCRIPT_FIELDS_WITH_AGGS)); } - List aggregatorFactories = aggregations.getAggregatorFactories(); + Set 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(Set 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..c1fdb524db264 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.Set; 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(Set 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()); } From 1e36d2a8ba0303fa93fa20e56cc30ac2f1a0aecd Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 25 Sep 2018 20:22:05 +0200 Subject: [PATCH 06/12] address review comments --- .../aggregations/AggregationBuilder.java | 6 ++-- .../aggregations/AggregatorFactories.java | 31 ++++++++++--------- .../PipelineAggregationBuilder.java | 6 ++-- .../AbstractPipelineAggregationBuilder.java | 10 +++--- ...cketMetricsPipelineAggregationBuilder.java | 6 ++-- ...tilesBucketPipelineAggregationBuilder.java | 6 ++-- ...StatsBucketPipelineAggregationBuilder.java | 6 ++-- .../BucketSortPipelineAggregationBuilder.java | 6 ++-- ...mulativeSumPipelineAggregationBuilder.java | 6 ++-- .../DerivativePipelineAggregationBuilder.java | 6 ++-- .../MovAvgPipelineAggregationBuilder.java | 6 ++-- .../MovFnPipelineAggregationBuilder.java | 6 ++-- .../AggregatorFactoriesBuilderTests.java | 4 +-- .../AggregatorFactoriesTests.java | 8 ++--- .../core/ml/datafeed/DatafeedConfig.java | 6 ++-- .../ml/datafeed/extractor/ExtractorUtils.java | 4 +-- 16 files changed, 62 insertions(+), 61 deletions(-) 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 1c2f2b6cbe72a..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,8 +28,8 @@ import org.elasticsearch.search.internal.SearchContext; import java.io.IOException; +import java.util.Collection; import java.util.Map; -import java.util.Set; /** * A factory that knows how to create an {@link Aggregator} of a specific type. @@ -79,12 +79,12 @@ public String getName() { public abstract AggregationBuilder subAggregation(PipelineAggregationBuilder aggregation); /** Return the configured set of subaggregations **/ - public Set getSubAggregations() { + public Collection getSubAggregations() { return factoriesBuilder.getAggregatorFactories(); } /** Return the configured set of pipeline aggregations **/ - public Set 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 df725877634a0..a46b76d6bb25e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -38,6 +38,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -237,8 +238,8 @@ public int countPipelineAggregators() { public static class Builder implements Writeable, ToXContentObject { private final Set names = new HashSet<>(); - private final Set aggregationBuilders = new HashSet<>(); - private final Set pipelineAggregatorBuilders = new HashSet<>(); + private final Collection aggregationBuilders = new HashSet<>(); + private final Collection pipelineAggregatorBuilders = new HashSet<>(); private boolean skipResolveOrder; /** @@ -332,22 +333,22 @@ public AggregatorFactories build(SearchContext context, AggregatorFactory par } private List resolvePipelineAggregatorOrder( - Set pipelineAggregatorBuilders2, Set aggregationBuilders2, + Collection pipelineAggregatorBuilders, Collection aggregationBuilders, AggregatorFactory parent) { Map pipelineAggregatorBuildersMap = new HashMap<>(); - for (PipelineAggregationBuilder builder : pipelineAggregatorBuilders2) { + for (PipelineAggregationBuilder builder : pipelineAggregatorBuilders) { pipelineAggregatorBuildersMap.put(builder.getName(), builder); } Map aggBuildersMap = new HashMap<>(); - for (AggregationBuilder aggBuilder : aggregationBuilders2) { + for (AggregationBuilder aggBuilder : aggregationBuilders) { aggBuildersMap.put(aggBuilder.name, aggBuilder); } List orderedPipelineAggregatorrs = new LinkedList<>(); - List unmarkedBuilders = new ArrayList<>(pipelineAggregatorBuilders2); - Set temporarilyMarked = new HashSet<>(); + List unmarkedBuilders = new ArrayList<>(pipelineAggregatorBuilders); + Collection temporarilyMarked = new HashSet<>(); while (!unmarkedBuilders.isEmpty()) { PipelineAggregationBuilder builder = unmarkedBuilders.get(0); - builder.validate(parent, aggregationBuilders2, pipelineAggregatorBuilders2); + builder.validate(parent, aggregationBuilders, pipelineAggregatorBuilders); resolvePipelineAggregatorOrder(aggBuildersMap, pipelineAggregatorBuildersMap, orderedPipelineAggregatorrs, unmarkedBuilders, temporarilyMarked, builder); } @@ -357,7 +358,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)) { @@ -378,7 +379,7 @@ private void resolvePipelineAggregatorOrder(Map aggB } else { // Check the non-pipeline sub-aggregator // factories - Set subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders; + Collection subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders; boolean foundSubBuilder = false; for (AggregationBuilder subBuilder : subBuilders) { if (aggName.equals(subBuilder.name)) { @@ -389,7 +390,7 @@ private void resolvePipelineAggregatorOrder(Map aggB } // Check the pipeline sub-aggregator factories if (!foundSubBuilder && (i == bucketsPathElements.size() - 1)) { - Set subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders; + Collection subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders; for (PipelineAggregationBuilder subFactory : subPipelineBuilders) { if (aggName.equals(subFactory.getName())) { foundSubBuilder = true; @@ -420,12 +421,12 @@ private void resolvePipelineAggregatorOrder(Map aggB } } - public Set getAggregatorFactories() { - return Collections.unmodifiableSet(aggregationBuilders); + public Collection getAggregatorFactories() { + return Collections.unmodifiableCollection(aggregationBuilders); } - public Set getPipelineAggregatorFactories() { - return Collections.unmodifiableSet(pipelineAggregatorBuilders); + public Collection getPipelineAggregatorFactories() { + return Collections.unmodifiableCollection(pipelineAggregatorBuilders); } public int count() { 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 20f22ba081af5..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,8 +25,8 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; +import java.util.Collection; import java.util.Map; -import java.util.Set; /** * A factory that knows how to create an {@link PipelineAggregator} of a @@ -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, Set aggregationBuilders2, - Set pipelineAggregatorBuilders2); + 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 52fda6cc18d26..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,9 +28,9 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.Map; import java.util.Objects; -import java.util.Set; /** * Base implementation of a {@link PipelineAggregationBuilder}. @@ -81,8 +81,8 @@ public String type() { * configured) */ @Override - public final void validate(AggregatorFactory parent, Set factories, - Set 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, Set factories, - Set 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 cce600ae2c95a..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,10 +32,10 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; +import java.util.Collection; import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.Set; public abstract class BucketMetricsPipelineAggregationBuilder> extends AbstractPipelineAggregationBuilder { @@ -109,8 +109,8 @@ public GapPolicy gapPolicy() { protected abstract PipelineAggregator createInternal(Map metaData) throws IOException; @Override - public void doValidate(AggregatorFactory parent, Set aggBuilders, - Set 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 2c2e8d5a36609..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,9 +35,9 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.Map; import java.util.Objects; -import java.util.Set; public class PercentilesBucketPipelineAggregationBuilder extends BucketMetricsPipelineAggregationBuilder { @@ -95,8 +95,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, Set aggFactories, - Set 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 c1bc172836cb9..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,9 +29,9 @@ import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.BucketMetricsPipelineAggregationBuilder; import java.io.IOException; +import java.util.Collection; import java.util.Map; import java.util.Objects; -import java.util.Set; public class ExtendedStatsBucketPipelineAggregationBuilder extends BucketMetricsPipelineAggregationBuilder { @@ -82,8 +82,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, Set aggBuilders, - Set 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 6c76804911241..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,12 +38,12 @@ 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; import java.util.Map; import java.util.Objects; -import java.util.Set; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.GAP_POLICY; @@ -146,8 +146,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, Set aggFactories, - Set 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 28c0d17d62466..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,10 +36,10 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Set; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT; @@ -98,8 +98,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, Set aggFactories, - Set 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 0c3258397c4f2..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,10 +42,10 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Set; public class DerivativePipelineAggregationBuilder extends AbstractPipelineAggregationBuilder { public static final String NAME = "derivative"; @@ -157,8 +157,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, Set aggFactories, - Set 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 3bb7b21ae4d33..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,10 +44,10 @@ 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; -import java.util.Set; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT; @@ -261,8 +261,8 @@ protected PipelineAggregator createInternal(Map metaData) throws } @Override - public void doValidate(AggregatorFactory parent, Set aggFactories, - Set 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 20974e1d9d4c1..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,10 +39,10 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; +import java.util.Collection; import java.util.Locale; import java.util.Map; import java.util.Objects; -import java.util.Set; import java.util.function.Function; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; @@ -173,8 +173,8 @@ public void setWindow(int window) { } @Override - public void doValidate(AggregatorFactory parent, Set aggFactories, - Set 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 index 755bfcc59a2fb..d8d7f416d2d84 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java @@ -78,14 +78,14 @@ protected Builder createTestInstance() { // ensure that the unlikely does not happen: 2 aggs share the same name Set names = new HashSet<>(); - for (int i = 1; i < randomIntBetween(1, 20); ++i) { + for (int i = 0; i < randomIntBetween(1, 20); ++i) { AggregationBuilder aggBuilder = getRandomAggregation(); if (names.add(aggBuilder.getName())) { builder.addAggregator(aggBuilder); } } - for (int i = 1; i < randomIntBetween(0, 20); ++i) { + for (int i = 0; i < randomIntBetween(0, 20); ++i) { PipelineAggregationBuilder aggBuilder = getRandomPipelineAggregation(); if (names.add(aggBuilder.getName())) { builder.addPipelineAggregator(aggBuilder); 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 c5e7d85ded784..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,8 +41,8 @@ import org.elasticsearch.test.AbstractQueryTestCase; import org.elasticsearch.test.ESTestCase; +import java.util.Collection; import java.util.Random; -import java.util.Set; 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")); - Set 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")); - Set pipelineAggregatorFactories = builder.getPipelineAggregatorFactories(); + Collection pipelineAggregatorFactories = builder.getPipelineAggregatorFactories(); assertThat(pipelineAggregatorFactories.size(), equalTo(1)); expectThrows(UnsupportedOperationException.class, () -> pipelineAggregatorFactories.add(PipelineAggregatorBuilders.avgBucket("bar", "path2"))); @@ -269,7 +269,7 @@ public void testRewrite() throws Exception { AggregatorFactories.Builder rewritten = builder .rewrite(new QueryRewriteContext(xContentRegistry, null, null, () -> 0L)); assertNotSame(builder, rewritten); - Set aggregatorFactories = rewritten.getAggregatorFactories(); + Collection aggregatorFactories = rewritten.getAggregatorFactories(); assertEquals(1, aggregatorFactories.size()); assertThat(aggregatorFactories.iterator().next(), instanceOf(FilterAggregationBuilder.class)); FilterAggregationBuilder rewrittenFilterAggBuilder = (FilterAggregationBuilder) aggregatorFactories.iterator().next(); 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 b659de4be07f9..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,13 +35,13 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Random; -import java.util.Set; import java.util.concurrent.TimeUnit; /** @@ -550,7 +550,7 @@ void validateAggregations() { throw ExceptionsHelper.badRequestException( Messages.getMessage(Messages.DATAFEED_CONFIG_CANNOT_USE_SCRIPT_FIELDS_WITH_AGGS)); } - Set aggregatorFactories = aggregations.getAggregatorFactories(); + Collection aggregatorFactories = aggregations.getAggregatorFactories(); if (aggregatorFactories.isEmpty()) { throw ExceptionsHelper.badRequestException(Messages.DATAFEED_AGGREGATIONS_REQUIRES_DATE_HISTOGRAM); } @@ -561,7 +561,7 @@ void validateAggregations() { checkHistogramIntervalIsPositive(histogramAggregation); } - private static void checkNoMoreHistogramAggregations(Set 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 c1fdb524db264..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.Set; +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(Set aggregations) { + public static AggregationBuilder getHistogramAggregation(Collection aggregations) { if (aggregations.isEmpty()) { throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.DATAFEED_AGGREGATIONS_REQUIRES_DATE_HISTOGRAM)); } From 3d777c17cfa92545c69ea870e7fc51d3de79e3f7 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Wed, 26 Sep 2018 08:16:33 +0200 Subject: [PATCH 07/12] rewrite IT to not depend on order --- .../aggregation/AggregationProfilerIT.java | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) 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()); From 2b5f938f3fcf1b80d70a5914213451c60f7d2ac2 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Wed, 26 Sep 2018 16:04:04 +0200 Subject: [PATCH 08/12] change HashSet into LinkedHashSet to preserve order --- .../search/aggregations/AggregatorFactories.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 a46b76d6bb25e..5bfb575bee892 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -42,6 +42,7 @@ 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; @@ -238,8 +239,11 @@ public int countPipelineAggregators() { public static class Builder implements Writeable, ToXContentObject { private final Set names = new HashSet<>(); - private final Collection aggregationBuilders = new HashSet<>(); - private final Collection pipelineAggregatorBuilders = new HashSet<>(); + + // 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; /** From 4e17f46fedac35b373ed40572035423cf06cee6e Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Thu, 27 Sep 2018 08:55:34 +0200 Subject: [PATCH 09/12] improve test coverage of BaseAggregationTestCase --- .../search/aggregations/metrics/AvgTests.java | 2 +- .../metrics/CardinalityTests.java | 2 +- .../metrics/ExtendedStatsTests.java | 2 +- .../search/aggregations/metrics/MaxTests.java | 2 +- .../search/aggregations/metrics/MinTests.java | 2 +- .../aggregations/metrics/StatsTests.java | 2 +- .../search/aggregations/metrics/SumTests.java | 2 +- .../aggregations/metrics/TopHitsTests.java | 2 +- .../aggregations/metrics/ValueCountTests.java | 2 +- .../aggregations/BaseAggregationTestCase.java | 73 +++++++++++++++++++ 10 files changed, 82 insertions(+), 9 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgTests.java index 5e1c0a4ebc346..3c7d0a32ceb0a 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgTests.java @@ -23,7 +23,7 @@ public class AvgTests extends AbstractNumericMetricTestCase 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. @@ -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; + } } From c8ef7c41d252a83cebc1099e9c0e4d7b8b15e15e Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Thu, 27 Sep 2018 09:43:28 +0200 Subject: [PATCH 10/12] fix more weak namings --- .../search/aggregations/bucket/DateRangeTests.java | 2 +- .../search/aggregations/bucket/HistogramTests.java | 2 +- .../elasticsearch/search/aggregations/bucket/MissingTests.java | 2 +- .../bucket/sampler/DiversifiedAggregationBuilderTests.java | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) 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 Date: Thu, 27 Sep 2018 10:36:20 +0200 Subject: [PATCH 11/12] more foo refactorings --- .../search/aggregations/bucket/GeoDistanceRangeTests.java | 2 +- .../elasticsearch/search/aggregations/bucket/RangeTests.java | 2 +- .../elasticsearch/search/aggregations/bucket/SamplerTests.java | 2 +- .../aggregations/bucket/histogram/DateHistogramTests.java | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) 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 Date: Thu, 27 Sep 2018 11:13:50 +0200 Subject: [PATCH 12/12] another foo --- .../elasticsearch/search/aggregations/bucket/IpRangeTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeTests.java index 1506da24f78d0..56587c78708c1 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeTests.java @@ -47,7 +47,7 @@ private static String randomIp(boolean v4) { @Override protected IpRangeAggregationBuilder createTestAggregatorBuilder() { int numRanges = randomIntBetween(1, 10); - IpRangeAggregationBuilder factory = new IpRangeAggregationBuilder("foo"); + IpRangeAggregationBuilder factory = new IpRangeAggregationBuilder(randomAlphaOfLengthBetween(3, 10)); for (int i = 0; i < numRanges; i++) { String key = null; if (randomBoolean()) {