From 988a09e6daffe54294f8b5d103ac95224b0f33a5 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Thu, 11 Jan 2018 15:16:18 +0000 Subject: [PATCH 1/2] Adds metadata to rewritten aggregations Previous to this change, if any filters in the filters aggregation were rewritten, the rewritten version of the FiltersAggregationBuilder would not contain the metadata form the original. This is because `AbstractAggregationBuilder.getMetadata()` returns an empty map when not metadata is set. Closes #28170 --- .../search/aggregations/AggregationBuilder.java | 2 +- .../search/aggregations/FiltersAggsRewriteIT.java | 6 ++++++ .../search/aggregations/bucket/FiltersTests.java | 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-) 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 99bf9be683ee3..0188836ac7a73 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java @@ -101,7 +101,7 @@ public final AggregationBuilder rewrite(QueryRewriteContext context) throws IOEx if (rewritten == this) { return rewritten; } - if (getMetaData() != null && rewritten.getMetaData() == null) { + if (getMetaData() != null && (rewritten.getMetaData() == null || rewritten.getMetaData().isEmpty())) { rewritten.setMetaData(getMetaData()); } AggregatorFactories.Builder rewrittenSubAggs = factoriesBuilder.rewrite(context); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/FiltersAggsRewriteIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/FiltersAggsRewriteIT.java index bb4c3a2a5eb0f..ce5e4a694f279 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/FiltersAggsRewriteIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/FiltersAggsRewriteIT.java @@ -32,6 +32,8 @@ import org.elasticsearch.test.ESSingleNodeTestCase; import java.io.IOException; +import java.util.HashMap; +import java.util.Map; public class FiltersAggsRewriteIT extends ESSingleNodeTestCase { @@ -58,10 +60,14 @@ public void testWrapperQueryIsRewritten() throws IOException { } FiltersAggregationBuilder builder = new FiltersAggregationBuilder("titles", new FiltersAggregator.KeyedFilter("titleterms", new WrapperQueryBuilder(bytesReference))); + Map metadata = new HashMap<>(); + metadata.put(randomAlphaOfLengthBetween(1, 20), randomAlphaOfLengthBetween(1, 20)); + builder.setMetaData(metadata); SearchResponse searchResponse = client().prepareSearch("test").setSize(0).addAggregation(builder).get(); assertEquals(3, searchResponse.getHits().getTotalHits()); InternalFilters filters = searchResponse.getAggregations().get("titles"); assertEquals(1, filters.getBuckets().size()); assertEquals(2, filters.getBuckets().get(0).getDocCount()); + assertEquals(metadata, filters.getMetaData()); } } 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 7e63bbb6f3855..e0cd490134f14 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 @@ -35,6 +35,7 @@ import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregator.KeyedFilter; import java.io.IOException; +import java.util.Collections; import static org.hamcrest.Matchers.instanceOf; @@ -123,6 +124,7 @@ public void testOtherBucket() throws IOException { public void testRewrite() throws IOException { // test non-keyed filter that doesn't rewrite AggregationBuilder original = new FiltersAggregationBuilder("my-agg", new MatchAllQueryBuilder()); + original.setMetaData(Collections.singletonMap(randomAlphaOfLengthBetween(1, 20), randomAlphaOfLengthBetween(1, 20))); AggregationBuilder rewritten = original.rewrite(new QueryRewriteContext(xContentRegistry(), null, null, () -> 0L)); assertSame(original, rewritten); From 5fcbc994176edcb05162afde7b657b87132bc62b Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Fri, 12 Jan 2018 15:12:39 +0000 Subject: [PATCH 2/2] Always set metadata when rewritten --- .../elasticsearch/search/aggregations/AggregationBuilder.java | 4 +--- 1 file changed, 1 insertion(+), 3 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 0188836ac7a73..80d8277f4cab2 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java @@ -101,9 +101,7 @@ public final AggregationBuilder rewrite(QueryRewriteContext context) throws IOEx if (rewritten == this) { return rewritten; } - if (getMetaData() != null && (rewritten.getMetaData() == null || rewritten.getMetaData().isEmpty())) { - rewritten.setMetaData(getMetaData()); - } + rewritten.setMetaData(getMetaData()); AggregatorFactories.Builder rewrittenSubAggs = factoriesBuilder.rewrite(context); rewritten.subAggregations(rewrittenSubAggs); return rewritten;