Skip to content

Commit 4b83879

Browse files
nik9000nik9000
authored
Aggs: Let terms run in global ords mode no match (elastic#124782) (elastic#125425)
Allows the `terms` agg to run with global ords if the top level query matches no docs *but* the agg is configured to collect buckets with 0 docs.
1 parent bbde3a0 commit 4b83879

File tree

4 files changed

+146
-9
lines changed

4 files changed

+146
-9
lines changed

‎docs/changelog/124782.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 124782
2+
summary: "Aggs: Let terms run in global ords mode no match"
3+
area: Aggregations
4+
type: bug
5+
issues: []

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,13 @@ private static TermsAggregatorSupplier bytesSupplier() {
109109
ExecutionMode execution = null;
110110
if (executionHint != null) {
111111
execution = ExecutionMode.fromString(executionHint);
112+
} else {
113+
if (matchNoDocs(context, parent) && bucketCountThresholds.getMinDocCount() > 0) {
114+
execution = ExecutionMode.MAP;
115+
}
112116
}
113117
// In some cases, using ordinals is just not supported: override it
114-
if (valuesSource.hasOrdinals() == false || matchNoDocs(context, parent)) {
118+
if (valuesSource.hasOrdinals() == false) {
115119
execution = ExecutionMode.MAP;
116120
}
117121
if (execution == null) {

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

Lines changed: 100 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@
141141
import static org.elasticsearch.search.aggregations.PipelineAggregatorBuilders.bucketScript;
142142
import static org.elasticsearch.test.MapMatcher.assertMap;
143143
import static org.elasticsearch.test.MapMatcher.matchesMap;
144+
import static org.hamcrest.Matchers.anyOf;
144145
import static org.hamcrest.Matchers.closeTo;
145146
import static org.hamcrest.Matchers.either;
146147
import static org.hamcrest.Matchers.equalTo;
@@ -286,8 +287,18 @@ public void testSimple() throws Exception {
286287
}
287288

288289
public void testMatchNoDocsQuery() throws Exception {
290+
for (TermsAggregatorFactory.ExecutionMode executionMode : TermsAggregatorFactory.ExecutionMode.values()) {
291+
testMatchNoDocsQuery(executionMode);
292+
}
293+
testMatchNoDocsQuery(null);
294+
}
295+
296+
private void testMatchNoDocsQuery(TermsAggregatorFactory.ExecutionMode hint) throws Exception {
289297
MappedFieldType fieldType = new KeywordFieldMapper.KeywordFieldType("string", randomBoolean(), true, Collections.emptyMap());
290298
TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name").field("string");
299+
if (hint != null) {
300+
aggregationBuilder.executionHint(hint.toString());
301+
}
291302
CheckedConsumer<RandomIndexWriter, IOException> createIndex = iw -> {
292303
iw.addDocument(doc(fieldType, "a", "b"));
293304
iw.addDocument(doc(fieldType, "", "c", "a"));
@@ -298,11 +309,8 @@ public void testMatchNoDocsQuery() throws Exception {
298309
createIndex,
299310
(InternalTerms<?, ?> result) -> { assertEquals(0, result.getBuckets().size()); },
300311
new AggTestConfig(aggregationBuilder, fieldType).withQuery(new MatchNoDocsQuery())
312+
.withCheckAggregator(checkTopLevelAggregator(hint == null ? TermsAggregatorFactory.ExecutionMode.MAP : hint))
301313
);
302-
303-
debugTestCase(aggregationBuilder, new MatchNoDocsQuery(), createIndex, (result, impl, debug) -> {
304-
assertEquals(impl, MapStringTermsAggregator.class);
305-
}, fieldType);
306314
}
307315

308316
public void testStringShardMinDocCount() throws IOException {
@@ -337,7 +345,6 @@ public void testStringShardZeroMinDocCount() throws IOException {
337345
.executionHint(executionMode.toString())
338346
.size(2)
339347
.minDocCount(0)
340-
.executionHint("map")
341348
.excludeDeletedDocs(true)
342349
.order(BucketOrder.key(true));
343350

@@ -360,7 +367,80 @@ public void testStringShardZeroMinDocCount() throws IOException {
360367
assertEquals("b", result.getBuckets().get(1).getKeyAsString());
361368
}
362369
assertEquals(0L, result.getBuckets().get(1).getDocCount());
363-
}, new AggTestConfig(aggregationBuilder, fieldType).withQuery(new TermQuery(new Term("string", "e"))));
370+
},
371+
new AggTestConfig(aggregationBuilder, fieldType).withQuery(new TermQuery(new Term("string", "e")))
372+
.withCheckAggregator(checkTopLevelAggregator(executionMode))
373+
);
374+
}
375+
376+
{
377+
boolean delete = randomBoolean();
378+
// force single shard/segment
379+
testCase(iw -> {
380+
// force single shard/segment
381+
iw.addDocuments(
382+
Arrays.asList(doc(fieldType, "a"), doc(fieldType, "c", "d"), doc(fieldType, "b", "d"), doc(fieldType, "b"))
383+
);
384+
if (delete) {
385+
iw.deleteDocuments(new TermQuery(new Term("string", "b")));
386+
}
387+
}, (InternalTerms<?, ?> result) -> {
388+
assertEquals(2, result.getBuckets().size());
389+
assertEquals("a", result.getBuckets().get(0).getKeyAsString());
390+
assertEquals(0L, result.getBuckets().get(0).getDocCount());
391+
if (delete) {
392+
assertEquals("c", result.getBuckets().get(1).getKeyAsString());
393+
} else {
394+
assertEquals("b", result.getBuckets().get(1).getKeyAsString());
395+
}
396+
assertEquals(0L, result.getBuckets().get(1).getDocCount());
397+
},
398+
new AggTestConfig(aggregationBuilder, fieldType).withQuery(new TermQuery(new Term("string", "e")))
399+
.withCheckAggregator(checkTopLevelAggregator(executionMode))
400+
);
401+
}
402+
}
403+
}
404+
405+
/**
406+
* Asserts that sane output for {@code min_doc_count: 0} when the top level
407+
* query is {@code match_none}. Especially important is that we respect the
408+
* execution hint in this case. It's a lot faster to collect {@code min_doc_count: 0}
409+
* via ordinals.
410+
*/
411+
public void testStringZeroMinDocCountMatchNone() throws IOException {
412+
MappedFieldType fieldType = new KeywordFieldMapper.KeywordFieldType("string", true, true, Collections.emptyMap());
413+
for (TermsAggregatorFactory.ExecutionMode executionMode : TermsAggregatorFactory.ExecutionMode.values()) {
414+
TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name").field("string")
415+
.executionHint(executionMode.toString())
416+
.size(2)
417+
.minDocCount(0)
418+
.excludeDeletedDocs(true)
419+
.order(BucketOrder.key(true));
420+
421+
{
422+
boolean delete = randomBoolean();
423+
// force single shard/segment
424+
testCase(iw -> {
425+
// force single shard/segment
426+
iw.addDocuments(Arrays.asList(doc(fieldType, "a"), doc(fieldType, "b"), doc(fieldType, "c"), doc(fieldType, "d")));
427+
if (delete) {
428+
iw.deleteDocuments(new TermQuery(new Term("string", "b")));
429+
}
430+
}, (InternalTerms<?, ?> result) -> {
431+
assertEquals(2, result.getBuckets().size());
432+
assertEquals("a", result.getBuckets().get(0).getKeyAsString());
433+
assertEquals(0L, result.getBuckets().get(0).getDocCount());
434+
if (delete) {
435+
assertEquals("c", result.getBuckets().get(1).getKeyAsString());
436+
} else {
437+
assertEquals("b", result.getBuckets().get(1).getKeyAsString());
438+
}
439+
assertEquals(0L, result.getBuckets().get(1).getDocCount());
440+
},
441+
new AggTestConfig(aggregationBuilder, fieldType).withQuery(new MatchNoDocsQuery())
442+
.withCheckAggregator(checkTopLevelAggregator(executionMode))
443+
);
364444
}
365445

366446
{
@@ -384,7 +464,10 @@ public void testStringShardZeroMinDocCount() throws IOException {
384464
assertEquals("b", result.getBuckets().get(1).getKeyAsString());
385465
}
386466
assertEquals(0L, result.getBuckets().get(1).getDocCount());
387-
}, new AggTestConfig(aggregationBuilder, fieldType).withQuery(new TermQuery(new Term("string", "e"))));
467+
},
468+
new AggTestConfig(aggregationBuilder, fieldType).withQuery(new MatchNoDocsQuery())
469+
.withCheckAggregator(checkTopLevelAggregator(executionMode))
470+
);
388471
}
389472
}
390473
}
@@ -2279,4 +2362,14 @@ protected List<ObjectMapper> objectMappers() {
22792362
private String randomHint() {
22802363
return randomFrom(TermsAggregatorFactory.ExecutionMode.values()).toString();
22812364
}
2365+
2366+
private Consumer<Aggregator> checkTopLevelAggregator(TermsAggregatorFactory.ExecutionMode executionMode) {
2367+
return switch (executionMode) {
2368+
case MAP -> a -> assertThat(a, instanceOf(MapStringTermsAggregator.class));
2369+
case GLOBAL_ORDINALS -> a -> assertThat(
2370+
a,
2371+
anyOf(instanceOf(GlobalOrdinalsStringTermsAggregator.class), instanceOf(StringTermsAggregatorFromFilters.class))
2372+
);
2373+
};
2374+
}
22822375
}

