Skip to content

Commit 5de0ed9

Browse files
jimczijimczi
authored
Replace AggregatorTestCase#search with AggregatorTestCase#searchAndReduce (#60683)
* Replace AggregatorTestCase#search with AggregatorTestCase#searchAndReduce This commit removes the ability to test the top level result of an aggregator before it runs the final reduce. All aggregator tests that use AggregatorTestCase#search are rewritten with AggregatorTestCase#searchAndReduce in order to ensure that we test the final output (the one sent to the end user) rather than an intermediary result that could be different. This change also removes spurious commits triggered on top of a random index writer. These commits slow down the tests and are redundant with the commits that the random index writer performs.
1 parent 807b0fe commit 5de0ed9

File tree

39 files changed

+480
-868
lines changed

39 files changed

+480
-868
lines changed

‎modules/aggs-matrix-stats/src/test/java/org/elasticsearch/search/aggregations/matrix/stats/MatrixStatsAggregatorTests.java‎

Lines changed: 4 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,9 @@ public void testNoData() throws Exception {
5353
IndexSearcher searcher = new IndexSearcher(reader);
5454
MatrixStatsAggregationBuilder aggBuilder = new MatrixStatsAggregationBuilder("my_agg")
5555
.fields(Collections.singletonList("field"));
56-
InternalMatrixStats stats = search(searcher, new MatchAllDocsQuery(), aggBuilder, ft);
56+
InternalMatrixStats stats = searchAndReduce(searcher, new MatchAllDocsQuery(), aggBuilder, ft);
5757
assertNull(stats.getStats());
58-
assertFalse(MatrixAggregationInspectionHelper.hasValue(stats));
58+
assertEquals(0L, stats.getDocCount());
5959
}
6060
}
6161
}
@@ -72,9 +72,9 @@ public void testUnmapped() throws Exception {
7272
IndexSearcher searcher = new IndexSearcher(reader);
7373
MatrixStatsAggregationBuilder aggBuilder = new MatrixStatsAggregationBuilder("my_agg")
7474
.fields(Collections.singletonList("bogus"));
75-
InternalMatrixStats stats = search(searcher, new MatchAllDocsQuery(), aggBuilder, ft);
75+
InternalMatrixStats stats = searchAndReduce(searcher, new MatchAllDocsQuery(), aggBuilder, ft);
7676
assertNull(stats.getStats());
77-
assertFalse(MatrixAggregationInspectionHelper.hasValue(stats));
77+
assertEquals(0L, stats.getDocCount());
7878
}
7979
}
8080
}
@@ -85,43 +85,6 @@ public void testTwoFields() throws Exception {
8585
String fieldB = "b";
8686
MappedFieldType ftB = new NumberFieldMapper.NumberFieldType(fieldB, NumberFieldMapper.NumberType.DOUBLE);
8787

88-
try (Directory directory = newDirectory();
89-
RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
90-
91-
int numDocs = scaledRandomIntBetween(8192, 16384);
92-
Double[] fieldAValues = new Double[numDocs];
93-
Double[] fieldBValues = new Double[numDocs];
94-
for (int docId = 0; docId < numDocs; docId++) {
95-
Document document = new Document();
96-
fieldAValues[docId] = randomDouble();
97-
document.add(new SortedNumericDocValuesField(fieldA, NumericUtils.doubleToSortableLong(fieldAValues[docId])));
98-
99-
fieldBValues[docId] = randomDouble();
100-
document.add(new SortedNumericDocValuesField(fieldB, NumericUtils.doubleToSortableLong(fieldBValues[docId])));
101-
indexWriter.addDocument(document);
102-
}
103-
104-
MultiPassStats multiPassStats = new MultiPassStats(fieldA, fieldB);
105-
multiPassStats.computeStats(Arrays.asList(fieldAValues), Arrays.asList(fieldBValues));
106-
try (IndexReader reader = indexWriter.getReader()) {
107-
IndexSearcher searcher = new IndexSearcher(reader);
108-
MatrixStatsAggregationBuilder aggBuilder = new MatrixStatsAggregationBuilder("my_agg")
109-
.fields(Arrays.asList(fieldA, fieldB));
110-
InternalMatrixStats stats = search(searcher, new MatchAllDocsQuery(), aggBuilder, ftA, ftB);
111-
// Since `search` doesn't do any reduction, and the InternalMatrixStats object will have a null `MatrixStatsResults`
112-
// object. That is created during the final reduction, which also does a final round of computations
113-
// So we have to create a MatrixStatsResults object here manually so that the final `compute()` is called
114-
multiPassStats.assertNearlyEqual(new MatrixStatsResults(stats.getStats()));
115-
}
116-
}
117-
}
118-
119-
public void testTwoFieldsReduce() throws Exception {
120-
String fieldA = "a";
121-
MappedFieldType ftA = new NumberFieldMapper.NumberFieldType(fieldA, NumberFieldMapper.NumberType.DOUBLE);
122-
String fieldB = "b";
123-
MappedFieldType ftB = new NumberFieldMapper.NumberFieldType(fieldB, NumberFieldMapper.NumberType.DOUBLE);
124-
12588
try (Directory directory = newDirectory();
12689
RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
12790

@@ -145,8 +108,6 @@ public void testTwoFieldsReduce() throws Exception {
145108
MatrixStatsAggregationBuilder aggBuilder = new MatrixStatsAggregationBuilder("my_agg")
146109
.fields(Arrays.asList(fieldA, fieldB));
147110
InternalMatrixStats stats = searchAndReduce(searcher, new MatchAllDocsQuery(), aggBuilder, ftA, ftB);
148-
// Unlike testTwoFields, `searchAndReduce` will execute reductions so the `MatrixStatsResults` object
149-
// will be populated and fully computed. We should use that value directly to test against
150111
multiPassStats.assertNearlyEqual(stats);
151112
assertTrue(MatrixAggregationInspectionHelper.hasValue(stats));
152113
}

‎modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,7 @@ private void testCase(Query query, IndexSearcher indexSearcher, Consumer<Interna
303303
aggregationBuilder.subAggregation(new MinAggregationBuilder("in_parent").field("number"));
304304

305305
MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);
306-
InternalParent result = search(indexSearcher, query, aggregationBuilder, fieldType);
306+
InternalParent result = searchAndReduce(indexSearcher, query, aggregationBuilder, fieldType);
307307
verify.accept(result);
308308
}
309309

@@ -314,7 +314,7 @@ private void testCaseTerms(Query query, IndexSearcher indexSearcher, Consumer<In
314314
aggregationBuilder.subAggregation(new TermsAggregationBuilder("value_terms").field("number"));
315315

316316
MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);
317-
InternalParent result = search(indexSearcher, query, aggregationBuilder, fieldType);
317+
InternalParent result = searchAndReduce(indexSearcher, query, aggregationBuilder, fieldType);
318318
verify.accept(result);
319319
}
320320

