From 9443de32e8802cbf12e021f2c87f49225c46389e Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Wed, 11 Aug 2021 13:01:28 -1000 Subject: [PATCH] Fix docCountError calculation for multiple reduces Fix docCountError calculation in case of multiple reduces. It fixes 2 mistakes in #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 #75667 --- .../benchmark/search/aggregations/TermsReduceBenchmark.java | 2 +- .../bucket/terms/StringTermsSerializationBenchmark.java | 2 +- .../search/aggregations/bucket/TermsDocCountErrorIT.java | 1 - .../aggregations/bucket/terms/AbstractInternalTerms.java | 2 +- .../bucket/terms/GlobalOrdinalsStringTermsAggregator.java | 2 +- .../aggregations/bucket/terms/MapStringTermsAggregator.java | 2 +- .../aggregations/bucket/terms/NumericTermsAggregator.java | 4 ++-- .../bucket/terms/StringTermsAggregatorFromFilters.java | 2 +- 8 files changed, 8 insertions(+), 9 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/TermsReduceBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/TermsReduceBenchmark.java index dd8f4eadafc31..aa09ae4503095 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/TermsReduceBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/TermsReduceBenchmark.java @@ -154,7 +154,7 @@ private StringTerms newTerms(Random rand, BytesRef[] dict, boolean withNested) { true, 0, buckets, - 0L + null ); } diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java index 93c45f7b5c7aa..a5786b75e05cf 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java @@ -70,7 +70,7 @@ private StringTerms newTerms(boolean withNested) { false, 100000, resultBuckets, - 0L + null ); } diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/TermsDocCountErrorIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/TermsDocCountErrorIT.java index 14db3b32bb2be..a2e76892342ba 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/TermsDocCountErrorIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/TermsDocCountErrorIT.java @@ -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") diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractInternalTerms.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractInternalTerms.java index a02e0811ced25..d22e354971c74 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractInternalTerms.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractInternalTerms.java @@ -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(); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java index 7ca079f5aefdd..e4d44114166ca 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java @@ -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 diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/MapStringTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/MapStringTermsAggregator.java index bb12d9049e65e..a7911bcdc3590 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/MapStringTermsAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/MapStringTermsAggregator.java @@ -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 diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/NumericTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/NumericTermsAggregator.java index 97a2a73deba54..6469edbba2281 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/NumericTermsAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/NumericTermsAggregator.java @@ -372,7 +372,7 @@ LongTerms buildResult(long owningBucketOrd, long otherDocCount, LongTerms.Bucket showTermDocCountError, otherDocCount, List.of(topBuckets), - 0L + null ); } @@ -454,7 +454,7 @@ DoubleTerms buildResult(long owningBucketOrd, long otherDocCount, DoubleTerms.Bu showTermDocCountError, otherDocCount, List.of(topBuckets), - 0L + null ); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringTermsAggregatorFromFilters.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringTermsAggregatorFromFilters.java index cde2e22ccf5f3..6acd90a910547 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringTermsAggregatorFromFilters.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringTermsAggregatorFromFilters.java @@ -227,7 +227,7 @@ protected boolean lessThan(OrdBucket a, OrdBucket b) { showTermDocCountError, otherDocsCount, buckets, - 0L + null ); }