Skip to content

Commit aa40e1d

Browse files
nik9000nik9000
committed
Aggs: Let terms run in global ords mode no match (elastic#124782)
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 ab8b1f0 commit aa40e1d

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
@@ -675,6 +675,7 @@ private <A extends InternalAggregation, C extends Aggregator> A searchAndReduce(
675675
}
676676
assertEquals(shouldBeCached, context.isCacheable());
677677
List<InternalAggregation> internalAggregations = List.of(a.buildTopLevel());
678+
aggTestConfig.checkAggregator().accept(a);
678679
assertRoundTrip(internalAggregations);
679680
internalAggs.add(InternalAggregations.from(internalAggregations));
680681
} finally {
@@ -1687,11 +1688,23 @@ public record AggTestConfig(
16871688

16881689
boolean useLogDocMergePolicy,
16891690
boolean testReductionCancellation,
1691+
Consumer<Aggregator> checkAggregator,
16901692
MappedFieldType... fieldTypes
16911693
) {
16921694

16931695
public AggTestConfig(AggregationBuilder builder, MappedFieldType... fieldTypes) {
1694-
this(new MatchAllDocsQuery(), builder, DEFAULT_MAX_BUCKETS, randomBoolean(), true, randomBoolean(), false, true, fieldTypes);
1696+
this(
1697+
new MatchAllDocsQuery(),
1698+
builder,
1699+
DEFAULT_MAX_BUCKETS,
1700+
randomBoolean(),
1701+
true,
1702+
randomBoolean(),
1703+
false,
1704+
true,
1705+
a -> {},
1706+
fieldTypes
1707+
);
16951708
}
16961709

16971710
public AggTestConfig withQuery(Query query) {
@@ -1704,6 +1717,7 @@ public AggTestConfig withQuery(Query query) {
17041717
incrementalReduce,
17051718
useLogDocMergePolicy,
17061719
testReductionCancellation,
1720+
checkAggregator,
17071721
fieldTypes
17081722
);
17091723
}
@@ -1718,6 +1732,7 @@ public AggTestConfig withSplitLeavesIntoSeperateAggregators(boolean splitLeavesI
17181732
incrementalReduce,
17191733
useLogDocMergePolicy,
17201734
testReductionCancellation,
1735+
checkAggregator,
17211736
fieldTypes
17221737
);
17231738
}
@@ -1732,6 +1747,7 @@ public AggTestConfig withShouldBeCached(boolean shouldBeCached) {
17321747
incrementalReduce,
17331748
useLogDocMergePolicy,
17341749
testReductionCancellation,
1750+
checkAggregator,
17351751
fieldTypes
17361752
);
17371753
}
@@ -1746,6 +1762,7 @@ public AggTestConfig withMaxBuckets(int maxBuckets) {
17461762
incrementalReduce,
17471763
useLogDocMergePolicy,
17481764
testReductionCancellation,
1765+
checkAggregator,
17491766
fieldTypes
17501767
);
17511768
}
@@ -1760,6 +1777,7 @@ public AggTestConfig withIncrementalReduce(boolean incrementalReduce) {
17601777
incrementalReduce,
17611778
useLogDocMergePolicy,
17621779
testReductionCancellation,
1780+
checkAggregator,
17631781
fieldTypes
17641782
);
17651783
}
@@ -1774,6 +1792,7 @@ public AggTestConfig withLogDocMergePolicy() {
17741792
incrementalReduce,
17751793
true,
17761794
testReductionCancellation,
1795+
checkAggregator,
17771796
fieldTypes
17781797
);
17791798
}
@@ -1788,6 +1807,22 @@ public AggTestConfig noReductionCancellation() {
17881807
incrementalReduce,
17891808
useLogDocMergePolicy,
17901809
false,
1810+
checkAggregator,
1811+
fieldTypes
1812+
);
1813+
}
1814+
1815+
public AggTestConfig withCheckAggregator(Consumer<Aggregator> checkAggregator) {
1816+
return new AggTestConfig(
1817+
query,
1818+
builder,
1819+
maxBuckets,
1820+
splitLeavesIntoSeparateAggregators,
1821+
shouldBeCached,
1822+
incrementalReduce,
1823+
useLogDocMergePolicy,
1824+
testReductionCancellation,
1825+
checkAggregator,
17911826
fieldTypes
17921827
);
17931828
}

0 commit comments

Comments
 (0)