@@ -328,7 +328,7 @@ private void testCaseTermsParentTerms(Query query, IndexSearcher indexSearcher,
328328

329329
MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);
330330
MappedFieldType subFieldType = new NumberFieldMapper.NumberFieldType("subNumber", NumberFieldMapper.NumberType.LONG);
331-
LongTerms result = search(indexSearcher, query, aggregationBuilder, fieldType, subFieldType);
331+
LongTerms result = searchAndReduce(indexSearcher, query, aggregationBuilder, fieldType, subFieldType);
332332
verify.accept(result);
333333
}
334334

‎modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,8 @@ public void testParentChildAsSubAgg() throws IOException {
164164
expectedOddMin = Math.min(expectedOddMin, e.getValue().v2());
165165
}
166166
}
167-
StringTerms result = search(indexSearcher, new MatchAllDocsQuery(), request, longField("number"), keywordField("kwd"));
167+
StringTerms result =
168+
searchAndReduce(indexSearcher, new MatchAllDocsQuery(), request, longField("number"), keywordField("kwd"));
168169

169170
StringTerms.Bucket evenBucket = result.getBucketByKey("even");
170171
InternalChildren evenChildren = evenBucket.getAggregations().get("children");
@@ -254,7 +255,7 @@ private void testCase(Query query, IndexSearcher indexSearcher, Consumer<Interna
254255
aggregationBuilder.subAggregation(new MinAggregationBuilder("in_child").field("number"));
255256

256257
MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);
257-
InternalChildren result = search(indexSearcher, query, aggregationBuilder, fieldType);
258+
InternalChildren result = searchAndReduce(indexSearcher, query, aggregationBuilder, fieldType);
258259
verify.accept(result);
259260
}
260261

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalVariableWidthHistogram.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,6 @@ protected boolean lessThan(IteratorAndCurrent a, IteratorAndCurrent b) {
400400
}
401401

