Create separate SourceLookup instance per segment slice in Significan…

…tTextAggregatorFactory (#8807) (#8880) * Remove flakey assertion in SearchTimeoutIT * Create separate SourceLookup instance per segment slice in SignificantTextAggregatorFactory --------- Signed-off-by: Jay Deng <[email protected]>
opensearch-project · Jul 25, 2023 · 4897ad5 · 4897ad5
1 parent cfa5573
commit 4897ad5
Show file tree

Hide file tree

Showing 3 changed files with 4 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -28,6 +28,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Remote Segment Store Repository setting moved from `index.remote_store.repository` to `index.remote_store.segment.repository` and `cluster.remote_store.repository` to `cluster.remote_store.segment.repository` respectively for Index and Cluster level settings ([#8719](https://github.com/opensearch-project/OpenSearch/pull/8719))
 - Change InternalSignificantTerms to sum shard-level superset counts only in final reduce ([#8735](https://github.com/opensearch-project/OpenSearch/pull/8735))
 - Exclude 'benchmarks' from codecov report ([#8805](https://github.com/opensearch-project/OpenSearch/pull/8805))
+- Create separate SourceLookup instance per segment slice in SignificantTextAggregatorFactory ([#8807](https://github.com/opensearch-project/OpenSearch/pull/8807))
 
 ### Deprecated
 

diff --git a/server/src/internalClusterTest/java/org/opensearch/search/SearchTimeoutIT.java b/server/src/internalClusterTest/java/org/opensearch/search/SearchTimeoutIT.java
@@ -79,7 +79,6 @@ public void testSimpleTimeout() throws Exception {
             .get();
         assertTrue(searchResponse.isTimedOut());
         assertEquals(0, searchResponse.getFailedShards());
-        assertTrue(numDocs > searchResponse.getHits().getTotalHits().value);
     }
 
     public void testSimpleDoesNotTimeout() throws Exception {

diff --git a/...ava/org/opensearch/search/aggregations/bucket/terms/SignificantTextAggregatorFactory.java b/...ava/org/opensearch/search/aggregations/bucket/terms/SignificantTextAggregatorFactory.java
@@ -148,7 +148,6 @@ protected Aggregator createInternal(
             : includeExclude.convertToStringFilter(DocValueFormat.RAW, maxRegexLength);
 
         MapStringTermsAggregator.CollectorSource collectorSource = new SignificantTextCollectorSource(
-            queryShardContext.lookup().source(),
             queryShardContext.bigArrays(),
             fieldType,
             sourceFieldNames,
@@ -186,13 +185,14 @@ private static class SignificantTextCollectorSource implements MapStringTermsAgg
         private ObjectArray<DuplicateByteSequenceSpotter> dupSequenceSpotters;
 
         SignificantTextCollectorSource(
-            SourceLookup sourceLookup,
             BigArrays bigArrays,
             MappedFieldType fieldType,
             String[] sourceFieldNames,
             boolean filterDuplicateText
         ) {
-            this.sourceLookup = sourceLookup;
+            // Create a new SourceLookup instance per aggregator instead of use the shared one from SearchLookup. This is fine because it
+            // will only be accessed by this Aggregator instance and not anywhere else.
+            this.sourceLookup = new SourceLookup();
             this.bigArrays = bigArrays;
             this.fieldType = fieldType;
             this.sourceFieldNames = sourceFieldNames;