From ba913e30f6f323272def048d10a0a6735d86d8ca Mon Sep 17 00:00:00 2001
From: Nik Everett <nik9000@gmail.com>
Date: Mon, 3 Feb 2020 16:03:09 -0500
Subject: [PATCH 1/2] Fix a sneaky bug in rare_terms

When the `rare_terms` aggregation contained another aggregation it'd
break them. Most of the time. This happened because the process that it
uses to remove buckets that turn out not to be rare was incorrectly
merging results from multiple leaves. This'd cause array index out of
bounds issues. We didn't catch it in the test because the issue doesn't
happen on the very first bucket. And the tests generated data in such a
way that the first bucket always contained the rare terms. Randomizing
the order of the generated data fixed the test so it caught the issue.

Closes #51020
---
 .../search.aggregation/280_rare_terms.yml     | 42 +++++++++++++++++++
 .../terms/AbstractRareTermsAggregator.java    |  7 ++--
 .../bucket/terms/LongRareTermsAggregator.java |  5 +--
 .../terms/StringRareTermsAggregator.java      |  5 +--
 .../terms/RareTermsAggregatorTests.java       |  4 +-
 5 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
index a82caddd9cfd4..5864acfd4a4c2 100644
--- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
+++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
@@ -313,4 +313,46 @@ setup:
   - match: { hits.total.value: 1 }
   - length: { aggregations.long_terms.buckets: 0 }
 
+---
+"sub aggs":
+  - do:
+      index:
+        refresh: true
+        index: test_1
+        id: 1
+        body: { "str" : "abc", "number": 1 }
+
+  - do:
+      index:
+        refresh: true
+        index: test_1
+        id: 2
+        body: { "str": "abc", "number": 2 }
 
+  - do:
+      index:
+        refresh: true
+        index: test_1
+        id: 3
+        body: { "str": "bcd", "number": 3 }
+
+  - do:
+      search:
+        body:
+          size: 0
+          aggs:
+            str_terms:
+              rare_terms:
+                field: str
+                max_doc_count: 1
+              aggs:
+                max_n:
+                  max:
+                    field: number
+
+  - match: { hits.total.value: 3 }
+  - length: { aggregations.str_terms.buckets: 1 }
+  - match: { aggregations.str_terms.buckets.0.key: "bcd" }
+  - is_false: aggregations.str_terms.buckets.0.key_as_string
+  - match: { aggregations.str_terms.buckets.0.doc_count: 1 }
+  - match: { aggregations.str_terms.buckets.0.max_n.value: 3.0 }
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractRareTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractRareTermsAggregator.java
index 2bbe3c01988df..bacc2ef217004 100644
--- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractRareTermsAggregator.java
+++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/AbstractRareTermsAggregator.java
@@ -50,7 +50,6 @@ public abstract class AbstractRareTermsAggregator<T extends ValuesSource,
     protected final U includeExclude;
 
     MergingBucketsDeferringCollector deferringCollector;
-    LeafBucketCollector subCollectors;
     final SetBackedScalingCuckooFilter filter;
 
     AbstractRareTermsAggregator(String name, AggregatorFactories factories, SearchContext context,
@@ -115,14 +114,14 @@ private String descendsFromNestedAggregator(Aggregator parent) {
         return null;
     }
 
-    protected void doCollect(V val, int docId) throws IOException {
+    protected void doCollect(LeafBucketCollector subCollector, V val, int docId) throws IOException {
         long bucketOrdinal = addValueToOrds(val);
 
         if (bucketOrdinal < 0) { // already seen
             bucketOrdinal = -1 - bucketOrdinal;
-            collectExistingBucket(subCollectors, docId, bucketOrdinal);
+            collectExistingBucket(subCollector, docId, bucketOrdinal);
         } else {
-            collectBucket(subCollectors, docId, bucketOrdinal);
+            collectBucket(subCollector, docId, bucketOrdinal);
         }
     }
 
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongRareTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongRareTermsAggregator.java
index b1d294fefdcf6..7c79e2db6b832 100644
--- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongRareTermsAggregator.java
+++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongRareTermsAggregator.java
@@ -64,9 +64,6 @@ protected SortedNumericDocValues getValues(ValuesSource.Numeric valuesSource, Le
     public LeafBucketCollector getLeafCollector(LeafReaderContext ctx,
                                                 final LeafBucketCollector sub) throws IOException {
         final SortedNumericDocValues values = getValues(valuesSource, ctx);
-        if (subCollectors == null) {
-            subCollectors = sub;
-        }
         return new LeafBucketCollectorBase(sub, values) {
 
             @Override
@@ -78,7 +75,7 @@ public void collect(int docId, long owningBucketOrdinal) throws IOException {
                         final long val = values.nextValue();
                         if (previous != val || i == 0) {
                             if ((includeExclude == null) || (includeExclude.accept(val))) {
-                                doCollect(val, docId);
+                                doCollect(sub, val, docId);
                             }
                             previous = val;
                         }
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringRareTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringRareTermsAggregator.java
index 0c200e96b242c..29bb46140ec4b 100644
--- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringRareTermsAggregator.java
+++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/StringRareTermsAggregator.java
@@ -60,9 +60,6 @@ public class StringRareTermsAggregator extends AbstractRareTermsAggregator<Value
     public LeafBucketCollector getLeafCollector(LeafReaderContext ctx,
                                                 final LeafBucketCollector sub) throws IOException {
         final SortedBinaryDocValues values = valuesSource.bytesValues(ctx);
-        if (subCollectors == null) {
-            subCollectors = sub;
-        }
         return new LeafBucketCollectorBase(sub, values) {
             final BytesRefBuilder previous = new BytesRefBuilder();
 
@@ -84,7 +81,7 @@ public void collect(int docId, long bucket) throws IOException {
                             continue;
                         }
 
-                        doCollect(bytes, docId);
+                        doCollect(sub, bytes, docId);
                         previous.copyBytes(bytes);
                     }
                 }
diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java
index a3b8104814db9..94bb43f3db62c 100644
--- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java
+++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregatorTests.java
@@ -562,7 +562,9 @@ private void executeTestCase(boolean reduced, Query query, List<Long> dataset,
         try (Directory directory = newDirectory()) {
             try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
                 Document document = new Document();
-                for (Long value : dataset) {
+                List<Long> shuffledDataset = new ArrayList<>(dataset);
+                Collections.shuffle(shuffledDataset, random());
+                for (Long value : shuffledDataset) {
                     if (frequently()) {
                         indexWriter.commit();
                     }

From be0c4ab316b60e427694c2a95b8caaa3231336a7 Mon Sep 17 00:00:00 2001
From: Nik Everett <nik9000@gmail.com>
Date: Tue, 4 Feb 2020 10:25:12 -0500
Subject: [PATCH 2/2] Skip old

---
 .../rest-api-spec/test/search.aggregation/280_rare_terms.yml  | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
index 5864acfd4a4c2..04d76a987809d 100644
--- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
+++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/280_rare_terms.yml
@@ -315,6 +315,10 @@ setup:
 
 ---
 "sub aggs":
+  - skip:
+      version: " - 7.99.99"
+      reason: Sub aggs fixed in 8.0 (to be backported to 7.6.1)
+
   - do:
       index:
         refresh: true