From da5b2053864f24dcb3b8c555a9fd19e0d91a14e6 Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Thu, 14 Mar 2024 04:42:53 +0530 Subject: [PATCH] =?UTF-8?q?Removing=20unused=20fetch=20sub=20phase=20proce?= =?UTF-8?q?ssor=20initialization=20during=20fetch=E2=80=A6=20(#12503)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Removing unused fetch sub phase processor initialization during fetch phase execution Signed-off-by: Ankit Jain * Updating CHANGELOG.md with the bug fix Signed-off-by: Ankit Jain * Fixing smoke test failures Signed-off-by: Ankit Jain * Addressing review comments Signed-off-by: Ankit Jain * Addressing review comments around boolean check convention Signed-off-by: Ankit Jain --------- Signed-off-by: Ankit Jain --- CHANGELOG.md | 1 + .../opensearch/search/fetch/FetchContext.java | 8 +++ .../fetch/subphase/InnerHitsContext.java | 5 ++ .../search/fetch/subphase/InnerHitsPhase.java | 2 +- .../fetch/subphase/ScriptFieldsPhase.java | 2 +- .../search/internal/SearchContext.java | 4 ++ .../fetch/subphase/InnerHitsPhaseTests.java | 53 +++++++++++++++++++ .../subphase/ScriptFieldsPhaseTests.java | 53 +++++++++++++++++++ 8 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 server/src/test/java/org/opensearch/search/fetch/subphase/InnerHitsPhaseTests.java create mode 100644 server/src/test/java/org/opensearch/search/fetch/subphase/ScriptFieldsPhaseTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index fb48e73e0b514..19a9a7461500f 100644 --- 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 index 5be3733106655..780a6f35524ea 100644 --- 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 index 5855a0b3217f3..fa80bb04c77f5 100644 --- 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 index 0b07dc35f13bb..cadad8529da9d 100644 --- 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 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 index 67d1863050a7b..bee536dbaf7f6 100644 --- 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 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 index cd8f9f8410d50..3d13378e58e5d 100644 --- 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 new file mode 100644 index 0000000000000..7ca5977a1c276 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..eb6338997ab9f --- /dev/null +++ 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))); + } + +}