Skip to content

Commit b5e08d8

Browse files
ajleong623ajleong623
committed
added edits to missing terms agg tests
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
1 parent ab13378 commit b5e08d8

File tree

5 files changed

+160
-144
lines changed

5 files changed

+160
-144
lines changed

‎server/src/main/java/org/opensearch/search/aggregations/support/ValuesSource.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,11 @@ public boolean needsScores() {
373373
return script.needs_score();
374374
}
375375

376+
@Override
377+
public String getIndexFieldName() {
378+
return delegate.getIndexFieldName();
379+
}
380+
376381
@Override
377382
public SortedBinaryDocValues bytesValues(LeafReaderContext context) throws IOException {
378383
return new BytesValues(delegate.bytesValues(context), script.newInstance(context));
@@ -517,6 +522,11 @@ public boolean needsScores() {
517522
return script.needs_score();
518523
}
519524

525+
@Override
526+
public String getIndexFieldName() {
527+
return delegate.getIndexFieldName();
528+
}
529+
520530
@Override
521531
public SortedBinaryDocValues bytesValues(LeafReaderContext context) throws IOException {
522532
return new Bytes.WithScript.BytesValues(delegate.bytesValues(context), script.newInstance(context));

‎server/src/test/java/org/opensearch/search/aggregations/bucket/missing/MissingAggregatorTests.java‎

Lines changed: 120 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ public void testMatchNoDocs() throws IOException {
9898
final MissingAggregationBuilder builder = new MissingAggregationBuilder("_name").field(fieldType.name());
9999
final boolean isIndexed = randomBoolean();
100100

101-
testCase(newMatchAllQuery(), builder, writer -> {
101+
CheckedConsumer<RandomIndexWriter, IOException> writeIndex = (writer -> {
102102
for (int i = 0; i < numDocs; i++) {
103103
if (isIndexed) {
104104
final long randomLong = randomLong();
@@ -112,16 +112,20 @@ public void testMatchNoDocs() throws IOException {
112112
writer.addDocument(singleton(new SortedNumericDocValuesField(fieldType.name(), randomLong())));
113113
}
114114
}
115-
}, internalMissing -> {
116-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
117-
assertEquals(0, internalMissingAgg.getDocCount());
118-
assertFalse(AggregationInspectionHelper.hasValue(internalMissingAgg));
119-
if (isIndexed) {
120-
assertEquals(0, internalMissing.getCount());
121-
} else {
122-
assertEquals(numDocs, internalMissing.getCount());
123-
}
124-
}, singleton(fieldType));
115+
});
116+
117+
if (isIndexed) {
118+
// The precompute optimization kicked in, so no docs were traversed.
119+
testCase(newMatchAllQuery(), builder, writeIndex, internalMissing -> {
120+
assertEquals(0, internalMissing.getDocCount());
121+
assertFalse(AggregationInspectionHelper.hasValue(internalMissing));
122+
}, singleton(fieldType), 0);
123+
} else {
124+
testCase(newMatchAllQuery(), builder, writeIndex, internalMissing -> {
125+
assertEquals(0, internalMissing.getDocCount());
126+
assertFalse(AggregationInspectionHelper.hasValue(internalMissing));
127+
}, singleton(fieldType), numDocs);
128+
}
125129
}
126130

127131
public void testMatchAllDocs() throws IOException {
@@ -134,7 +138,7 @@ public void testMatchAllDocs() throws IOException {
134138

135139
final boolean isIndexed = false;
136140

137-
testCase(newMatchAllQuery(), builder, writer -> {
141+
CheckedConsumer<RandomIndexWriter, IOException> writeIndex = (writer -> {
138142
for (int i = 0; i < numDocs; i++) {
139143
if (isIndexed) {
140144
final long randomLong = randomLong();
@@ -148,18 +152,21 @@ public void testMatchAllDocs() throws IOException {
148152
writer.addDocument(singleton(new SortedNumericDocValuesField(anotherFieldType.name(), randomLong())));
149153
}
150154
}
151-
}, internalMissing -> {
152-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
153-
assertEquals(numDocs, internalMissingAgg.getDocCount());
154-
assertTrue(AggregationInspectionHelper.hasValue(internalMissingAgg));
155+
});
155156

156-
if (isIndexed) {
157-
assertEquals(0, internalMissing.getCount());
158-
} else {
159-
// We can use precomputation because we are counting a field that has never been added.
160-
assertEquals(0, internalMissing.getCount());
161-
}
162-
}, List.of(aggFieldType, anotherFieldType));
157+
if (isIndexed) {
158+
// The precompute optimization kicked in, so no docs were traversed.
159+
testCase(newMatchAllQuery(), builder, writeIndex, internalMissing -> {
160+
assertEquals(numDocs, internalMissing.getDocCount());
161+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
162+
}, List.of(aggFieldType, anotherFieldType), 0);
163+
} else {
164+
// We can use precomputation because we are counting a field that has never been added.
165+
testCase(newMatchAllQuery(), builder, writeIndex, internalMissing -> {
166+
assertEquals(numDocs, internalMissing.getDocCount());
167+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
168+
}, List.of(aggFieldType, anotherFieldType), 0);
169+
}
163170
}
164171

165172
public void testMatchSparse() throws IOException {
@@ -205,17 +212,18 @@ public void testMatchSparse() throws IOException {
205212
}
206213
final int finalDocsMissingAggField = docsMissingAggField;
207214

208-
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
209-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
210-
assertEquals(finalDocsMissingAggField, internalMissingAgg.getDocCount());
211-
assertTrue(AggregationInspectionHelper.hasValue(internalMissingAgg));
212-
213-
if (isIndexed) {
214-
assertEquals(0, internalMissing.getCount());
215-
} else {
216-
assertEquals(numDocs, internalMissing.getCount());
217-
}
218-
}, List.of(aggFieldType, anotherFieldType));
215+
if (isIndexed) {
216+
// The precompute optimization kicked in, so no docs were traversed.
217+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
218+
assertEquals(finalDocsMissingAggField, internalMissing.getDocCount());
219+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
220+
}, List.of(aggFieldType, anotherFieldType), 0);
221+
} else {
222+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
223+
assertEquals(finalDocsMissingAggField, internalMissing.getDocCount());
224+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
225+
}, List.of(aggFieldType, anotherFieldType), numDocs);
226+
}
219227
}
220228

