From b19398d42d3c19c54f74f93b17c5ef0bf3800c14 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 6 Apr 2020 15:42:15 -0400 Subject: [PATCH] Allow terms agg to default to depth first If you didn't explictly set `global_ordinals` execution mode we were never collecting the information that we needed to select `depth_first` based on the request so we were always defaulting to `breadth_first`. This fixes it so we collect the information. --- .../bucket/terms/TermsAggregatorFactory.java | 2 +- .../bucket/terms/TermsAggregatorTests.java | 23 ++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java index d6d393266881b..ed88022b3bdc4 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java @@ -102,10 +102,10 @@ public Aggregator build(String name, if (valuesSource instanceof ValuesSource.Bytes.WithOrdinals == false) { execution = ExecutionMode.MAP; } - final long maxOrd = execution == ExecutionMode.GLOBAL_ORDINALS ? getMaxOrd(valuesSource, context.searcher()) : -1; if (execution == null) { execution = ExecutionMode.GLOBAL_ORDINALS; } + final long maxOrd = execution == ExecutionMode.GLOBAL_ORDINALS ? getMaxOrd(valuesSource, context.searcher()) : -1; if (subAggCollectMode == null) { subAggCollectMode = SubAggCollectionMode.DEPTH_FIRST; // TODO can we remove concept of AggregatorFactories.EMPTY? diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java index e48f6df687ef2..f345d1367a031 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java @@ -139,7 +139,7 @@ protected List getSupportedValuesSourceTypes() { CoreValuesSourceType.BOOLEAN); } - public void testGlobalOrdinalsExecutionHint() throws Exception { + public void testUsesGlobalOrdinalsByDefault() throws Exception { randomizeAggregatorImpl = false; Directory directory = newDirectory(); @@ -150,8 +150,7 @@ public void testGlobalOrdinalsExecutionHint() throws Exception { IndexSearcher indexSearcher = new IndexSearcher(indexReader); TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name").userValueTypeHint(ValueType.STRING) - .field("string") - .collectMode(Aggregator.SubAggCollectionMode.BREADTH_FIRST); + .field("string"); MappedFieldType fieldType = new KeywordFieldMapper.KeywordFieldType(); fieldType.setName("string"); fieldType.setHasDocValues(true); @@ -161,11 +160,29 @@ public void testGlobalOrdinalsExecutionHint() throws Exception { GlobalOrdinalsStringTermsAggregator globalAgg = (GlobalOrdinalsStringTermsAggregator) aggregator; assertFalse(globalAgg.remapGlobalOrds()); + // Infers depth_first because the maxOrd is 0 which is less than the size aggregationBuilder .subAggregation(AggregationBuilders.cardinality("card").field("string")); aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType); assertThat(aggregator, instanceOf(GlobalOrdinalsStringTermsAggregator.class)); globalAgg = (GlobalOrdinalsStringTermsAggregator) aggregator; + assertThat(globalAgg.collectMode, equalTo(Aggregator.SubAggCollectionMode.DEPTH_FIRST)); + assertTrue(globalAgg.remapGlobalOrds()); + + aggregationBuilder + .collectMode(Aggregator.SubAggCollectionMode.DEPTH_FIRST); + aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType); + assertThat(aggregator, instanceOf(GlobalOrdinalsStringTermsAggregator.class)); + globalAgg = (GlobalOrdinalsStringTermsAggregator) aggregator; + assertThat(globalAgg.collectMode, equalTo(Aggregator.SubAggCollectionMode.DEPTH_FIRST)); + assertTrue(globalAgg.remapGlobalOrds()); + + aggregationBuilder + .collectMode(Aggregator.SubAggCollectionMode.BREADTH_FIRST); + aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType); + assertThat(aggregator, instanceOf(GlobalOrdinalsStringTermsAggregator.class)); + globalAgg = (GlobalOrdinalsStringTermsAggregator) aggregator; + assertThat(globalAgg.collectMode, equalTo(Aggregator.SubAggCollectionMode.BREADTH_FIRST)); assertFalse(globalAgg.remapGlobalOrds()); aggregationBuilder