Skip to content

Commit

Permalink
Fix docCountError calculation for multiple reduces
Browse files Browse the repository at this point in the history
Fix docCountError calculation in case of multiple reduces. It fixes 2 mistakes
in elastic#43874. The first error was introduced in the original PR, where unknown doc
count errors were initialized equal to 0, the second was introduced during in
order to fix the first one by ignoring these 0s, which essentially disabled the
original fix.

Fixes elastic#75667
  • Loading branch information
imotov committed Aug 11, 2021
1 parent dfcb341 commit 9443de3
Show file tree
Hide file tree
Showing 8 changed files with 8 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ private StringTerms newTerms(Random rand, BytesRef[] dict, boolean withNested) {
true,
0,
buckets,
0L
null
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ private StringTerms newTerms(boolean withNested) {
false,
100000,
resultBuckets,
0L
null
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1034,7 +1034,6 @@ public void testFixedDocs() throws Exception {
* Tests the upper bounds are correct when performing incremental reductions
* See https://github.com/elastic/elasticsearch/issues/40005 for more details
*/
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/75667")
public void testIncrementalReduction() {
SearchResponse response = client().prepareSearch("idx_fixed_docs_3", "idx_fixed_docs_4", "idx_fixed_docs_5")
.addAggregation(terms("terms")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ private long getDocCountError(A terms) {
if (size == 0 || size < terms.getShardSize() || isKeyOrder(terms.getOrder())) {
return 0;
} else if (InternalOrder.isCountDesc(terms.getOrder())) {
if (terms.getDocCountError() != null && terms.getDocCountError() > 0) {
if (terms.getDocCountError() != null) {
// If there is an existing docCountError for this agg then
// use this as the error for this aggregation
return terms.getDocCountError();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ StringTerms buildResult(long owningBucketOrd, long otherDocCount, StringTerms.Bu
}
return new StringTerms(name, reduceOrder, order, bucketCountThresholds.getRequiredSize(),
bucketCountThresholds.getMinDocCount(), metadata(), format, bucketCountThresholds.getShardSize(), showTermDocCountError,
otherDocCount, Arrays.asList(topBuckets), 0L);
otherDocCount, Arrays.asList(topBuckets), null);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ StringTerms buildResult(long owningBucketOrd, long otherDocCount, StringTerms.Bu
}
return new StringTerms(name, reduceOrder, order, bucketCountThresholds.getRequiredSize(),
bucketCountThresholds.getMinDocCount(), metadata(), format, bucketCountThresholds.getShardSize(), showTermDocCountError,
otherDocCount, Arrays.asList(topBuckets), 0L);
otherDocCount, Arrays.asList(topBuckets), null);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ LongTerms buildResult(long owningBucketOrd, long otherDocCount, LongTerms.Bucket
showTermDocCountError,
otherDocCount,
List.of(topBuckets),
0L
null
);
}

Expand Down Expand Up @@ -454,7 +454,7 @@ DoubleTerms buildResult(long owningBucketOrd, long otherDocCount, DoubleTerms.Bu
showTermDocCountError,
otherDocCount,
List.of(topBuckets),
0L
null
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ protected boolean lessThan(OrdBucket a, OrdBucket b) {
showTermDocCountError,
otherDocsCount,
buckets,
0L
null
);
}

Expand Down

0 comments on commit 9443de3

Please sign in to comment.