‎test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -678,6 +678,7 @@ private <A extends InternalAggregation, C extends Aggregator> A searchAndReduce(
678678
}
679679
assertEquals(shouldBeCached, context.isCacheable());
680680
List<InternalAggregation> internalAggregations = List.of(a.buildTopLevel());
681+
aggTestConfig.checkAggregator().accept(a);
681682
assertRoundTrip(internalAggregations);
682683
internalAggs.add(InternalAggregations.from(internalAggregations));
683684
} finally {
@@ -1696,11 +1697,23 @@ public record AggTestConfig(
16961697

16971698
boolean useLogDocMergePolicy,
16981699
boolean testReductionCancellation,
1700+
Consumer<Aggregator> checkAggregator,
16991701
MappedFieldType... fieldTypes
17001702
) {
17011703

17021704
public AggTestConfig(AggregationBuilder builder, MappedFieldType... fieldTypes) {
1703-
this(new MatchAllDocsQuery(), builder, DEFAULT_MAX_BUCKETS, randomBoolean(), true, randomBoolean(), false, true, fieldTypes);
1705+
this(
1706+
new MatchAllDocsQuery(),
1707+
builder,
1708+
DEFAULT_MAX_BUCKETS,
1709+
randomBoolean(),
1710+
true,
1711+
randomBoolean(),
1712+
false,
1713+
true,
1714+
a -> {},
1715+
fieldTypes
1716+
);
17041717
}
17051718

17061719
public AggTestConfig withQuery(Query query) {
@@ -1713,6 +1726,7 @@ public AggTestConfig withQuery(Query query) {
17131726
incrementalReduce,
17141727
useLogDocMergePolicy,
17151728
testReductionCancellation,
1729+
checkAggregator,
17161730
fieldTypes
17171731
);
17181732
}
@@ -1727,6 +1741,7 @@ public AggTestConfig withSplitLeavesIntoSeperateAggregators(boolean splitLeavesI
17271741
incrementalReduce,
17281742
useLogDocMergePolicy,
17291743
testReductionCancellation,
1744+
checkAggregator,
17301745
fieldTypes
17311746
);
17321747
}
@@ -1741,6 +1756,7 @@ public AggTestConfig withShouldBeCached(boolean shouldBeCached) {
17411756
incrementalReduce,
17421757
useLogDocMergePolicy,
17431758
testReductionCancellation,
1759+
checkAggregator,
17441760
fieldTypes
17451761
);
17461762
}
@@ -1755,6 +1771,7 @@ public AggTestConfig withMaxBuckets(int maxBuckets) {
17551771
incrementalReduce,
17561772
useLogDocMergePolicy,
17571773
testReductionCancellation,
1774+
checkAggregator,
17581775
fieldTypes
17591776
);
17601777
}
@@ -1769,6 +1786,7 @@ public AggTestConfig withIncrementalReduce(boolean incrementalReduce) {
17691786
incrementalReduce,
17701787
useLogDocMergePolicy,
17711788
testReductionCancellation,
1789+
checkAggregator,
17721790
fieldTypes
17731791
);
17741792
}
@@ -1783,6 +1801,7 @@ public AggTestConfig withLogDocMergePolicy() {
17831801
incrementalReduce,
17841802
true,
17851803
testReductionCancellation,
1804+
checkAggregator,
17861805
fieldTypes
17871806
);
17881807
}
@@ -1797,6 +1816,22 @@ public AggTestConfig noReductionCancellation() {
17971816
incrementalReduce,
17981817
useLogDocMergePolicy,
17991818
false,
1819+
checkAggregator,
1820+
fieldTypes
1821+
);
1822+
}
1823+
1824+
public AggTestConfig withCheckAggregator(Consumer<Aggregator> checkAggregator) {
1825+
return new AggTestConfig(
1826+
query,
1827+
builder,
1828+
maxBuckets,
1829+
splitLeavesIntoSeparateAggregators,
1830+
shouldBeCached,
1831+
incrementalReduce,
1832+
useLogDocMergePolicy,
1833+
testReductionCancellation,
1834+
checkAggregator,
18001835
fieldTypes
18011836
);
18021837
}

0 commit comments

Comments
 (0)