Skip to content

Commit 4c6184b

Browse files
authored
Allow terms agg to default to depth first (#54845)
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.
1 parent 6182db5 commit 4c6184b

File tree

2 files changed

+21
-4
lines changed

2 files changed

+21
-4
lines changed

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,10 +102,10 @@ public Aggregator build(String name,
102102
if (valuesSource instanceof ValuesSource.Bytes.WithOrdinals == false) {
103103
execution = ExecutionMode.MAP;
104104
}
105-
final long maxOrd = execution == ExecutionMode.GLOBAL_ORDINALS ? getMaxOrd(valuesSource, context.searcher()) : -1;
106105
if (execution == null) {
107106
execution = ExecutionMode.GLOBAL_ORDINALS;
108107
}
108+
final long maxOrd = execution == ExecutionMode.GLOBAL_ORDINALS ? getMaxOrd(valuesSource, context.searcher()) : -1;
109109
if (subAggCollectMode == null) {
110110
subAggCollectMode = SubAggCollectionMode.DEPTH_FIRST;
111111
// TODO can we remove concept of AggregatorFactories.EMPTY?

server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ protected List<ValuesSourceType> getSupportedValuesSourceTypes() {
139139
CoreValuesSourceType.BOOLEAN);
140140
}
141141

142-
public void testGlobalOrdinalsExecutionHint() throws Exception {
142+
public void testUsesGlobalOrdinalsByDefault() throws Exception {
143143
randomizeAggregatorImpl = false;
144144

145145
Directory directory = newDirectory();
@@ -150,8 +150,7 @@ public void testGlobalOrdinalsExecutionHint() throws Exception {
150150
IndexSearcher indexSearcher = new IndexSearcher(indexReader);
151151

152152
TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name").userValueTypeHint(ValueType.STRING)
153-
.field("string")
154-
.collectMode(Aggregator.SubAggCollectionMode.BREADTH_FIRST);
153+
.field("string");
155154
MappedFieldType fieldType = new KeywordFieldMapper.KeywordFieldType();
156155
fieldType.setName("string");
157156
fieldType.setHasDocValues(true);
@@ -161,11 +160,29 @@ public void testGlobalOrdinalsExecutionHint() throws Exception {
161160
GlobalOrdinalsStringTermsAggregator globalAgg = (GlobalOrdinalsStringTermsAggregator) aggregator;
162161
assertFalse(globalAgg.remapGlobalOrds());
163162

163+
// Infers depth_first because the maxOrd is 0 which is less than the size
164164
aggregationBuilder
165165
.subAggregation(AggregationBuilders.cardinality("card").field("string"));
166166
aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
167167
assertThat(aggregator, instanceOf(GlobalOrdinalsStringTermsAggregator.class));
168168
globalAgg = (GlobalOrdinalsStringTermsAggregator) aggregator;
169+
assertThat(globalAgg.collectMode, equalTo(Aggregator.SubAggCollectionMode.DEPTH_FIRST));
170+
assertTrue(globalAgg.remapGlobalOrds());
171+
172+
aggregationBuilder
173+
.collectMode(Aggregator.SubAggCollectionMode.DEPTH_FIRST);
174+
aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
175+
assertThat(aggregator, instanceOf(GlobalOrdinalsStringTermsAggregator.class));
176+
globalAgg = (GlobalOrdinalsStringTermsAggregator) aggregator;
177+
assertThat(globalAgg.collectMode, equalTo(Aggregator.SubAggCollectionMode.DEPTH_FIRST));
178+
assertTrue(globalAgg.remapGlobalOrds());
179+
180+
aggregationBuilder
181+
.collectMode(Aggregator.SubAggCollectionMode.BREADTH_FIRST);
182+
aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
183+
assertThat(aggregator, instanceOf(GlobalOrdinalsStringTermsAggregator.class));
184+
globalAgg = (GlobalOrdinalsStringTermsAggregator) aggregator;
185+
assertThat(globalAgg.collectMode, equalTo(Aggregator.SubAggCollectionMode.BREADTH_FIRST));
169186
assertFalse(globalAgg.remapGlobalOrds());
170187

171188
aggregationBuilder

0 commit comments

Comments
 (0)