From b01e3f0d3b35f15032ec90f2838d51873b961566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 5 Apr 2016 13:06:47 +0200 Subject: [PATCH] Add shuffling xContent to aggregation tests --- .../bucket/filters/FiltersAggregator.java | 7 +++- .../filters/FiltersAggregatorBuilder.java | 5 ++- .../tophits/TopHitsAggregatorBuilder.java | 12 +++--- .../tophits/TopHitsAggregatorFactory.java | 5 ++- .../pipeline/PipelineAggregatorBuilder.java | 6 +-- .../bucketscript/BucketScriptParser.java | 1 + ...BucketScriptPipelineAggregatorBuilder.java | 8 ++-- .../bucketselector/BucketSelectorParser.java | 1 + ...cketSelectorPipelineAggregatorBuilder.java | 10 +++-- .../aggregations/BaseAggregationTestCase.java | 12 +++++- .../BasePipelineAggregationTestCase.java | 14 +++++-- .../search/aggregations/bucket/FiltersIT.java | 38 ++++++++++++------- 12 files changed, 82 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregator.java index 35cc6cd2b49..38b2ba390e8 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregator.java @@ -57,7 +57,7 @@ public class FiltersAggregator extends BucketsAggregator { public static final ParseField OTHER_BUCKET_FIELD = new ParseField("other_bucket"); public static final ParseField OTHER_BUCKET_KEY_FIELD = new ParseField("other_bucket_key"); - public static class KeyedFilter implements Writeable, ToXContent { + public static class KeyedFilter implements Writeable, ToXContent, Comparable { static final KeyedFilter PROTOTYPE = new KeyedFilter("", EmptyQueryBuilder.PROTOTYPE); private final String key; @@ -122,6 +122,11 @@ public class FiltersAggregator extends BucketsAggregator { return Objects.equals(key, other.key) && Objects.equals(filter, other.filter); } + + @Override + public int compareTo(KeyedFilter o) { + return this.key.compareTo(o.key); + } } private final String[] keys; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregatorBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregatorBuilder.java index ad683e134e5..d9cd8ebf4d2 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregatorBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregatorBuilder.java @@ -25,14 +25,15 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.query.EmptyQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.search.aggregations.AggregatorBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; +import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.bucket.filters.FiltersAggregator.KeyedFilter; import org.elasticsearch.search.aggregations.support.AggregationContext; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Objects; @@ -57,6 +58,8 @@ public class FiltersAggregatorBuilder extends AggregatorBuilder filters) { super(name, InternalFilters.TYPE); + // internally we want to have a fixed order of filters, regardless of the order of the filters in the request + Collections.sort(filters); this.filters = filters; this.keyed = true; } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorBuilder.java index c4dd8452cbd..009a7cf1e61 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorBuilder.java @@ -42,8 +42,10 @@ import org.elasticsearch.search.sort.SortOrder; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.Set; public class TopHitsAggregatorBuilder extends AggregatorBuilder { @@ -58,7 +60,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder fieldNames; private List fieldDataFields; - private List scriptFields; + private Set scriptFields; private FetchSourceContext fetchSourceContext; public TopHitsAggregatorBuilder(String name) { @@ -378,7 +380,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder(); + scriptFields = new HashSet<>(); } scriptFields.add(new ScriptField(name, script, ignoreFailure)); return this; @@ -389,7 +391,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder(); + this.scriptFields = new HashSet<>(); } this.scriptFields.addAll(scriptFields); return this; @@ -398,7 +400,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder scriptFields() { + public Set scriptFields() { return scriptFields; } @@ -541,7 +543,7 @@ public class TopHitsAggregatorBuilder extends AggregatorBuilder scriptFields = new ArrayList<>(size); + Set scriptFields = new HashSet<>(size); for (int i = 0; i < size; i++) { scriptFields.add(ScriptField.PROTOTYPE.readFrom(in)); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java index 5b16301d3be..b24adb04fe0 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java @@ -42,6 +42,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; public class TopHitsAggregatorFactory extends AggregatorFactory { @@ -54,12 +55,12 @@ public class TopHitsAggregatorFactory extends AggregatorFactory fieldNames; private final List fieldDataFields; - private final List scriptFields; + private final Set scriptFields; private final FetchSourceContext fetchSourceContext; public TopHitsAggregatorFactory(String name, Type type, int from, int size, boolean explain, boolean version, boolean trackScores, List> sorts, HighlightBuilder highlightBuilder, List fieldNames, List fieldDataFields, - List scriptFields, FetchSourceContext fetchSourceContext, AggregationContext context, AggregatorFactory parent, + Set scriptFields, FetchSourceContext fetchSourceContext, AggregationContext context, AggregatorFactory parent, AggregatorFactories.Builder subFactories, Map metaData) throws IOException { super(name, type, context, parent, subFactories, metaData); this.from = from; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregatorBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregatorBuilder.java index b77cd7a13b1..73adf489f97 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregatorBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregatorBuilder.java @@ -39,9 +39,9 @@ import java.util.Objects; public abstract class PipelineAggregatorBuilder> extends ToXContentToBytes implements NamedWriteable>, ToXContent { - protected String name; - protected String type; - protected String[] bucketsPaths; + protected final String name; + protected final String type; + protected final String[] bucketsPaths; protected Map metaData; /** diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptParser.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptParser.java index 9e34a311767..8b1fb8f06a2 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptParser.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptParser.java @@ -27,6 +27,7 @@ import org.elasticsearch.script.Script; import org.elasticsearch.script.Script.ScriptField; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; + import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregatorBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregatorBuilder.java index 85bf0ee3332..6d3e9e2ddd8 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregatorBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregatorBuilder.java @@ -24,9 +24,9 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.script.Script; import org.elasticsearch.script.Script.ScriptField; +import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregatorBuilder; -import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.support.format.ValueFormat; import org.elasticsearch.search.aggregations.support.format.ValueFormatter; @@ -34,8 +34,9 @@ import java.io.IOException; import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.Objects; import java.util.Map.Entry; +import java.util.Objects; +import java.util.TreeMap; public class BucketScriptPipelineAggregatorBuilder extends PipelineAggregatorBuilder { @@ -48,7 +49,8 @@ public class BucketScriptPipelineAggregatorBuilder extends PipelineAggregatorBui private GapPolicy gapPolicy = GapPolicy.SKIP; public BucketScriptPipelineAggregatorBuilder(String name, Map bucketsPathsMap, Script script) { - super(name, BucketScriptPipelineAggregator.TYPE.name(), bucketsPathsMap.values().toArray(new String[bucketsPathsMap.size()])); + super(name, BucketScriptPipelineAggregator.TYPE.name(), new TreeMap<>(bucketsPathsMap).values() + .toArray(new String[bucketsPathsMap.size()])); this.bucketsPathsMap = bucketsPathsMap; this.script = script; } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorParser.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorParser.java index 084f79da306..ffd43795dc2 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorParser.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorParser.java @@ -27,6 +27,7 @@ import org.elasticsearch.script.Script; import org.elasticsearch.script.Script.ScriptField; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; + import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregatorBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregatorBuilder.java index 0def3374323..0e2ca8e6928 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregatorBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketselector/BucketSelectorPipelineAggregatorBuilder.java @@ -24,17 +24,18 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.script.Script; import org.elasticsearch.script.Script.ScriptField; +import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregatorBuilder; -import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.bucketscript.BucketScriptParser; import java.io.IOException; import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.Objects; import java.util.Map.Entry; +import java.util.Objects; +import java.util.TreeMap; public class BucketSelectorPipelineAggregatorBuilder extends PipelineAggregatorBuilder { @@ -43,10 +44,11 @@ public class BucketSelectorPipelineAggregatorBuilder extends PipelineAggregatorB private Script script; private GapPolicy gapPolicy = GapPolicy.SKIP; - private Map bucketsPathsMap; + private final Map bucketsPathsMap; public BucketSelectorPipelineAggregatorBuilder(String name, Map bucketsPathsMap, Script script) { - super(name, BucketSelectorPipelineAggregator.TYPE.name(), bucketsPathsMap.values().toArray(new String[bucketsPathsMap.size()])); + super(name, BucketSelectorPipelineAggregator.TYPE.name(), new TreeMap<>(bucketsPathsMap).values() + .toArray(new String[bucketsPathsMap.size()])); this.bucketsPathsMap = bucketsPathsMap; this.script = script; } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java index 79de06ff9b7..0a458428391 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/BaseAggregationTestCase.java @@ -36,8 +36,11 @@ import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsModule; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.env.Environment; import org.elasticsearch.env.EnvironmentModule; import org.elasticsearch.index.Index; @@ -215,8 +218,13 @@ public abstract class BaseAggregationTestCase> public void testFromXContent() throws IOException { AB testAgg = createTestAggregatorBuilder(); AggregatorFactories.Builder factoriesBuilder = AggregatorFactories.builder().addAggregator(testAgg); - String contentString = factoriesBuilder.toString(); - XContentParser parser = XContentFactory.xContent(contentString).createParser(contentString); + XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values())); + if (randomBoolean()) { + builder.prettyPrint(); + } + factoriesBuilder.toXContent(builder, ToXContent.EMPTY_PARAMS); + XContentBuilder shuffled = shuffleXContent(builder, Collections.emptySet()); + XContentParser parser = XContentFactory.xContent(shuffled.bytes()).createParser(shuffled.bytes()); QueryParseContext parseContext = new QueryParseContext(queriesRegistry); parseContext.reset(parser); parseContext.parseFieldMatcher(parseFieldMatcher); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java index 05be7e17bdf..16bba36ed77 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java @@ -36,8 +36,11 @@ import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsModule; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.env.Environment; import org.elasticsearch.env.EnvironmentModule; import org.elasticsearch.index.Index; @@ -216,9 +219,14 @@ public abstract class BasePipelineAggregationTestCase emptyFilter = new BoolQueryBuilder(); SearchResponse response = client().prepareSearch("idx") - .addAggregation(filters("tags", new KeyedFilter("all", emptyFilter), new KeyedFilter("tag1", termQuery("tag", "tag1")))) + .addAggregation(filters("tags", randomOrder(new KeyedFilter("all", emptyFilter), + new KeyedFilter("tag1", termQuery("tag", "tag1"))))) .execute().actionGet(); assertSearchResponse(response); @@ -154,8 +156,8 @@ public class FiltersIT extends ESIntegTestCase { public void testWithSubAggregation() throws Exception { SearchResponse response = client().prepareSearch("idx") - .addAggregation(filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")), - new KeyedFilter("tag2", termQuery("tag", "tag2"))).subAggregation(avg("avg_value").field("value"))) + .addAggregation(filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")), + new KeyedFilter("tag2", termQuery("tag", "tag2")))).subAggregation(avg("avg_value").field("value"))) .execute().actionGet(); assertSearchResponse(response); @@ -254,9 +256,9 @@ public class FiltersIT extends ESIntegTestCase { try { client().prepareSearch("idx") .addAggregation( - filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")), new KeyedFilter("tag2", termQuery("tag", "tag2"))) - .subAggregation(avg("avg_value")) - ) + filters("tags", + randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")), + new KeyedFilter("tag2", termQuery("tag", "tag2")))).subAggregation(avg("avg_value"))) .execute().actionGet(); fail("expected execution to fail - an attempt to have a context based numeric sub-aggregation, but there is not value source" + @@ -314,8 +316,8 @@ public class FiltersIT extends ESIntegTestCase { public void testOtherBucket() throws Exception { SearchResponse response = client().prepareSearch("idx").addAggregation( - filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")), new KeyedFilter("tag2", termQuery("tag", "tag2"))) - .otherBucket(true)) + filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")), + new KeyedFilter("tag2", termQuery("tag", "tag2")))).otherBucket(true)) .execute().actionGet(); assertSearchResponse(response); @@ -341,8 +343,8 @@ public class FiltersIT extends ESIntegTestCase { public void testOtherNamedBucket() throws Exception { SearchResponse response = client().prepareSearch("idx") - .addAggregation(filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")), - new KeyedFilter("tag2", termQuery("tag", "tag2"))).otherBucket(true).otherBucketKey("foobar")) + .addAggregation(filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")), + new KeyedFilter("tag2", termQuery("tag", "tag2")))).otherBucket(true).otherBucketKey("foobar")) .execute().actionGet(); assertSearchResponse(response); @@ -397,8 +399,8 @@ public class FiltersIT extends ESIntegTestCase { public void testOtherWithSubAggregation() throws Exception { SearchResponse response = client().prepareSearch("idx") - .addAggregation(filters("tags", new KeyedFilter("tag1", termQuery("tag", "tag1")), - new KeyedFilter("tag2", termQuery("tag", "tag2"))).otherBucket(true) + .addAggregation(filters("tags", randomOrder(new KeyedFilter("tag1", termQuery("tag", "tag1")), + new KeyedFilter("tag2", termQuery("tag", "tag2")))).otherBucket(true) .subAggregation(avg("avg_value").field("value"))) .execute().actionGet(); @@ -485,4 +487,14 @@ public class FiltersIT extends ESIntegTestCase { assertThat(other.getDocCount(), is(0L)); } + private static KeyedFilter[] randomOrder(KeyedFilter... filters) { + for (int i = 0; i < filters.length; i++) { + KeyedFilter tmp = filters[i]; + int index = randomInt(filters.length - 1); + filters[i] = filters[index]; + filters[index] = tmp; + } + return filters; + } + }