221229
public void testMatchSparseRangeField() throws IOException {
@@ -261,17 +269,20 @@ public void testMatchSparseRangeField() throws IOException {
261269
}
262270
final int finalDocsMissingAggField = docsMissingAggField;
263271

264-
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
265-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
266-
assertEquals(finalDocsMissingAggField, internalMissingAgg.getDocCount());
267-
assertTrue(AggregationInspectionHelper.hasValue(internalMissingAgg));
268-
269-
if (isIndexed) {
270-
assertEquals(numDocs, internalMissing.getCount());
271-
} else {
272-
assertEquals(numDocs, internalMissing.getCount());
273-
}
274-
}, Arrays.asList(aggFieldType, anotherFieldType));
272+
if (isIndexed) {
273+
// The precompute does not work because only the other field was actually indexed. Therefore, the
274+
// precomputation could not declare whether the field was simply not indexed or if there were
275+
// actually no values in that field.
276+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
277+
assertEquals(finalDocsMissingAggField, internalMissing.getDocCount());
278+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
279+
}, Arrays.asList(aggFieldType, anotherFieldType), numDocs);
280+
} else {
281+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
282+
assertEquals(finalDocsMissingAggField, internalMissing.getDocCount());
283+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
284+
}, Arrays.asList(aggFieldType, anotherFieldType), numDocs);
285+
}
275286
}
276287

