Skip to content

Commit

Permalink
Fix random failures in InternalTopHitsTests#testReduceRandom (#53832)
Browse files Browse the repository at this point in the history
The test was randomly and very rarely failing due to generating the same sort
key for multiple records, which was making order of these records in the results
nondeterministic. While investigating the test I also found that the data wasn't
generated in the way that matches the actual data. Normally, the order of
documents in hits and scoreDocs in InternalTopHits should be the same. However,
in the test only scoreDocs were sorted which was cause very confusing failure
messages. This commit fixes this issue as well.

Fixes #53676
  • Loading branch information
imotov committed Mar 20, 2020
1 parent 0d66ab2 commit 2671530
Showing 1 changed file with 28 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.elasticsearch.search.SearchHits;
import org.elasticsearch.search.aggregations.ParsedAggregation;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.InternalAggregationTestCase;

import java.io.IOException;
Expand Down Expand Up @@ -62,6 +63,12 @@ public class InternalTopHitsTests extends InternalAggregationTestCase<InternalTo
*/
private SortField[] testInstancesSortFields;

/**
* Collects all generated scores and fields to ensure that all scores are unique. That is necessary for deterministic results
*/
private Set<Float> usedScores = new HashSet<>();
private Set<Object> usedFields = new HashSet<>();

@Override
public void setUp() throws Exception {
super.setUp();
Expand All @@ -80,7 +87,8 @@ protected InternalTopHits createTestInstance(String name, List<PipelineAggregato
SearchHit[] hits = new SearchHit[actualSize];
Set<Integer> usedDocIds = new HashSet<>();
for (int i = 0; i < actualSize; i++) {
float score = randomFloat();
float score = randomValueOtherThanMany(usedScores::contains, ESTestCase::randomFloat);
usedScores.add(score);
maxScore = max(maxScore, score);
int docId = randomValueOtherThanMany(usedDocIds::contains, () -> between(0, IndexWriter.MAX_DOCS));
usedDocIds.add(docId);
Expand All @@ -89,7 +97,9 @@ protected InternalTopHits createTestInstance(String name, List<PipelineAggregato
if (testInstancesLookSortedByField) {
Object[] fields = new Object[testInstancesSortFields.length];
for (int f = 0; f < testInstancesSortFields.length; f++) {
fields[f] = randomOfType(testInstancesSortFields[f].getType());
final int ff = f;
fields[f] = randomValueOtherThanMany(usedFields::contains, () -> randomOfType(testInstancesSortFields[ff].getType()));
usedFields.add(fields[f]);
}
scoreDocs[i] = new FieldDoc(docId, score, fields);
} else {
Expand All @@ -99,10 +109,10 @@ protected InternalTopHits createTestInstance(String name, List<PipelineAggregato
hits[i].score(score);
}
int totalHits = between(actualSize, 500000);
sort(hits, scoreDocs, scoreDocComparator());
SearchHits searchHits = new SearchHits(hits, totalHits, maxScore);

TopDocs topDocs;
Arrays.sort(scoreDocs, scoreDocComparator());
if (testInstancesLookSortedByField) {
topDocs = new TopFieldDocs(totalHits, scoreDocs, testInstancesSortFields, maxScore);
} else {
Expand All @@ -112,6 +122,21 @@ protected InternalTopHits createTestInstance(String name, List<PipelineAggregato
return new InternalTopHits(name, from, requestedSize, topDocs, searchHits, pipelineAggregators, metaData);
}

/**
* Sorts both searchHits and scoreDocs together based on scoreDocComparator()
*/
private void sort(SearchHit[] searchHits, ScoreDoc[] scoreDocs, Comparator<ScoreDoc> comparator) {
List<Tuple<SearchHit, ScoreDoc>> hitScores = new ArrayList<>();
for (int i = 0; i < searchHits.length; i++) {
hitScores.add(new Tuple<>(searchHits[i], scoreDocs[i]));
}
hitScores.sort((t1, t2) -> comparator.compare(t1.v2(), t2.v2()));
for (int i = 0; i < searchHits.length; i++) {
searchHits[i] = hitScores.get(i).v1();
scoreDocs[i] = hitScores.get(i).v2();
}
}

@Override
protected void assertFromXContent(InternalTopHits aggregation, ParsedAggregation parsedAggregation) throws IOException {
final SearchHits expectedSearchHits = aggregation.getHits();
Expand Down

0 comments on commit 2671530

Please sign in to comment.