Removing unused fetch sub phase processor initialization during fetch… (

opensearch-project#12503) * Removing unused fetch sub phase processor initialization during fetch phase execution Signed-off-by: Ankit Jain <[email protected]> * Updating CHANGELOG.md with the bug fix Signed-off-by: Ankit Jain <[email protected]> * Fixing smoke test failures Signed-off-by: Ankit Jain <[email protected]> * Addressing review comments Signed-off-by: Ankit Jain <[email protected]> * Addressing review comments around boolean check convention Signed-off-by: Ankit Jain <[email protected]> --------- Signed-off-by: Ankit Jain <[email protected]>
rayshrey · Mar 18, 2024 · 50cd290 · 50cd290
1 parent c22956b
commit 50cd290
Show file tree

Hide file tree

Showing 8 changed files with 126 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -152,6 +152,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add a system property to configure YamlParser codepoint limits ([#12298](https://github.com/opensearch-project/OpenSearch/pull/12298))
 - Prevent read beyond slice boundary in ByteArrayIndexInput ([#10481](https://github.com/opensearch-project/OpenSearch/issues/10481))
 - Fix the "highlight.max_analyzer_offset" request parameter with "plain" highlighter ([#10919](https://github.com/opensearch-project/OpenSearch/pull/10919))
+- Prevent unnecessary fetch sub phase processor initialization during fetch phase execution ([#12503](https://github.com/opensearch-project/OpenSearch/pull/12503))
 - Warn about deprecated and ignored index.mapper.dynamic index setting ([#11193](https://github.com/opensearch-project/OpenSearch/pull/11193))
 - Fix `terms` query on `float` field when `doc_values` are turned off by reverting back to `FloatPoint` from `FloatField` ([#12499](https://github.com/opensearch-project/OpenSearch/pull/12499))
 - Fix get task API does not refresh resource stats ([#11531](https://github.com/opensearch-project/OpenSearch/pull/11531))

diff --git a/server/src/main/java/org/opensearch/search/fetch/FetchContext.java b/server/src/main/java/org/opensearch/search/fetch/FetchContext.java
@@ -192,6 +192,10 @@ public boolean includeNamedQueriesScore() {
         return searchContext.includeNamedQueriesScore();
     }
 
+    public boolean hasInnerHits() {
+        return searchContext.hasInnerHits();
+    }
+
     /**
      * Configuration for returning inner hits
      */
@@ -213,6 +217,10 @@ public FetchFieldsContext fetchFieldsContext() {
         return searchContext.fetchFieldsContext();
     }
 
+    public boolean hasScriptFields() {
+        return searchContext.hasScriptFields();
+    }
+
     /**
      * Configuration for script fields
      */

diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/InnerHitsContext.java b/server/src/main/java/org/opensearch/search/fetch/subphase/InnerHitsContext.java
@@ -119,6 +119,11 @@ public String getName() {
             return name;
         }
 
+        @Override
+        public boolean hasInnerHits() {
+            return childInnerHits != null;
+        }
+
         @Override
         public InnerHitsContext innerHits() {
             return childInnerHits;

diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/InnerHitsPhase.java b/server/src/main/java/org/opensearch/search/fetch/subphase/InnerHitsPhase.java
@@ -64,7 +64,7 @@ public InnerHitsPhase(FetchPhase fetchPhase) {
 
     @Override
     public FetchSubPhaseProcessor getProcessor(FetchContext searchContext) {
-        if (searchContext.innerHits() == null) {
+        if (searchContext.hasInnerHits() == false) {
             return null;
         }
         Map<String, InnerHitsContext.InnerHitSubContext> innerHits = searchContext.innerHits().getInnerHits();

diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/ScriptFieldsPhase.java b/server/src/main/java/org/opensearch/search/fetch/subphase/ScriptFieldsPhase.java
@@ -54,7 +54,7 @@ public final class ScriptFieldsPhase implements FetchSubPhase {
 
     @Override
     public FetchSubPhaseProcessor getProcessor(FetchContext context) {
-        if (context.scriptFields() == null) {
+        if (context.hasScriptFields() == false) {
             return null;
         }
         List<ScriptFieldsContext.ScriptField> scriptFields = context.scriptFields().fields();

diff --git a/server/src/main/java/org/opensearch/search/internal/SearchContext.java b/server/src/main/java/org/opensearch/search/internal/SearchContext.java
@@ -188,6 +188,10 @@ public final void close() {
 
     public abstract void highlight(SearchHighlightContext highlight);
 
+    public boolean hasInnerHits() {
+        return innerHitsContext != null;
+    }
+
     public InnerHitsContext innerHits() {
         if (innerHitsContext == null) {
             innerHitsContext = new InnerHitsContext();

diff --git a/server/src/test/java/org/opensearch/search/fetch/subphase/InnerHitsPhaseTests.java b/server/src/test/java/org/opensearch/search/fetch/subphase/InnerHitsPhaseTests.java
@@ -0,0 +1,53 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.search.fetch.subphase;
+
+import org.opensearch.index.query.QueryShardContext;
+import org.opensearch.search.fetch.FetchContext;
+import org.opensearch.search.internal.SearchContext;
+import org.opensearch.search.lookup.SearchLookup;
+import org.opensearch.test.OpenSearchTestCase;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class InnerHitsPhaseTests extends OpenSearchTestCase {
+
+    /*
+    Returns mock search context reused across test methods
+     */
+    private SearchContext getMockSearchContext(final boolean hasInnerHits) {
+        final QueryShardContext queryShardContext = mock(QueryShardContext.class);
+        when(queryShardContext.newFetchLookup()).thenReturn(mock(SearchLookup.class));
+
+        final SearchContext searchContext = mock(SearchContext.class);
+        when(searchContext.hasInnerHits()).thenReturn(hasInnerHits);
+        when(searchContext.getQueryShardContext()).thenReturn(queryShardContext);
+
+        return searchContext;
+    }
+
+    /*
+    Validates that InnerHitsPhase processor is not initialized when no inner hits
+     */
+    public void testInnerHitsNull() {
+        assertNull(new InnerHitsPhase(null).getProcessor(new FetchContext(getMockSearchContext(false))));
+    }
+
+    /*
+    Validates that InnerHitsPhase processor is initialized when inner hits are present
+     */
+    public void testInnerHitsNonNull() {
+        final SearchContext searchContext = getMockSearchContext(true);
+        when(searchContext.innerHits()).thenReturn(new InnerHitsContext());
+
+        assertNotNull(new InnerHitsPhase(null).getProcessor(new FetchContext(searchContext)));
+    }
+
+}
diff --git a/server/src/test/java/org/opensearch/search/fetch/subphase/ScriptFieldsPhaseTests.java b/server/src/test/java/org/opensearch/search/fetch/subphase/ScriptFieldsPhaseTests.java
@@ -0,0 +1,53 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.search.fetch.subphase;
+
+import org.opensearch.index.query.QueryShardContext;
+import org.opensearch.search.fetch.FetchContext;
+import org.opensearch.search.internal.SearchContext;
+import org.opensearch.search.lookup.SearchLookup;
+import org.opensearch.test.OpenSearchTestCase;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class ScriptFieldsPhaseTests extends OpenSearchTestCase {
+
+    /*
+    Returns mock search context reused across test methods
+     */
+    private SearchContext getMockSearchContext(final boolean hasScriptFields) {
+        final QueryShardContext queryShardContext = mock(QueryShardContext.class);
+        when(queryShardContext.newFetchLookup()).thenReturn(mock(SearchLookup.class));
+
+        final SearchContext searchContext = mock(SearchContext.class);
+        when(searchContext.hasScriptFields()).thenReturn(hasScriptFields);
+        when(searchContext.getQueryShardContext()).thenReturn(queryShardContext);
+
+        return searchContext;
+    }
+
+    /*
+    Validates that ScriptFieldsPhase processor is not initialized when no script fields
+     */
+    public void testScriptFieldsNull() {
+        assertNull(new ScriptFieldsPhase().getProcessor(new FetchContext(getMockSearchContext(false))));
+    }
+
+    /*
+    Validates that ScriptFieldsPhase processor is initialized when script fields are present
+     */
+    public void testScriptFieldsNonNull() {
+        final SearchContext searchContext = getMockSearchContext(true);
+        when(searchContext.scriptFields()).thenReturn(new ScriptFieldsContext());
+
+        assertNotNull(new ScriptFieldsPhase().getProcessor(new FetchContext(searchContext)));
+    }
+
+}