Skip to content

Commit

Permalink
Allow unmapped fields in composite aggregations (elastic#35331)
Browse files Browse the repository at this point in the history
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 elastic#35317
  • Loading branch information
jimczi authored and pgomulka committed Nov 13, 2018
1 parent e8fb974 commit 0fa41e4
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -304,13 +303,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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -130,16 +128,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<Map<String, List<Object>>> 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 {
Expand Down

0 comments on commit 0fa41e4

Please sign in to comment.