From 0bae19cd56785d312e0d19d18ad20de5f3c6241a Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Wed, 7 Nov 2018 10:28:27 +0100 Subject: [PATCH 1/2] Allow unmapped fields in composite aggregations Today the `composite` aggregation throws an error if a source targets an unmapped field and `missing_bucket` is set to false. Documents without a value for a source cannot produce any bucket if `missing_bucket` is not activated so the error is a shortcut to say that the response will be empty. However this is not consistent with the `terms` aggregation which accepts unmapped field by default even if the response is also guaranteed to be empty. This commit removes this restriction, if a source contains an unmapped field we now return an empty response (no buckets). Closes #35317 --- .../CompositeValuesSourceBuilder.java | 7 -- .../composite/CompositeAggregatorTests.java | 85 ++++++++++++++++--- 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java index 32a94f59be5c2..35b0f5ef51800 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java @@ -304,13 +304,6 @@ public String format() { public final CompositeValuesSourceConfig build(SearchContext context) throws IOException { ValuesSourceConfig config = ValuesSourceConfig.resolve(context.getQueryShardContext(), valueType, field, script, null,null, format); - - if (config.unmapped() && field != null && missingBucket == false) { - // this source cannot produce any values so we refuse to build - // since composite buckets are not created on null values by default. - throw new QueryShardException(context.getQueryShardContext(), - "failed to find field [" + field + "] and [missing_bucket] is not set"); - } return innerBuild(context, config); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java index 52f6e4227e7cd..46a998902eeee 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java @@ -130,16 +130,81 @@ public void tearDown() throws Exception { } public void testUnmappedField() throws Exception { - TermsValuesSourceBuilder terms = new TermsValuesSourceBuilder(randomAlphaOfLengthBetween(5, 10)) - .field("unknown"); - CompositeAggregationBuilder builder = new CompositeAggregationBuilder("test", Collections.singletonList(terms)); - IndexSearcher searcher = new IndexSearcher(new MultiReader()); - QueryShardException exc = - expectThrows(QueryShardException.class, () -> createAggregatorFactory(builder, searcher)); - assertThat(exc.getMessage(), containsString("failed to find field [unknown] and [missing_bucket] is not set")); - // should work when missing_bucket is set - terms.missingBucket(true); - createAggregatorFactory(builder, searcher); + final List>> dataset = new ArrayList<>(); + dataset.addAll( + Arrays.asList( + createDocument("keyword", "a"), + createDocument("keyword", "c"), + createDocument("keyword", "a"), + createDocument("keyword", "d"), + createDocument("keyword", "c") + ) + ); + testSearchCase(Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("keyword")), dataset, + () -> new CompositeAggregationBuilder("name", + Arrays.asList( + new TermsValuesSourceBuilder("unmapped").field("unmapped") + ) + ), + (result) -> { + assertEquals(0, result.getBuckets().size()); + } + ); + + testSearchCase(Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("keyword")), dataset, + () -> new CompositeAggregationBuilder("name", + Arrays.asList( + new TermsValuesSourceBuilder("unmapped").field("unmapped").missingBucket(true) + ) + ), + (result) -> { + assertEquals(1, result.getBuckets().size()); + assertEquals("{unmapped=null}", result.afterKey().toString()); + assertEquals("{unmapped=null}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(5L, result.getBuckets().get(0).getDocCount()); + } + ); + + testSearchCase(Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("keyword")), dataset, + () -> new CompositeAggregationBuilder("name", + Arrays.asList( + new TermsValuesSourceBuilder("unmapped").field("unmapped").missingBucket(true) + )).aggregateAfter(Collections.singletonMap("unmapped", null)), + (result) -> { + assertEquals(0, result.getBuckets().size()); + } + ); + + testSearchCase(Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("keyword")), dataset, + () -> new CompositeAggregationBuilder("name", + Arrays.asList( + new TermsValuesSourceBuilder("keyword").field("keyword"), + new TermsValuesSourceBuilder("unmapped").field("unmapped") + ) + ), + (result) -> { + assertEquals(0, result.getBuckets().size()); + } + ); + + testSearchCase(Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("keyword")), dataset, + () -> new CompositeAggregationBuilder("name", + Arrays.asList( + new TermsValuesSourceBuilder("keyword").field("keyword"), + new TermsValuesSourceBuilder("unmapped").field("unmapped").missingBucket(true) + ) + ), + (result) -> { + assertEquals(3, result.getBuckets().size()); + assertEquals("{keyword=d, unmapped=null}", result.afterKey().toString()); + assertEquals("{keyword=a, unmapped=null}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(0).getDocCount()); + assertEquals("{keyword=c, unmapped=null}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(1).getDocCount()); + assertEquals("{keyword=d, unmapped=null}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(2).getDocCount()); + } + ); } public void testWithKeyword() throws Exception { From 7e67d59c296838b4b7419f62392b7d2335567562 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Wed, 7 Nov 2018 10:49:05 +0100 Subject: [PATCH 2/2] unused import --- .../bucket/composite/CompositeValuesSourceBuilder.java | 1 - .../aggregations/bucket/composite/CompositeAggregatorTests.java | 2 -- 2 files changed, 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java index 35b0f5ef51800..992d050b73cab 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java @@ -25,7 +25,6 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.index.query.QueryShardException; import org.elasticsearch.script.Script; import org.elasticsearch.search.aggregations.support.ValueType; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java index 46a998902eeee..f2cb91673ce10 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java @@ -30,7 +30,6 @@ import org.apache.lucene.document.StringField; import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexReader; -import org.apache.lucene.index.MultiReader; import org.apache.lucene.index.RandomIndexWriter; import org.apache.lucene.search.DocValuesFieldExistsQuery; import org.apache.lucene.search.IndexSearcher; @@ -47,7 +46,6 @@ import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.NumberFieldMapper; -import org.elasticsearch.index.query.QueryShardException; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;