402402
mergeBucketsIfNeeded(reducedBuckets, targetNumBuckets, reduceContext);
403-
404403
return reducedBuckets;
405404
}
406405

@@ -451,7 +450,8 @@ private void mergeBucketsWithPlan(List<Bucket> buckets, List<BucketRange> plan,
451450
}
452451
toMerge.add(buckets.get(startIdx)); // Don't remove the startIdx bucket because it will be replaced by the merged bucket
453452

454-
reduceContext.consumeBucketsAndMaybeBreak(- (toMerge.size() - 1));
453+
int toRemove = toMerge.stream().mapToInt(b -> countInnerBucket(b)+1).sum();
454+
reduceContext.consumeBucketsAndMaybeBreak(-toRemove + 1);
455455
Bucket merged_bucket = reduceBucket(toMerge, reduceContext);
456456

457457
buckets.set(startIdx, merged_bucket);

‎server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1879,7 +1879,7 @@ public void testEarlyTermination() throws Exception {
18791879
)
18801880
);
18811881

1882-
executeTestCase(true, false, new TermQuery(new Term("foo", "bar")),
1882+
executeTestCase(true, new TermQuery(new Term("foo", "bar")),
18831883
dataset,
18841884
() ->
18851885
new CompositeAggregationBuilder("name",
@@ -1899,7 +1899,7 @@ public void testEarlyTermination() throws Exception {
18991899
);
19001900

19011901
// source field and index sorting config have different order
1902-
executeTestCase(true, false, new TermQuery(new Term("foo", "bar")),
1902+
executeTestCase(true, new TermQuery(new Term("foo", "bar")),
19031903
dataset,
19041904
() ->
19051905
new CompositeAggregationBuilder("name",
@@ -1936,7 +1936,7 @@ public void testIndexSortWithDuplicate() throws Exception {
19361936
);
19371937

19381938
for (SortOrder order : SortOrder.values()) {
1939-
executeTestCase(true, false, new MatchAllDocsQuery(),
1939+
executeTestCase(true, new MatchAllDocsQuery(),
19401940
dataset,
19411941
() ->
19421942
new CompositeAggregationBuilder("name",
@@ -1959,7 +1959,7 @@ public void testIndexSortWithDuplicate() throws Exception {
19591959
}
19601960
);
19611961

1962-
executeTestCase(true, false, new MatchAllDocsQuery(),
1962+
executeTestCase(true, new MatchAllDocsQuery(),
19631963
dataset,
19641964
() ->
19651965
new CompositeAggregationBuilder("name",
@@ -1989,14 +1989,12 @@ private void testSearchCase(List<Query> queries,
19891989
Supplier<CompositeAggregationBuilder> create,
19901990
Consumer<InternalComposite> verify) throws IOException {
19911991
for (Query query : queries) {
1992-
executeTestCase(false, false, query, dataset, create, verify);
1993-
executeTestCase(false, true, query, dataset, create, verify);
1994-
executeTestCase(true, true, query, dataset, create, verify);
1992+
executeTestCase(false, query, dataset, create, verify);
1993+
executeTestCase(true, query, dataset, create, verify);
19951994
}
19961995
}
19971996

19981997
private void executeTestCase(boolean useIndexSort,
1999-
boolean reduced,
20001998
Query query,
20011999
List<Map<String, List<Object>>> dataset,
20022000
Supplier<CompositeAggregationBuilder> create,
@@ -2019,18 +2017,13 @@ private void executeTestCase(boolean useIndexSort,
20192017
indexWriter.addDocument(document);
20202018
document.clear();
20212019
}
2022-
if (reduced == false && randomBoolean()) {
2020+
if (rarely()) {
20232021
indexWriter.forceMerge(1);
20242022
}
20252023
}
20262024
try (IndexReader indexReader = DirectoryReader.open(directory)) {
20272025
IndexSearcher indexSearcher = new IndexSearcher(indexReader);
2028-
final InternalComposite composite;
2029-
if (reduced) {
2030-
composite = searchAndReduce(indexSettings, indexSearcher, query, aggregationBuilder, FIELD_TYPES);
2031-
} else {
2032-
composite = search(indexSettings, indexSearcher, query, aggregationBuilder, FIELD_TYPES);
2033-
}
2026+
InternalComposite composite = searchAndReduce(indexSettings, indexSearcher, query, aggregationBuilder, FIELD_TYPES);
20342027
verify.accept(composite);
20352028
}
20362029
}

‎server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public void testEmpty() throws Exception {
6262
IndexSearcher indexSearcher = newSearcher(indexReader, true, true);
6363
QueryBuilder filter = QueryBuilders.termQuery("field", randomAlphaOfLength(5));
6464
FilterAggregationBuilder builder = new FilterAggregationBuilder("test", filter);
65-
InternalFilter response = search(indexSearcher, new MatchAllDocsQuery(), builder,
65+
InternalFilter response = searchAndReduce(indexSearcher, new MatchAllDocsQuery(), builder,
6666
fieldType);
6767
assertEquals(response.getDocCount(), 0);
6868
assertFalse(AggregationInspectionHelper.hasValue(response));
@@ -80,7 +80,7 @@ public void testRandom() throws Exception {
8080
for (int i = 0; i < numDocs; i++) {
8181
if (frequently()) {
8282
// make sure we have more than one segment to test the merge
83-
indexWriter.getReader().close();
83+
indexWriter.commit();
8484
}
8585
int value = randomInt(maxTerm-1);
8686
expectedBucketCount[value] += 1;
@@ -98,20 +98,12 @@ public void testRandom() throws Exception {
9898
QueryBuilder filter = QueryBuilders.termQuery("field", Integer.toString(value));
9999
FilterAggregationBuilder builder = new FilterAggregationBuilder("test", filter);
100100

101-
for (boolean doReduce : new boolean[]{true, false}) {
102-
final InternalFilter response;
103-
if (doReduce) {
104-
response = searchAndReduce(indexSearcher, new MatchAllDocsQuery(), builder,
105-
fieldType);
106-
} else {
107-
response = search(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
108-
}
109-
assertEquals(response.getDocCount(), (long) expectedBucketCount[value]);
110-
if (expectedBucketCount[value] > 0) {
111-
assertTrue(AggregationInspectionHelper.hasValue(response));
112-
} else {
113-
assertFalse(AggregationInspectionHelper.hasValue(response));
114-
}
101+
final InternalFilter response = searchAndReduce(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
102+
assertEquals(response.getDocCount(), (long) expectedBucketCount[value]);
103+
if (expectedBucketCount[value] > 0) {
104+
assertTrue(AggregationInspectionHelper.hasValue(response));
105+
} else {
106+
assertFalse(AggregationInspectionHelper.hasValue(response));
115107
}
116108
} finally {
117109
indexReader.close();

‎server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java‎

Lines changed: 22 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public void testEmpty() throws Exception {
6060
}
6161
FiltersAggregationBuilder builder = new FiltersAggregationBuilder("test", filters);
6262
builder.otherBucketKey("other");
63-
InternalFilters response = search(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
63+
InternalFilters response = searchAndReduce(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
6464
assertEquals(response.getBuckets().size(), numFilters);
6565
for (InternalFilters.InternalBucket filter : response.getBuckets()) {
6666
assertEquals(filter.getDocCount(), 0);
@@ -113,22 +113,15 @@ public void testKeyedFilter() throws Exception {
113113
FiltersAggregationBuilder builder = new FiltersAggregationBuilder("test", keys);
114114
builder.otherBucket(true);
115115
builder.otherBucketKey("other");
116-
for (boolean doReduce : new boolean[] {true, false}) {
117-
final InternalFilters filters;
118-
if (doReduce) {
119-
filters = searchAndReduce(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
120-
} else {
121-
filters = search(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
122-
}
123-
assertEquals(filters.getBuckets().size(), 7);
124-
assertEquals(filters.getBucketByKey("foobar").getDocCount(), 2);
125-
assertEquals(filters.getBucketByKey("foo").getDocCount(), 2);
126-
assertEquals(filters.getBucketByKey("foo2").getDocCount(), 2);
127-
assertEquals(filters.getBucketByKey("bar").getDocCount(), 1);
128-
assertEquals(filters.getBucketByKey("same").getDocCount(), 1);
129-
assertEquals(filters.getBucketByKey("other").getDocCount(), 2);
130-
assertTrue(AggregationInspectionHelper.hasValue(filters));
131-
}
116+
final InternalFilters filters = searchAndReduce(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
117+
assertEquals(filters.getBuckets().size(), 7);
118+
assertEquals(filters.getBucketByKey("foobar").getDocCount(), 2);
119+
assertEquals(filters.getBucketByKey("foo").getDocCount(), 2);
120+
assertEquals(filters.getBucketByKey("foo2").getDocCount(), 2);
121+
assertEquals(filters.getBucketByKey("bar").getDocCount(), 1);
122+
assertEquals(filters.getBucketByKey("same").getDocCount(), 1);
123+
assertEquals(filters.getBucketByKey("other").getDocCount(), 2);
124+
assertTrue(AggregationInspectionHelper.hasValue(filters));
132125

133126
indexReader.close();
134127
directory.close();
@@ -175,28 +168,21 @@ public void testRandom() throws Exception {
175168
builder.otherBucket(true);
176169
builder.otherBucketKey("other");
177170

178-
for (boolean doReduce : new boolean[]{true, false}) {
179-
final InternalFilters response;
180-
if (doReduce) {
181-
response = searchAndReduce(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
182-
} else {
183-
response = search(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
184-
}
185-
List<InternalFilters.InternalBucket> buckets = response.getBuckets();
186-
assertEquals(buckets.size(), filters.length + 1);
171+
final InternalFilters response = searchAndReduce(indexSearcher, new MatchAllDocsQuery(), builder, fieldType);
172+
List<InternalFilters.InternalBucket> buckets = response.getBuckets();
173+
assertEquals(buckets.size(), filters.length + 1);
187174

188-
for (InternalFilters.InternalBucket bucket : buckets) {
189-
if ("other".equals(bucket.getKey())) {
190-
assertEquals(bucket.getDocCount(), expectedOtherCount);
191-
} else {
192-
int index = Integer.parseInt(bucket.getKey());
193-
assertEquals(bucket.getDocCount(), (long) expectedBucketCount[filterTerms[index]]);
194-
}
175+
for (InternalFilters.InternalBucket bucket : buckets) {
176+
if ("other".equals(bucket.getKey())) {
177+
assertEquals(bucket.getDocCount(), expectedOtherCount);
178+
} else {
179+
int index = Integer.parseInt(bucket.getKey());
180+
assertEquals(bucket.getDocCount(), (long) expectedBucketCount[filterTerms[index]]);
195181
}
196-
197-
// Always true because we include 'other' in the agg
198-
assertTrue(AggregationInspectionHelper.hasValue(response));
199182
}
183+
184+
// Always true because we include 'other' in the agg
185+
assertTrue(AggregationInspectionHelper.hasValue(response));
200186
} finally {
201187
indexReader.close();
202188
directory.close();

0 commit comments

Comments
 (0)