277288
public void testUnmappedWithoutMissingParam() throws IOException {
@@ -283,7 +294,7 @@ public void testUnmappedWithoutMissingParam() throws IOException {
283294
// Determines whether the fields we add to the documents are indexed.
284295
final boolean isIndexed = randomBoolean();
285296

286-
testCase(newMatchAllQuery(), builder, writer -> {
297+
CheckedConsumer<RandomIndexWriter, IOException> writeIndex = (writer -> {
287298
for (int i = 0; i < numDocs; i++) {
288299
if (isIndexed) {
289300
final long randomLong = randomLong();
@@ -297,17 +308,20 @@ public void testUnmappedWithoutMissingParam() throws IOException {
297308
writer.addDocument(singleton(new SortedNumericDocValuesField(aggFieldType.name(), randomLong())));
298309
}
299310
}
300-
}, internalMissing -> {
301-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
302-
assertEquals(numDocs, internalMissingAgg.getDocCount());
303-
assertTrue(AggregationInspectionHelper.hasValue(internalMissingAgg));
304-
if (isIndexed) {
305-
// Unfortunately, the values source is not provided, therefore, we cannot use the precomputation.
306-
assertEquals(numDocs, internalMissing.getCount());
307-
} else {
308-
assertEquals(numDocs, internalMissing.getCount());
309-
}
310-
}, singleton(aggFieldType));
311+
});
312+
313+
if (isIndexed) {
314+
// Unfortunately, the values source is not provided, therefore, we cannot use the precomputation.
315+
testCase(newMatchAllQuery(), builder, writeIndex, internalMissing -> {
316+
assertEquals(numDocs, internalMissing.getDocCount());
317+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
318+
}, singleton(aggFieldType), numDocs);
319+
} else {
320+
testCase(newMatchAllQuery(), builder, writeIndex, internalMissing -> {
321+
assertEquals(numDocs, internalMissing.getDocCount());
322+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
323+
}, singleton(aggFieldType), numDocs);
324+
}
311325
}
312326

313327
public void testUnmappedWithMissingParam() throws IOException {
@@ -319,6 +333,7 @@ public void testUnmappedWithMissingParam() throws IOException {
319333
// Determines whether the fields we add to the documents are indexed.
320334
final boolean isIndexed = randomBoolean();
321335

336+
// Having the missing parameter will make the missing aggregator not responsible for any documents, so it will short circuit
322337
testCase(newMatchAllQuery(), builder, writer -> {
323338
for (int i = 0; i < numDocs; i++) {
324339
if (isIndexed) {
@@ -334,12 +349,9 @@ public void testUnmappedWithMissingParam() throws IOException {
334349
}
335350
}
336351
}, internalMissing -> {
337-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
338-
assertEquals(0, internalMissingAgg.getDocCount());
339-
assertFalse(AggregationInspectionHelper.hasValue(internalMissingAgg));
340-
// Having the missing parameter will make the missing aggregator not responsible for any documents, so it will short circuit
341-
assertEquals(0, internalMissing.getCount());
342-
}, singleton(aggFieldType));
352+
assertEquals(0, internalMissing.getDocCount());
353+
assertFalse(AggregationInspectionHelper.hasValue(internalMissing));
354+
}, singleton(aggFieldType), 0);
343355
}
344356

345357
public void testMissingParam() throws IOException {
@@ -352,7 +364,8 @@ public void testMissingParam() throws IOException {
352364

353365
// Determines whether the fields we add to the documents are indexed.
354366
final boolean isIndexed = randomBoolean();
355-
367+
368+
// Having the missing parameter will make the missing aggregator not responsible for any documents, so it will short-circuit
356369
testCase(newMatchAllQuery(), builder, writer -> {
357370
for (int i = 0; i < numDocs; i++) {
358371
if (isIndexed) {
@@ -368,13 +381,9 @@ public void testMissingParam() throws IOException {
368381
}
369382
}
370383
}, internalMissing -> {
371-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
372-
assertEquals(0, internalMissingAgg.getDocCount());
373-
assertFalse(AggregationInspectionHelper.hasValue(internalMissingAgg));
374-
375-
// Having the missing parameter will make the missing aggregator not responsible for any documents, so it will short-circuit
376-
assertEquals(0, internalMissing.getCount());
377-
}, List.of(aggFieldType, anotherFieldType));
384+
assertEquals(0, internalMissing.getDocCount());
385+
assertFalse(AggregationInspectionHelper.hasValue(internalMissing));
386+
}, List.of(aggFieldType, anotherFieldType), 0);
378387
}
379388

380389
public void testMultiValuedField() throws IOException {
@@ -430,17 +439,18 @@ public void testMultiValuedField() throws IOException {
430439
}
431440
final int finalDocsMissingAggField = docsMissingAggField;
432441

433-
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
434-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
435-
assertEquals(finalDocsMissingAggField, internalMissingAgg.getDocCount());
436-
assertTrue(AggregationInspectionHelper.hasValue(internalMissingAgg));
437-
438-
if (isIndexed) {
439-
assertEquals(0, internalMissing.getCount());
440-
} else {
441-
assertEquals(numDocs, internalMissing.getCount());
442-
}
443-
}, List.of(aggFieldType, anotherFieldType));
442+
if (isIndexed) {
443+
// The precompute optimization kicked in, so no docs were traversed.
444+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
445+
assertEquals(finalDocsMissingAggField, internalMissing.getDocCount());
446+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
447+
}, List.of(aggFieldType, anotherFieldType), 0);
448+
} else {
449+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
450+
assertEquals(finalDocsMissingAggField, internalMissing.getDocCount());
451+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
452+
}, List.of(aggFieldType, anotherFieldType), numDocs);
453+
}
444454
}
445455

