From fed07a54ba6dc94e723267dd357a7bd4bd22f8c5 Mon Sep 17 00:00:00 2001
From: Alan Woodward <romseygeek@apache.org>
Date: Thu, 10 Feb 2022 09:24:57 +0000
Subject: [PATCH] Test bug in TimeSeriesIndexSearcherTests (#83730)

This test checks that documents are presented in tsid and then timestamp
order to collectors; it was not taking a deep copy of the tsid and so when
a change in tsid stayed on the same segment we could get inaccurate
comparisons as the BytesRef would change out from underneath us and
would compare as equal to the old timestamp, meaning that the check for
strictly increasing timestamp would fail.

Fixes #83647
---
 .../aggregations/timeseries/TimeSeriesIndexSearcher.java | 9 ++++++++-
 .../timeseries/TimeSeriesIndexSearcherTests.java         | 8 +++-----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java b/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java
index 4999346310f5..4837a291df98 100644
--- a/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java
+++ b/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java
@@ -20,6 +20,7 @@ import org.apache.lucene.search.Scorer;
 import org.apache.lucene.search.Weight;
 import org.apache.lucene.util.Bits;
 import org.apache.lucene.util.BytesRef;
+import org.apache.lucene.util.BytesRefBuilder;
 import org.apache.lucene.util.PriorityQueue;
 import org.elasticsearch.cluster.metadata.DataStream;
 import org.elasticsearch.index.mapper.TimeSeriesIdFieldMapper;
@@ -99,7 +100,7 @@ public class TimeSeriesIndexSearcher {
                 it.remove();
                 continue;
             }
-            BytesRef tsid = leafWalker.tsids.lookupOrd(leafWalker.tsids.ordValue());
+            BytesRef tsid = leafWalker.getTsid();
             if (currentTsid == null) {
                 currentTsid = tsid;
             }
@@ -136,6 +137,7 @@ public class TimeSeriesIndexSearcher {
         private final DocIdSetIterator iterator;
         private final SortedDocValues tsids;
         private final SortedNumericDocValues timestamps;    // TODO can we have this just a NumericDocValues?
+        private final BytesRefBuilder scratch = new BytesRefBuilder();
         int docId = -1;
         int tsidOrd;
         long timestamp;
@@ -168,6 +170,11 @@ public class TimeSeriesIndexSearcher {
             return docId;
         }
 
+        BytesRef getTsid() throws IOException {
+            scratch.copyBytes(tsids.lookupOrd(tsids.ordValue()));
+            return scratch.get();
+        }
+
         // invalid if the doc is deleted or if it doesn't have a tsid or timestamp entry
         private boolean isInvalidDoc(int docId) throws IOException {
             return (liveDocs != null && liveDocs.get(docId) == false)
diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java
index d68099727541..670a6b1f1d31 100644
--- a/server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java
+++ b/server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java
@@ -26,7 +26,6 @@ import org.apache.lucene.search.Sort;
 import org.apache.lucene.search.SortField;
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.util.BytesRef;
-import org.apache.lucene.util.LuceneTestCase.AwaitsFix;
 import org.elasticsearch.cluster.metadata.DataStream;
 import org.elasticsearch.index.mapper.TimeSeriesIdFieldMapper;
 import org.elasticsearch.search.aggregations.BucketCollector;
@@ -40,7 +39,6 @@ import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 
-@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/83647")
 public class TimeSeriesIndexSearcherTests extends ESTestCase {
 
     // Index a random set of docs with timestamp and tsid with the tsid/timestamp sort order
@@ -108,13 +106,13 @@ public class TimeSeriesIndexSearcherTests extends ESTestCase {
                         BytesRef latestTSID = tsid.lookupOrd(tsid.ordValue());
                         long latestTimestamp = timestamp.longValue();
                         if (currentTSID != null) {
-                            assertTrue(latestTSID.compareTo(currentTSID) >= 0);
+                            assertTrue(currentTSID + "->" + latestTSID.utf8ToString(), latestTSID.compareTo(currentTSID) >= 0);
                             if (latestTSID.equals(currentTSID)) {
-                                assertTrue(latestTimestamp >= currentTimestamp);
+                                assertTrue(currentTimestamp + "->" + latestTimestamp, latestTimestamp >= currentTimestamp);
                             }
                         }
                         currentTimestamp = latestTimestamp;
-                        currentTSID = latestTSID;
+                        currentTSID = BytesRef.deepCopyOf(latestTSID);
                         total++;
                     }
                 };