From dbac80efa585bdf4562f06a5d3b75336ab6faaee Mon Sep 17 00:00:00 2001 From: Martin Gaievski Date: Thu, 30 Nov 2023 15:42:13 -0800 Subject: [PATCH] Addressing code comments Signed-off-by: Martin Gaievski --- CHANGELOG.md | 6 +-- .../query/HybridQueryPhaseSearcher.java | 52 +++++++++---------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c68a924f..093dbff70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Features ### Enhancements ### Bug Fixes -Fix async actions are left in neural_sparse query ([#438](https://github.com/opensearch-project/neural-search/pull/438)) -Fixed exception for case when Hybrid query being wrapped into bool query ([#490](https://github.com/opensearch-project/neural-search/pull/490)) -Hybrid query and nested type fields ([#498](https://github.com/opensearch-project/neural-search/pull/498)) +- Fix async actions are left in neural_sparse query ([#438](https://github.com/opensearch-project/neural-search/pull/438)) +- Fixed exception for case when Hybrid query being wrapped into bool query ([#490](https://github.com/opensearch-project/neural-search/pull/490)) +- Hybrid query and nested type fields ([#498](https://github.com/opensearch-project/neural-search/pull/498)) ### Infrastructure ### Documentation ### Maintenance diff --git a/src/main/java/org/opensearch/neuralsearch/search/query/HybridQueryPhaseSearcher.java b/src/main/java/org/opensearch/neuralsearch/search/query/HybridQueryPhaseSearcher.java index fd29aec29..ecfab7611 100644 --- a/src/main/java/org/opensearch/neuralsearch/search/query/HybridQueryPhaseSearcher.java +++ b/src/main/java/org/opensearch/neuralsearch/search/query/HybridQueryPhaseSearcher.java @@ -78,29 +78,29 @@ public boolean searchWith( private boolean isHybridQuery(final Query query, final SearchContext searchContext) { if (query instanceof HybridQuery) { return true; - } else if (hasNestedFieldOrNestedDocs(query, searchContext) && mightBeWrappedHybridQuery(query)) { - // checking if this is a hybrid query that is wrapped into a Bool query by core Opensearch code - // https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/search/DefaultSearchContext.java#L367-L370. - // main reason for that is performance optimization, at time of writing we are ok with loosing on performance if that's unblocks - // hybrid query for indexes with nested field types. - // in such case we consider query a valid hybrid query. Later in the code we will extract it and execute as a main query for - // this search request. - // below is sample structure of such query: - // - // Boolean { - // should: { - // hybrid: { - // sub_query1 {} - // sub_query2 {} - // } - // } - // filter: { - // exists: { - // field: "_primary_term" - // } - // } - // } - // TODO Need to add logic for passing hybrid sub-queries through the same logic in core to ensure there is no latency regression + } else if (hasNestedFieldOrNestedDocs(query, searchContext) && isWrappedHybridQuery(query)) { + /* Checking if this is a hybrid query that is wrapped into a Bool query by core Opensearch code + https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/search/DefaultSearchContext.java#L367-L370. + main reason for that is performance optimization, at time of writing we are ok with loosing on performance if that's unblocks + hybrid query for indexes with nested field types. + in such case we consider query a valid hybrid query. Later in the code we will extract it and execute as a main query for + this search request. + below is sample structure of such query: + + Boolean { + should: { + hybrid: { + sub_query1 {} + sub_query2 {} + } + } + filter: { + exists: { + field: "_primary_term" + } + } + } + TODO Need to add logic for passing hybrid sub-queries through the same logic in core to ensure there is no latency regression */ if (query instanceof BooleanQuery == false) { return false; } @@ -120,14 +120,14 @@ private boolean hasNestedFieldOrNestedDocs(final Query query, final SearchContex return searchContext.mapperService().hasNested() && new NestedHelper(searchContext.mapperService()).mightMatchNestedDocs(query); } - private boolean mightBeWrappedHybridQuery(final Query query) { + private boolean isWrappedHybridQuery(final Query query) { return query instanceof BooleanQuery && ((BooleanQuery) query).clauses().stream().anyMatch(clauseQuery -> clauseQuery.getQuery() instanceof HybridQuery); } private Query extractHybridQuery(final SearchContext searchContext, final Query query) { if (hasNestedFieldOrNestedDocs(query, searchContext) - && mightBeWrappedHybridQuery(query) + && isWrappedHybridQuery(query) && ((BooleanQuery) query).clauses().size() > 0) { // extract hybrid query and replace bool with hybrid query List booleanClauses = ((BooleanQuery) query).clauses(); @@ -165,7 +165,7 @@ private void validateQuery(final SearchContext searchContext, final Query query) } } - private void validateNestedBooleanQuery(final Query query, int level) { + private void validateNestedBooleanQuery(final Query query, final int level) { if (query instanceof HybridQuery) { throw new IllegalArgumentException("hybrid query must be a top level query and cannot be wrapped into other queries"); }