446456
public void testSingleValuedFieldWithValueScript() throws IOException {
@@ -494,16 +504,18 @@ private void valueScriptTestCase(Script script) throws IOException {
494504
}
495505
final int finalDocsMissingField = docsMissingAggField;
496506

497-
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
498-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
499-
assertEquals(finalDocsMissingField, internalMissingAgg.getDocCount());
500-
assertTrue(AggregationInspectionHelper.hasValue(internalMissingAgg));
501-
if (isIndexed) {
502-
assertEquals(numDocs, internalMissing.getCount());
503-
} else {
504-
assertEquals(numDocs, internalMissing.getCount());
505-
}
506-
}, List.of(aggFieldType, anotherFieldType));
507+
if (isIndexed) {
508+
// The precompute optimization kicked in, so no docs were traversed.
509+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
510+
assertEquals(finalDocsMissingField, internalMissing.getDocCount());
511+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
512+
}, List.of(aggFieldType, anotherFieldType), 0);
513+
} else {
514+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
515+
assertEquals(finalDocsMissingField, internalMissing.getDocCount());
516+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
517+
}, List.of(aggFieldType, anotherFieldType), numDocs);
518+
}
507519
}
508520

509521
public void testMultiValuedFieldWithFieldScriptWithParams() throws IOException {
@@ -561,25 +573,27 @@ private void fieldScriptTestCase(Script script, long threshold) throws IOExcepti
561573
}
562574
final int finalDocsBelowThreshold = docsBelowThreshold;
563575

564-
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
565-
InternalMissing internalMissingAgg = (InternalMissing) internalMissing.getAgg();
566-
assertEquals(finalDocsBelowThreshold, internalMissingAgg.getDocCount());
567-
assertTrue(AggregationInspectionHelper.hasValue(internalMissingAgg));
568-
if (isIndexed) {
569-
// The field name was not accessible from a script.
570-
assertEquals(numDocs, internalMissing.getCount());
571-
} else {
572-
assertEquals(numDocs, internalMissing.getCount());
573-
}
574-
}, singleton(aggFieldType));
576+
if (isIndexed) {
577+
// The precompute optimization did not kick in because the values source did not have an indexed name.
578+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
579+
assertEquals(finalDocsBelowThreshold, internalMissing.getDocCount());
580+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
581+
}, singleton(aggFieldType), numDocs);
582+
} else {
583+
testCase(newMatchAllQuery(), builder, writer -> writer.addDocuments(docs), internalMissing -> {
584+
assertEquals(finalDocsBelowThreshold, internalMissing.getDocCount());
585+
assertTrue(AggregationInspectionHelper.hasValue(internalMissing));
586+
}, singleton(aggFieldType), numDocs);
587+
}
575588
}
576589

577590
private void testCase(
578591
Query query,
579592
MissingAggregationBuilder builder,
580593
CheckedConsumer<RandomIndexWriter, IOException> writeIndex,
581-
Consumer<CompositeAggregationAndCount> verify,
582-
Collection<MappedFieldType> fieldTypes
594+
Consumer<InternalMissing> verify,
595+
Collection<MappedFieldType> fieldTypes,
596+
int expectedCount
583597
) throws IOException {
584598
try (Directory directory = newDirectory()) {
585599
try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
@@ -592,7 +606,7 @@ private void testCase(
592606
// When counting the number of collects, we want to record how many collects actually happened. The new composite type
593607
// ends up keeping track of the number of counts that happened, allowing us to verify whether the precomputation was used
594608
// or not.
595-
final CompositeAggregationAndCount missing = searchAndReduceCounting(indexSearcher, query, builder, fieldTypesArray);
609+
final InternalMissing missing = searchAndReduceCounting(expectedCount, indexSearcher, query, builder, fieldTypesArray);
596610
verify.accept(missing);
597611
}
598612
}

‎server/src/test/java/org/opensearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -632,6 +632,7 @@ private <A extends InternalAggregation> A executeTestCase(Query query, List<Long
632632
}
633633
}
634634

635+
// Executes the tests while indexing the values for the field names.
635636
private <A extends InternalAggregation> A executeTestCaseIndexString(
636637
Query query,
637638
List<Long> dataset,

0 commit comments

Comments
 (0)