From 5133e5fd881402fcd9a4d6dcc1845aea5f73e0ec Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 1 May 2024 15:33:18 -0400 Subject: [PATCH] =?UTF-8?q?Revert=20"Removing=20unused=20fetch=20sub=20pha?= =?UTF-8?q?se=20processor=20initialization=20during=20fetch=E2=80=A6=20(#1?= =?UTF-8?q?2503)"=20(#13486)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit da5b2053864f24dcb3b8c555a9fd19e0d91a14e6. Signed-off-by: Andriy Redko --- CHANGELOG.md | 1 + .../search.aggregation/400_inner_hits.yml | 65 +++++++++++++++++++ .../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 --------------- 9 files changed, 68 insertions(+), 125 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/400_inner_hits.yml delete mode 100644 server/src/test/java/org/opensearch/search/fetch/subphase/InnerHitsPhaseTests.java delete mode 100644 server/src/test/java/org/opensearch/search/fetch/subphase/ScriptFieldsPhaseTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index ec85052873407..5bff49af99473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Refactoring globMatch using simpleMatchWithNormalizedStrings from Regex ([#13104](https://github.com/opensearch-project/OpenSearch/pull/13104)) - [BWC and API enforcement] Reconsider the breaking changes check policy to detect breaking changes against released versions ([#13292](https://github.com/opensearch-project/OpenSearch/pull/13292)) - Switch to macos-13 runner for precommit and assemble github actions due to macos-latest is now arm64 ([#13412](https://github.com/opensearch-project/OpenSearch/pull/13412)) +- [Revert] Prevent unnecessary fetch sub phase processor initialization during fetch phase execution ([#12503](https://github.com/opensearch-project/OpenSearch/pull/12503)) ### Deprecated diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/400_inner_hits.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/400_inner_hits.yml new file mode 100644 index 0000000000000..d4584a251816e --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/400_inner_hits.yml @@ -0,0 +1,65 @@ +setup: + - do: + indices.create: + index: test_1 + body: + settings: + number_of_replicas: 0 + mappings: + properties: + list_id: + type: integer + names: + type: nested + properties: + full_name: + type: text + + - do: + bulk: + refresh: true + body: + - index: + _index: test_1 + _id: 1 + - list_id: 1 + names: + - full_name: John Doe + - full_name: John Micheal Doe + - index: + _index: test_1 + _id: 2 + - list_id: 2 + names: + - full_name: Jane Doe + - full_name: Jane Michelle Doe + +--- +"Include inner hits in top hits": + - do: + search: + rest_total_hits_as_int: true + body: + query: + nested: + path: names + query: + match: + names.full_name: Doe + inner_hits: { } + size: 0 + aggs: + lists: + terms: + field: list_id + aggs: + top_result: + top_hits: + size: 10 + + - length: { hits.hits: 0 } + - length: { aggregations.lists.buckets: 2 } + - length: { aggregations.lists.buckets.0.top_result.hits.hits: 1 } + - length: { aggregations.lists.buckets.0.top_result.hits.hits.0.inner_hits.names.hits.hits: 2 } + - length: { aggregations.lists.buckets.1.top_result.hits.hits: 1 } + - length: { aggregations.lists.buckets.1.top_result.hits.hits.0.inner_hits.names.hits.hits: 2 } 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 780a6f35524ea..5be3733106655 100644 --- a/server/src/main/java/org/opensearch/search/fetch/FetchContext.java +++ b/server/src/main/java/org/opensearch/search/fetch/FetchContext.java @@ -192,10 +192,6 @@ public boolean includeNamedQueriesScore() { return searchContext.includeNamedQueriesScore(); } - public boolean hasInnerHits() { - return searchContext.hasInnerHits(); - } - /** * Configuration for returning inner hits */ @@ -217,10 +213,6 @@ 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 fa80bb04c77f5..5855a0b3217f3 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,11 +119,6 @@ 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 cadad8529da9d..0b07dc35f13bb 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.hasInnerHits() == false) { + if (searchContext.innerHits() == null) { 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 bee536dbaf7f6..67d1863050a7b 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.hasScriptFields() == false) { + if (context.scriptFields() == null) { 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 07f3616bbc138..0c8240d3a8322 100644 --- a/server/src/main/java/org/opensearch/search/internal/SearchContext.java +++ b/server/src/main/java/org/opensearch/search/internal/SearchContext.java @@ -194,10 +194,6 @@ 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 deleted file mode 100644 index 7ca5977a1c276..0000000000000 --- a/server/src/test/java/org/opensearch/search/fetch/subphase/InnerHitsPhaseTests.java +++ /dev/null @@ -1,53 +0,0 @@ -/* - * 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 deleted file mode 100644 index eb6338997ab9f..0000000000000 --- a/server/src/test/java/org/opensearch/search/fetch/subphase/ScriptFieldsPhaseTests.java +++ /dev/null @@ -1,53 +0,0 @@ -/* - * 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))); - } - -}