Skip to content

Commit 91283ed

Browse files
nik9000nik9000
committed
Aggs: Let terms run in global ords mode no match
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 519381f commit 91283ed

File tree

3 files changed

+126
-5
lines changed

3 files changed

+126
-5
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,10 @@ private static TermsAggregatorSupplier bytesSupplier() {
111111
execution = ExecutionMode.fromString(executionHint);
112112
}
113113
// In some cases, using ordinals is just not supported: override it
114-
if (valuesSource.hasOrdinals() == false || matchNoDocs(context, parent)) {
114+
if (valuesSource.hasOrdinals() == false) {
115+
execution = ExecutionMode.MAP;
116+
}
117+
if (matchNoDocs(context, parent) && bucketCountThresholds.getMinDocCount() > 0) {
115118
execution = ExecutionMode.MAP;
116119
}
117120
if (execution == null) {

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

Lines changed: 86 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,7 @@ public void testMatchNoDocsQuery() throws Exception {
298298
createIndex,
299299
(InternalTerms<?, ?> result) -> { assertEquals(0, result.getBuckets().size()); },
300300
new AggTestConfig(aggregationBuilder, fieldType).withQuery(new MatchNoDocsQuery())
301+
.withCheckAggregator(checkTopLevelAggregatorNoRewrites(TermsAggregatorFactory.ExecutionMode.MAP))
301302
);
302303

303304
debugTestCase(aggregationBuilder, new MatchNoDocsQuery(), createIndex, (result, impl, debug) -> {
@@ -337,7 +338,6 @@ public void testStringShardZeroMinDocCount() throws IOException {
337338
.executionHint(executionMode.toString())
338339
.size(2)
339340
.minDocCount(0)
340-
.executionHint("map")
341341
.excludeDeletedDocs(true)
342342
.order(BucketOrder.key(true));
343343

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

366439
{
@@ -384,7 +457,10 @@ public void testStringShardZeroMinDocCount() throws IOException {
384457
assertEquals("b", result.getBuckets().get(1).getKeyAsString());
385458
}
386459
assertEquals(0L, result.getBuckets().get(1).getDocCount());
387-
}, new AggTestConfig(aggregationBuilder, fieldType).withQuery(new TermQuery(new Term("string", "e"))));
460+
},
461+
new AggTestConfig(aggregationBuilder, fieldType).withQuery(new MatchNoDocsQuery())
462+
.withCheckAggregator(checkTopLevelAggregatorNoRewrites(executionMode))
463+
);
388464
}
389465
}
390466
}
@@ -2279,4 +2355,11 @@ protected List<ObjectMapper> objectMappers() {
22792355
private String randomHint() {
22802356
return randomFrom(TermsAggregatorFactory.ExecutionMode.values()).toString();
22812357
}
2358+
2359+
private Consumer<Aggregator> checkTopLevelAggregatorNoRewrites(TermsAggregatorFactory.ExecutionMode executionMode) {
2360+
return switch (executionMode) {
2361+
case MAP -> a -> assertThat(a, instanceOf(MapStringTermsAggregator.class));
2362+
case GLOBAL_ORDINALS -> a -> assertThat(a, instanceOf(GlobalOrdinalsStringTermsAggregator.class));
2363+
};
2364+
}
22822365
}

‎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
@@ -685,6 +685,7 @@ private <A extends InternalAggregation, C extends Aggregator> A searchAndReduce(
685685
}
686686
assertEquals(shouldBeCached, context.isCacheable());
687687
List<InternalAggregation> internalAggregations = List.of(a.buildTopLevel());
688+
aggTestConfig.checkAggregator().accept(a);
688689
assertRoundTrip(internalAggregations);
689690
internalAggs.add(InternalAggregations.from(internalAggregations));
690691
} finally {
@@ -1703,11 +1704,23 @@ public record AggTestConfig(
17031704

17041705
boolean useLogDocMergePolicy,
17051706
boolean testReductionCancellation,
1707+
Consumer<Aggregator> checkAggregator,
17061708
MappedFieldType... fieldTypes
17071709
) {
17081710

17091711
public AggTestConfig(AggregationBuilder builder, MappedFieldType... fieldTypes) {
1710-
this(new MatchAllDocsQuery(), builder, DEFAULT_MAX_BUCKETS, randomBoolean(), true, randomBoolean(), false, true, fieldTypes);
1712+
this(
1713+
new MatchAllDocsQuery(),
1714+
builder,
1715+
DEFAULT_MAX_BUCKETS,
1716+
randomBoolean(),
1717+
true,
1718+
randomBoolean(),
1719+
false,
1720+
true,
1721+
a -> {},
1722+
fieldTypes
1723+
);
17111724
}
17121725

17131726
public AggTestConfig withQuery(Query query) {
@@ -1720,6 +1733,7 @@ public AggTestConfig withQuery(Query query) {
17201733
incrementalReduce,
17211734
useLogDocMergePolicy,
17221735
testReductionCancellation,
1736+
checkAggregator,
17231737
fieldTypes
17241738
);
17251739
}
@@ -1734,6 +1748,7 @@ public AggTestConfig withSplitLeavesIntoSeperateAggregators(boolean splitLeavesI
17341748
incrementalReduce,
17351749
useLogDocMergePolicy,
17361750
testReductionCancellation,
1751+
checkAggregator,
17371752
fieldTypes
17381753
);
17391754
}
@@ -1748,6 +1763,7 @@ public AggTestConfig withShouldBeCached(boolean shouldBeCached) {
17481763
incrementalReduce,
17491764
useLogDocMergePolicy,
17501765
testReductionCancellation,
1766+
checkAggregator,
17511767
fieldTypes
17521768
);
17531769
}
@@ -1762,6 +1778,7 @@ public AggTestConfig withMaxBuckets(int maxBuckets) {
17621778
incrementalReduce,
17631779
useLogDocMergePolicy,
17641780
testReductionCancellation,
1781+
checkAggregator,
17651782
fieldTypes
17661783
);
17671784
}
@@ -1776,6 +1793,7 @@ public AggTestConfig withIncrementalReduce(boolean incrementalReduce) {
17761793
incrementalReduce,
17771794
useLogDocMergePolicy,
17781795
testReductionCancellation,
1796+
checkAggregator,
17791797
fieldTypes
17801798
);
17811799
}
@@ -1790,6 +1808,7 @@ public AggTestConfig withLogDocMergePolicy() {
17901808
incrementalReduce,
17911809
true,
17921810
testReductionCancellation,
1811+
checkAggregator,
17931812
fieldTypes
17941813
);
17951814
}
@@ -1804,6 +1823,22 @@ public AggTestConfig noReductionCancellation() {
18041823
incrementalReduce,
18051824
useLogDocMergePolicy,
18061825
false,
1826+
checkAggregator,
1827+
fieldTypes
1828+
);
1829+
}
1830+
1831+
public AggTestConfig withCheckAggregator(Consumer<Aggregator> checkAggregator) {
1832+
return new AggTestConfig(
1833+
query,
1834+
builder,
1835+
maxBuckets,
1836+
splitLeavesIntoSeparateAggregators,
1837+
shouldBeCached,
1838+
incrementalReduce,
1839+
useLogDocMergePolicy,
1840+
testReductionCancellation,
1841+
checkAggregator,
18071842
fieldTypes
18081843
);
18091844
}

0 commit comments

Comments
 (0)