From 16b7c191824af1026100c1d21a1946464e3687d1 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sun, 17 Dec 2023 20:02:43 +0100 Subject: [PATCH 01/23] add support for scored named queries Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- CHANGELOG.md | 1 + .../core/common/io/stream/StreamInput.java | 71 +++++++++ .../resources/rest-api-spec/api/search.json | 5 + .../test/search/350_matched_queries.yml | 103 ++++++++++++ .../fetch/subphase/MatchedQueriesIT.java | 96 ++++++----- .../search/functionscore/QueryRescorerIT.java | 2 +- .../rest/action/search/RestSearchAction.java | 6 +- .../java/org/opensearch/search/SearchHit.java | 137 +++++++++------- .../fetch/subphase/MatchedQueriesPhase.java | 32 ++-- .../common/io/stream/BytesStreamsTests.java | 34 ++++ .../org/opensearch/search/SearchHitTests.java | 150 +++++++++++++++++- 11 files changed, 522 insertions(+), 115 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/search/350_matched_queries.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index c5ea3f83bffc7..e10e795fbec70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -121,6 +121,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Adding slf4j license header to LoggerMessageFormat.java ([#11069](https://github.com/opensearch-project/OpenSearch/pull/11069)) - [BWC and API enforcement] Introduce checks for enforcing the API restrictions ([#11175](https://github.com/opensearch-project/OpenSearch/pull/11175)) - Create separate transport action for render search template action ([#11170](https://github.com/opensearch-project/OpenSearch/pull/11170)) +- Backwards compatible support for returning scores in matched queries ((https://github.com/opensearch-project/OpenSearch/pull/11549)) ### Dependencies - Bump Lucene from 9.7.0 to 9.8.0 ([10276](https://github.com/opensearch-project/OpenSearch/pull/10276)) diff --git a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java index 3e996bdee83a2..ac4e0f50b924c 100644 --- a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java +++ b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java @@ -632,6 +632,77 @@ public String[] readOptionalStringArray() throws IOException { return null; } + + /** + * Reads an ordered {@link Map} from a data source using provided key and value readers. + * + * This method creates a `LinkedHashMap`, ensuring that the iteration order of the map + * matches the order in which the entries were read from the data source. It uses the + * `keyReader` and `valueReader` to read each key-value pair and constructs the map + * with an initial capacity optimized based on the expected size. + * + * Usage example: + * 

+     * Map<Integer, String> orderedMap = readOrderedMap(StreamInput::readInteger, StreamInput::readString);
+     *
+ * + * @param keyReader The {@link Writeable.Reader} used to read the keys of the map. + * @param valueReader The {@link Writeable.Reader} used to read the values of the map. + * @return A {@link LinkedHashMap} containing the keys and values read from the data source. + * The map maintains the order in which keys and values were read. + * @throws IOException If an I/O error occurs during reading from the data source. + */ + public Map readOrderedMap(Writeable.Reader keyReader, Writeable.Reader valueReader) throws IOException { + return readMap(keyReader, valueReader, (expectedSize) -> new LinkedHashMap<>(capacity(expectedSize))); + } + + static int capacity(int expectedSize) { + assert expectedSize >= 0; + return expectedSize < 2 ? expectedSize + 1 : (int) (expectedSize / 0.75 + 1.0); + } + + /** + * Reads a {@link Map} from a data source using provided key and value readers and a map constructor. + * + * This method is a flexible utility for reading a map from a data source, such as a file or network stream. + * It uses the provided `keyReader` and `valueReader` to read each key-value pair. The size of the map is + * determined first by reading the array size. A map constructor is also provided to create a map of the + * appropriate size, allowing for optimized performance based on the expected number of entries. + * + * If the read size is zero, an empty map is returned. Otherwise, the method iterates over the size, + * reading keys and values and adding them to the map. + * + * Usage example: + * 

+     * Map<Integer, String> map = readMap(StreamInput::readInteger, StreamInput::readString, HashMap::new);
+     *
+ * + * Note: If the map is empty, an immutable empty map is returned. Otherwise, a mutable map is created. + * + * @param keyReader The {@link Writeable.Reader} used to read the keys of the map. + * @param valueReader The {@link Writeable.Reader} used to read the values of the map. + * @param constructor A function that takes an integer (the initial size) and returns a new map. + * This allows for the creation of a map with an initial capacity matching the + * expected size, optimizing performance. + * @return A {@link Map} containing the keys and values read from the data source. This map is either + * empty (and immutable) or mutable and filled with the read data. + * @throws IOException If an I/O error occurs during reading from the data source. + */ + private Map readMap(Writeable.Reader keyReader, Writeable.Reader valueReader, IntFunction> constructor) + throws IOException { + int size = readArraySize(); + if (size == 0) { + return Collections.emptyMap(); + } + Map map = constructor.apply(size); + for (int i = 0; i < size; i++) { + K key = keyReader.read(this); + V value = valueReader.read(this); + map.put(key, value); + } + return map; + } + /** * If the returned map contains any entries it will be mutable. If it is empty it might be immutable. */ diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/search.json b/rest-api-spec/src/main/resources/rest-api-spec/api/search.json index e0fbeeb83ffc4..e78d49a67a98a 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/api/search.json +++ b/rest-api-spec/src/main/resources/rest-api-spec/api/search.json @@ -229,6 +229,11 @@ "search_pipeline": { "type": "string", "description": "The search pipeline to use to execute this request" + }, + "include_named_queries_score":{ + "type": "boolean", + "description":"Indicates whether hit.matched_queries should be rendered as a map that includes the name of the matched query associated with its score (true) or as an array containing the name of the matched queries (false)", + "default":false } }, "body":{ diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_matched_queries.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_matched_queries.yml new file mode 100644 index 0000000000000..25de51a316bd4 --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/350_matched_queries.yml @@ -0,0 +1,103 @@ +setup: + - skip: + version: " - 2.12.0" + reason: "implemented for versions post 2.12.0" + +--- +"matched queries": + - do: + indices.create: + index: test + + - do: + bulk: + refresh: true + body: + - '{ "index" : { "_index" : "test_1", "_id" : "1" } }' + - '{"field" : 1 }' + - '{ "index" : { "_index" : "test_1", "_id" : "2" } }' + - '{"field" : [1, 2] }' + + - do: + search: + index: test_1 + body: + query: + bool: { + should: [ + { + match: { + field: { + query: 1, + _name: match_field_1 + } + } + }, + { + match: { + field: { + query: 2, + _name: match_field_2, + boost: 10 + } + } + } + ] + } + + - match: {hits.total.value: 2} + - length: {hits.hits.0.matched_queries: 2} + - match: {hits.hits.0.matched_queries: [ "match_field_1", "match_field_2" ]} + - length: {hits.hits.1.matched_queries: 1} + - match: {hits.hits.1.matched_queries: [ "match_field_1" ]} + +--- + +"matched queries with scores": + - do: + indices.create: + index: test + + - do: + bulk: + refresh: true + body: + - '{ "index" : { "_index" : "test_1", "_id" : "1" } }' + - '{"field" : 1 }' + - '{ "index" : { "_index" : "test_1", "_id" : "2" } }' + - '{"field" : [1, 2] }' + + - do: + search: + include_named_queries_score: true + index: test_1 + body: + query: + bool: { + should: [ + { + match: { + field: { + query: 1, + _name: match_field_1 + } + } + }, + { + match: { + field: { + query: 2, + _name: match_field_2, + boost: 10 + } + } + } + ] + } + + - match: { hits.total.value: 2 } + - length: { hits.hits.0.matched_queries: 2 } + - match: { hits.hits.0.matched_queries.match_field_1: 1 } + - match: { hits.hits.0.matched_queries.match_field_2: 10 } + - length: { hits.hits.1.matched_queries: 1 } + - match: { hits.hits.1.matched_queries.match_field_1: 1 } diff --git a/server/src/internalClusterTest/java/org/opensearch/search/fetch/subphase/MatchedQueriesIT.java b/server/src/internalClusterTest/java/org/opensearch/search/fetch/subphase/MatchedQueriesIT.java index 83cedb8c20e1d..36d3d2cfa59ed 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/fetch/subphase/MatchedQueriesIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/fetch/subphase/MatchedQueriesIT.java @@ -62,7 +62,9 @@ import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasItemInArray; +import static org.hamcrest.Matchers.hasKey; public class MatchedQueriesIT extends ParameterizedOpenSearchIntegTestCase { @@ -105,11 +107,13 @@ public void testSimpleMatchedQueryFromFilteredQuery() throws Exception { assertHitCount(searchResponse, 3L); for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("3") || hit.getId().equals("2")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("test2")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("test2")); + assertThat(hit.getMatchedQueryScore("test2"), equalTo(1f)); } else if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("test1")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("test1")); + assertThat(hit.getMatchedQueryScore("test1"), equalTo(1f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -123,11 +127,13 @@ public void testSimpleMatchedQueryFromFilteredQuery() throws Exception { assertHitCount(searchResponse, 3L); for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1") || hit.getId().equals("2")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("test1")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("test1")); + assertThat(hit.getMatchedQueryScore("test1"), equalTo(1f)); } else if (hit.getId().equals("3")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("test2")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("test2")); + assertThat(hit.getMatchedQueryScore("test2"), equalTo(1f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -153,12 +159,15 @@ public void testSimpleMatchedQueryFromTopLevelFilter() throws Exception { assertHitCount(searchResponse, 3L); for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(2)); - assertThat(hit.getMatchedQueries(), hasItemInArray("name")); - assertThat(hit.getMatchedQueries(), hasItemInArray("title")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(2)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("name")); + assertThat(hit.getMatchedQueryScore("name"), greaterThan(0f)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("title")); + assertThat(hit.getMatchedQueryScore("title"), greaterThan(0f)); } else if (hit.getId().equals("2") || hit.getId().equals("3")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("name")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("name")); + assertThat(hit.getMatchedQueryScore("name"), greaterThan(0f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -174,12 +183,15 @@ public void testSimpleMatchedQueryFromTopLevelFilter() throws Exception { assertHitCount(searchResponse, 3L); for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(2)); - assertThat(hit.getMatchedQueries(), hasItemInArray("name")); - assertThat(hit.getMatchedQueries(), hasItemInArray("title")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(2)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("name")); + assertThat(hit.getMatchedQueryScore("name"), greaterThan(0f)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("title")); + assertThat(hit.getMatchedQueryScore("title"), greaterThan(0f)); } else if (hit.getId().equals("2") || hit.getId().equals("3")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("name")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("name")); + assertThat(hit.getMatchedQueryScore("name"), greaterThan(0f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -203,9 +215,11 @@ public void testSimpleMatchedQueryFromTopLevelFilterAndFilteredQuery() throws Ex assertHitCount(searchResponse, 3L); for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1") || hit.getId().equals("2") || hit.getId().equals("3")) { - assertThat(hit.getMatchedQueries().length, equalTo(2)); - assertThat(hit.getMatchedQueries(), hasItemInArray("name")); - assertThat(hit.getMatchedQueries(), hasItemInArray("title")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(2)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("name")); + assertThat(hit.getMatchedQueryScore("name"), greaterThan(0f)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("title")); + assertThat(hit.getMatchedQueryScore("title"), greaterThan(0f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -242,8 +256,9 @@ public void testRegExpQuerySupportsName() throws InterruptedException { for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("regex")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("regex")); + assertThat(hit.getMatchedQueryScore("regex"), equalTo(1f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -265,8 +280,9 @@ public void testPrefixQuerySupportsName() throws InterruptedException { for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("prefix")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("prefix")); + assertThat(hit.getMatchedQueryScore("prefix"), equalTo(1f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -288,8 +304,9 @@ public void testFuzzyQuerySupportsName() throws InterruptedException { for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("fuzzy")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("fuzzy")); + assertThat(hit.getMatchedQueryScore("fuzzy"), greaterThan(0f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -311,8 +328,9 @@ public void testWildcardQuerySupportsName() throws InterruptedException { for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("wildcard")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("wildcard")); + assertThat(hit.getMatchedQueryScore("wildcard"), equalTo(1f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -334,8 +352,9 @@ public void testSpanFirstQuerySupportsName() throws InterruptedException { for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("span")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("span")); + assertThat(hit.getMatchedQueryScore("span"), greaterThan(0f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -369,11 +388,13 @@ public void testMatchedWithShould() throws Exception { assertHitCount(searchResponse, 2L); for (SearchHit hit : searchResponse.getHits()) { if (hit.getId().equals("1")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("dolor")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("dolor")); + assertThat(hit.getMatchedQueryScore("dolor"), greaterThan(0f)); } else if (hit.getId().equals("2")) { - assertThat(hit.getMatchedQueries().length, equalTo(1)); - assertThat(hit.getMatchedQueries(), hasItemInArray("elit")); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("elit")); + assertThat(hit.getMatchedQueryScore("elit"), greaterThan(0f)); } else { fail("Unexpected document returned with id " + hit.getId()); } @@ -397,7 +418,10 @@ public void testMatchedWithWrapperQuery() throws Exception { for (QueryBuilder query : queries) { SearchResponse searchResponse = client().prepareSearch().setQuery(query).get(); assertHitCount(searchResponse, 1L); - assertThat(searchResponse.getHits().getAt(0).getMatchedQueries()[0], equalTo("abc")); + SearchHit hit = searchResponse.getHits().getAt(0); + assertThat(hit.getMatchedQueriesAndScores().size(), equalTo(1)); + assertThat(hit.getMatchedQueriesAndScores(), hasKey("abc")); + assertThat(hit.getMatchedQueryScore("abc"), greaterThan(0f)); } } } diff --git a/server/src/internalClusterTest/java/org/opensearch/search/functionscore/QueryRescorerIT.java b/server/src/internalClusterTest/java/org/opensearch/search/functionscore/QueryRescorerIT.java index bda6284d9535a..a7faad63e759a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/functionscore/QueryRescorerIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/functionscore/QueryRescorerIT.java @@ -128,7 +128,7 @@ public void testEnforceWindowSize() throws InterruptedException { new QueryRescorerBuilder( functionScoreQuery(matchAllQuery(), ScoreFunctionBuilders.weightFactorFunction(100)).boostMode( CombineFunction.REPLACE - ) + ).queryName("hello world") ).setQueryWeight(0.0f).setRescoreQueryWeight(1.0f), 1 ) diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java index 080366e536da1..9b1bea362ffae 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java @@ -86,10 +86,13 @@ public class RestSearchAction extends BaseRestHandler { */ public static final String TOTAL_HITS_AS_INT_PARAM = "rest_total_hits_as_int"; public static final String TYPED_KEYS_PARAM = "typed_keys"; + public static final String INCLUDE_NAMED_QUERIES_SCORE_PARAM = "include_named_queries_score"; private static final Set RESPONSE_PARAMS; static { - final Set responseParams = new HashSet<>(Arrays.asList(TYPED_KEYS_PARAM, TOTAL_HITS_AS_INT_PARAM)); + final Set responseParams = new HashSet<>( + Arrays.asList(TYPED_KEYS_PARAM, TOTAL_HITS_AS_INT_PARAM, INCLUDE_NAMED_QUERIES_SCORE_PARAM) + ); RESPONSE_PARAMS = Collections.unmodifiableSet(responseParams); } @@ -209,6 +212,7 @@ public static void parseSearchRequest( searchRequest.pipeline(request.param("search_pipeline")); checkRestTotalHits(request, searchRequest); + request.paramAsBoolean(INCLUDE_NAMED_QUERIES_SCORE_PARAM, false); if (searchRequest.pointInTimeBuilder() != null) { preparePointInTime(searchRequest, request, namedWriteableRegistry); diff --git a/server/src/main/java/org/opensearch/search/SearchHit.java b/server/src/main/java/org/opensearch/search/SearchHit.java index 10e65fca3afb5..58b57b24a4bab 100644 --- a/server/src/main/java/org/opensearch/search/SearchHit.java +++ b/server/src/main/java/org/opensearch/search/SearchHit.java @@ -64,16 +64,17 @@ import org.opensearch.index.mapper.MapperService; import org.opensearch.index.mapper.SourceFieldMapper; import org.opensearch.index.seqno.SequenceNumbers; +import org.opensearch.rest.action.search.RestSearchAction; import org.opensearch.search.fetch.subphase.highlight.HighlightField; import org.opensearch.search.lookup.SourceLookup; import org.opensearch.transport.RemoteClusterAware; import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -120,7 +121,7 @@ public final class SearchHit implements Writeable, ToXContentObject, Iterable matchedQueries = Collections.emptyMap(); private Explanation explanation; @@ -203,10 +204,14 @@ public SearchHit(StreamInput in) throws IOException { sortValues = new SearchSortValues(in); size = in.readVInt(); - if (size > 0) { - matchedQueries = new String[size]; + if (in.getVersion().after(Version.V_2_12_0)) { + if (size > 0) { + matchedQueries = in.readOrderedMap(StreamInput::readString, StreamInput::readFloat); + } + } else { + matchedQueries = new LinkedHashMap<>(size); for (int i = 0; i < size; i++) { - matchedQueries[i] = in.readString(); + matchedQueries.put(in.readString(), Float.NaN); } } // we call the setter here because that also sets the local index parameter @@ -224,36 +229,6 @@ public SearchHit(StreamInput in) throws IOException { } } - private Map readFields(StreamInput in) throws IOException { - Map fields; - int size = in.readVInt(); - if (size == 0) { - fields = emptyMap(); - } else if (size == 1) { - DocumentField hitField = new DocumentField(in); - fields = singletonMap(hitField.getName(), hitField); - } else { - fields = new HashMap<>(size); - for (int i = 0; i < size; i++) { - DocumentField field = new DocumentField(in); - fields.put(field.getName(), field); - } - fields = unmodifiableMap(fields); - } - return fields; - } - - private void writeFields(StreamOutput out, Map fields) throws IOException { - if (fields == null) { - out.writeVInt(0); - } else { - out.writeVInt(fields.size()); - for (DocumentField field : fields.values()) { - field.writeTo(out); - } - } - } - private static final Text SINGLE_MAPPING_TYPE = new Text(MapperService.SINGLE_MAPPING_NAME); @Override @@ -286,11 +261,13 @@ public void writeTo(StreamOutput out) throws IOException { } sortValues.writeTo(out); - if (matchedQueries.length == 0) { - out.writeVInt(0); + out.writeVInt(matchedQueries.size()); + if (out.getVersion().after(Version.V_2_12_0)) { + if (!matchedQueries.isEmpty()) { + out.writeMap(matchedQueries, StreamOutput::writeString, StreamOutput::writeFloat); + } } else { - out.writeVInt(matchedQueries.length); - for (String matchedFilter : matchedQueries) { + for (String matchedFilter : matchedQueries.keySet()) { out.writeString(matchedFilter); } } @@ -458,11 +435,11 @@ public DocumentField field(String fieldName) { } /* - * Adds a new DocumentField to the map in case both parameters are not null. - * */ + * Adds a new DocumentField to the map in case both parameters are not null. + * */ public void setDocumentField(String fieldName, DocumentField field) { if (fieldName == null || field == null) return; - if (documentFields.size() == 0) this.documentFields = new HashMap<>(); + if (documentFields.isEmpty()) this.documentFields = new HashMap<>(); this.documentFields.put(fieldName, field); } @@ -475,7 +452,7 @@ public DocumentField removeDocumentField(String fieldName) { * were required to be loaded. */ public Map getFields() { - if (metaFields.size() > 0 || documentFields.size() > 0) { + if (!metaFields.isEmpty() || !documentFields.isEmpty()) { final Map fields = new HashMap<>(); fields.putAll(metaFields); fields.putAll(documentFields); @@ -559,7 +536,7 @@ public String getClusterAlias() { return clusterAlias; } - public void matchedQueries(String[] matchedQueries) { + public void matchedQueries(Map matchedQueries) { this.matchedQueries = matchedQueries; } @@ -567,7 +544,21 @@ public void matchedQueries(String[] matchedQueries) { * The set of query and filter names the query matched with. Mainly makes sense for compound filters and queries. */ public String[] getMatchedQueries() { - return this.matchedQueries; + return matchedQueries == null ? new String[0] : matchedQueries.keySet().toArray(new String[0]); + } + + /** + * @return The score of the provided named query if it matches, {@code null} otherwise. + */ + public Float getMatchedQueryScore(String name) { + return getMatchedQueriesAndScores().get(name); + } + + /** + * @return The map of the named queries that matched and their associated score. + */ + public Map getMatchedQueriesAndScores() { + return matchedQueries == null ? Collections.emptyMap() : matchedQueries; } /** @@ -654,7 +645,7 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t for (DocumentField field : metaFields.values()) { // ignore empty metadata fields - if (field.getValues().size() == 0) { + if (field.getValues().isEmpty()) { continue; } // _ignored is the only multi-valued meta field @@ -670,10 +661,10 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t } if (documentFields.isEmpty() == false && // ignore fields all together if they are all empty - documentFields.values().stream().anyMatch(df -> df.getValues().size() > 0)) { + documentFields.values().stream().anyMatch(df -> !df.getValues().isEmpty())) { builder.startObject(Fields.FIELDS); for (DocumentField field : documentFields.values()) { - if (field.getValues().size() > 0) { + if (!field.getValues().isEmpty()) { field.toXContent(builder, params); } } @@ -687,12 +678,21 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t builder.endObject(); } sortValues.toXContent(builder, params); - if (matchedQueries.length > 0) { - builder.startArray(Fields.MATCHED_QUERIES); - for (String matchedFilter : matchedQueries) { - builder.value(matchedFilter); + if (matchedQueries != null && !matchedQueries.isEmpty()) { + boolean includeMatchedQueriesScore = params.paramAsBoolean(RestSearchAction.INCLUDE_NAMED_QUERIES_SCORE_PARAM, false); + if (includeMatchedQueriesScore) { + builder.startObject(Fields.MATCHED_QUERIES); + for (Map.Entry entry : matchedQueries.entrySet()) { + builder.field(entry.getKey(), entry.getValue()); + } + builder.endObject(); + } else { + builder.startArray(Fields.MATCHED_QUERIES); + for (String matchedFilter : matchedQueries.keySet()) { + builder.value(matchedFilter); + } + builder.endArray(); } - builder.endArray(); } if (getExplanation() != null) { builder.field(Fields._EXPLANATION); @@ -797,7 +797,25 @@ public static void declareInnerHitsParseFields(ObjectParser, (p, c) -> parseInnerHits(p), new ParseField(Fields.INNER_HITS) ); - parser.declareStringArray((map, list) -> map.put(Fields.MATCHED_QUERIES, list), new ParseField(Fields.MATCHED_QUERIES)); + parser.declareField((p, map, context) -> { + XContentParser.Token token = p.currentToken(); + Map matchedQueries = new LinkedHashMap<>(); + if (token == XContentParser.Token.START_OBJECT) { + String fieldName = null; + while ((token = p.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + fieldName = p.currentName(); + } else if (token.isValue()) { + matchedQueries.put(fieldName, p.floatValue()); + } + } + } else if (token == XContentParser.Token.START_ARRAY) { + while (p.nextToken() != XContentParser.Token.END_ARRAY) { + matchedQueries.put(p.text(), Float.NaN); + } + } + map.put(Fields.MATCHED_QUERIES, matchedQueries); + }, new ParseField(Fields.MATCHED_QUERIES), ObjectParser.ValueType.OBJECT_ARRAY); parser.declareField( (map, list) -> map.put(Fields.SORT, list), SearchSortValues::fromXContent, @@ -828,7 +846,7 @@ public static SearchHit createFromMap(Map values) { assert shardId.getIndexName().equals(index); searchHit.shard(new SearchShardTarget(nodeId, shardId, clusterAlias, OriginalIndices.NONE)); } else { - // these fields get set anyways when setting the shard target, + // these fields get set anyway when setting the shard target, // but we set them explicitly when we don't have enough info to rebuild the shard target searchHit.index = index; searchHit.clusterAlias = clusterAlias; @@ -842,10 +860,7 @@ public static SearchHit createFromMap(Map values) { searchHit.sourceRef(get(SourceFieldMapper.NAME, values, null)); searchHit.explanation(get(Fields._EXPLANATION, values, null)); searchHit.setInnerHits(get(Fields.INNER_HITS, values, null)); - List matchedQueries = get(Fields.MATCHED_QUERIES, values, null); - if (matchedQueries != null) { - searchHit.matchedQueries(matchedQueries.toArray(new String[0])); - } + searchHit.matchedQueries(get(Fields.MATCHED_QUERIES, values, null)); return searchHit; } @@ -965,7 +980,7 @@ public boolean equals(Object obj) { && Objects.equals(documentFields, other.documentFields) && Objects.equals(metaFields, other.metaFields) && Objects.equals(getHighlightFields(), other.getHighlightFields()) - && Arrays.equals(matchedQueries, other.matchedQueries) + && Objects.equals(getMatchedQueriesAndScores(), other.getMatchedQueriesAndScores()) && Objects.equals(explanation, other.explanation) && Objects.equals(shard, other.shard) && Objects.equals(innerHits, other.innerHits) @@ -985,7 +1000,7 @@ public int hashCode() { documentFields, metaFields, getHighlightFields(), - Arrays.hashCode(matchedQueries), + getMatchedQueriesAndScores(), explanation, shard, innerHits, diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java index 6c589438d6b4c..6734bc11fd5c0 100644 --- a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java @@ -34,18 +34,16 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.search.Query; import org.apache.lucene.search.ScoreMode; +import org.apache.lucene.search.Scorer; import org.apache.lucene.search.ScorerSupplier; import org.apache.lucene.search.Weight; -import org.apache.lucene.util.Bits; -import org.opensearch.common.lucene.Lucene; import org.opensearch.search.fetch.FetchContext; import org.opensearch.search.fetch.FetchSubPhase; import org.opensearch.search.fetch.FetchSubPhaseProcessor; import java.io.IOException; -import java.util.ArrayList; import java.util.HashMap; -import java.util.List; +import java.util.LinkedHashMap; import java.util.Map; /** @@ -71,12 +69,12 @@ public FetchSubPhaseProcessor getProcessor(FetchContext context) throws IOExcept for (Map.Entry entry : namedQueries.entrySet()) { weights.put( entry.getKey(), - context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), ScoreMode.COMPLETE_NO_SCORES, 1) + context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), ScoreMode.COMPLETE, 1) ); } return new FetchSubPhaseProcessor() { - final Map matchingIterators = new HashMap<>(); + final Map matchingIterators = new HashMap<>(); @Override public void setNextReader(LeafReaderContext readerContext) throws IOException { @@ -84,22 +82,28 @@ public void setNextReader(LeafReaderContext readerContext) throws IOException { for (Map.Entry entry : weights.entrySet()) { ScorerSupplier ss = entry.getValue().scorerSupplier(readerContext); if (ss != null) { - Bits matchingBits = Lucene.asSequentialAccessBits(readerContext.reader().maxDoc(), ss); - matchingIterators.put(entry.getKey(), matchingBits); + Scorer scorer = ss.get(0L); + if (scorer != null) { + matchingIterators.put(entry.getKey(), scorer); + } } } } @Override - public void process(HitContext hitContext) { - List matches = new ArrayList<>(); + public void process(HitContext hitContext) throws IOException { + Map matches = new LinkedHashMap<>(); int doc = hitContext.docId(); - for (Map.Entry iterator : matchingIterators.entrySet()) { - if (iterator.getValue().get(doc)) { - matches.add(iterator.getKey()); + for (Map.Entry entry : matchingIterators.entrySet()) { + Scorer scorer = entry.getValue(); + if (scorer.iterator().docID() < doc) { + scorer.iterator().advance(doc); + } + if (scorer.iterator().docID() == doc) { + matches.put(entry.getKey(), scorer.score()); } } - hitContext.hit().matchedQueries(matches.toArray(new String[0])); + hitContext.hit().matchedQueries(matches); } }; } diff --git a/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java b/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java index 370c691daf401..1a5118ac57d9d 100644 --- a/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java +++ b/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java @@ -536,6 +536,40 @@ public void testWriteMap() throws IOException { assertThat(expected, equalTo(loaded)); } + public void testReadOrderedMapEmpty() throws IOException { + BytesStreamOutput out = new BytesStreamOutput(); + out.writeVInt(0); + + StreamInput in = StreamInput.wrap(BytesReference.toBytes(out.bytes())); + Map result = in.readOrderedMap(StreamInput::readInt, StreamInput::readString); + assertTrue("Map should be empty", result.isEmpty()); + + // If the map is empty, it's not necessarily a LinkedHashMap due to optimization + assertTrue("Resulting map should be immutable", Collections.emptyMap().getClass().isInstance(result)); + + in.close(); + out.close(); + } + + public void testReadOrderedMapNonEmpty() throws IOException { + BytesStreamOutput out = new BytesStreamOutput(); + out.writeVInt(2); + out.writeInt(1); + out.writeString("One"); + out.writeInt(2); + out.writeString("Two"); + + StreamInput in = StreamInput.wrap(BytesReference.toBytes(out.bytes())); + Map result = in.readOrderedMap(StreamInput::readInt, StreamInput::readString); + assertEquals("Map should have 2 elements", 2, result.size()); + assertTrue("Resulting map should be a LinkedHashMap", result instanceof LinkedHashMap); + assertEquals("First element should be 'One'", "One", result.get(1)); + assertEquals("Second element should be 'Two'", "Two", result.get(2)); + + in.close(); + out.close(); + } + public void testWriteMapOfLists() throws IOException { final int size = randomIntBetween(0, 5); final Map> expected = new HashMap<>(size); diff --git a/server/src/test/java/org/opensearch/search/SearchHitTests.java b/server/src/test/java/org/opensearch/search/SearchHitTests.java index 88d5fb38a6cb1..8b407152e5c63 100644 --- a/server/src/test/java/org/opensearch/search/SearchHitTests.java +++ b/server/src/test/java/org/opensearch/search/SearchHitTests.java @@ -61,6 +61,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.function.Predicate; @@ -76,6 +77,25 @@ import static org.hamcrest.Matchers.nullValue; public class SearchHitTests extends AbstractWireSerializingTestCase { + + private Map getSampleMatchedQueries() { + Map matchedQueries = new LinkedHashMap<>(); + matchedQueries.put("query1", 1.0f); + matchedQueries.put("query2", 0.5f); + return matchedQueries; + } + + public static SearchHit createTestItemWithMatchedQueriesScores(boolean withOptionalInnerHits, boolean withShardTarget) { + var searchHit = createTestItem(randomFrom(XContentType.values()), withOptionalInnerHits, withShardTarget); + int size = randomIntBetween(1, 5); // Ensure at least one matched query + Map matchedQueries = new LinkedHashMap<>(size); + for (int i = 0; i < size; i++) { + matchedQueries.put(randomAlphaOfLength(5), randomFloat()); + } + searchHit.matchedQueries(matchedQueries); + return searchHit; + } + public static SearchHit createTestItem(boolean withOptionalInnerHits, boolean withShardTarget) { return createTestItem(randomFrom(XContentType.values()), withOptionalInnerHits, withShardTarget); } @@ -129,9 +149,9 @@ public static SearchHit createTestItem(final MediaType mediaType, boolean withOp } if (randomBoolean()) { int size = randomIntBetween(0, 5); - String[] matchedQueries = new String[size]; + Map matchedQueries = new LinkedHashMap<>(size); for (int i = 0; i < size; i++) { - matchedQueries[i] = randomAlphaOfLength(5); + matchedQueries.put(randomAlphaOfLength(5), Float.NaN); } hit.matchedQueries(matchedQueries); } @@ -219,6 +239,13 @@ public void testFromXContentLenientParsing() throws IOException { assertToXContentEquivalent(originalBytes, toXContent(parsed, xContentType, true), xContentType); } + public void testSerializationDeserializationWithMatchedQueriesScores() throws IOException { + SearchHit searchHit = createTestItemWithMatchedQueriesScores(true, true); + SearchHit deserializedSearchHit = copyWriteable(searchHit, getNamedWriteableRegistry(), SearchHit::new, Version.V_3_0_0); + assertEquals(searchHit, deserializedSearchHit); + assertEquals(searchHit.getMatchedQueriesAndScores(), deserializedSearchHit.getMatchedQueriesAndScores()); + } + /** * When e.g. with "stored_fields": "_none_", only "_index" and "_score" are returned. */ @@ -244,6 +271,125 @@ public void testToXContent() throws IOException { assertEquals("{\"_id\":\"id1\",\"_score\":1.5}", builder.toString()); } + public void testSerializeShardTargetWithNewVersion() throws Exception { + String clusterAlias = randomBoolean() ? null : "cluster_alias"; + SearchShardTarget target = new SearchShardTarget( + "_node_id", + new ShardId(new Index("_index", "_na_"), 0), + clusterAlias, + OriginalIndices.NONE + ); + + Map innerHits = new HashMap<>(); + SearchHit innerHit1 = new SearchHit(0, "_id", null, null); + innerHit1.shard(target); + SearchHit innerInnerHit2 = new SearchHit(0, "_id", null, null); + innerInnerHit2.shard(target); + innerHits.put("1", new SearchHits(new SearchHit[] { innerInnerHit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1f)); + innerHit1.setInnerHits(innerHits); + SearchHit innerHit2 = new SearchHit(0, "_id", null, null); + innerHit2.shard(target); + SearchHit innerHit3 = new SearchHit(0, "_id", null, null); + innerHit3.shard(target); + + innerHits = new HashMap<>(); + SearchHit hit1 = new SearchHit(0, "_id", null, null); + innerHits.put("1", new SearchHits(new SearchHit[] { innerHit1, innerHit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1f)); + innerHits.put("2", new SearchHits(new SearchHit[] { innerHit3 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1f)); + hit1.shard(target); + hit1.setInnerHits(innerHits); + + SearchHit hit2 = new SearchHit(0, "_id", null, null); + hit2.shard(target); + + SearchHits hits = new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(2, TotalHits.Relation.EQUAL_TO), 1f); + + SearchHits results = copyWriteable(hits, getNamedWriteableRegistry(), SearchHits::new, Version.V_3_0_0); + SearchShardTarget deserializedTarget = results.getAt(0).getShard(); + assertThat(deserializedTarget, equalTo(target)); + assertThat(results.getAt(0).getInnerHits().get("1").getAt(0).getShard(), notNullValue()); + assertThat(results.getAt(0).getInnerHits().get("1").getAt(0).getInnerHits().get("1").getAt(0).getShard(), notNullValue()); + assertThat(results.getAt(0).getInnerHits().get("1").getAt(1).getShard(), notNullValue()); + assertThat(results.getAt(0).getInnerHits().get("2").getAt(0).getShard(), notNullValue()); + for (SearchHit hit : results) { + assertEquals(clusterAlias, hit.getClusterAlias()); + if (hit.getInnerHits() != null) { + for (SearchHits innerhits : hit.getInnerHits().values()) { + for (SearchHit innerHit : innerhits) { + assertEquals(clusterAlias, innerHit.getClusterAlias()); + } + } + } + } + assertThat(results.getAt(1).getShard(), equalTo(target)); + } + + public void testSerializeShardTargetWithNewVersionAndMatchedQueries() throws Exception { + String clusterAlias = randomBoolean() ? null : "cluster_alias"; + SearchShardTarget target = new SearchShardTarget( + "_node_id", + new ShardId(new Index("_index", "_na_"), 0), + clusterAlias, + OriginalIndices.NONE + ); + + Map innerHits = new HashMap<>(); + SearchHit innerHit1 = new SearchHit(0, "_id", null, null); + innerHit1.shard(target); + innerHit1.matchedQueries(getSampleMatchedQueries()); + SearchHit innerInnerHit2 = new SearchHit(0, "_id", null, null); + innerInnerHit2.shard(target); + innerHits.put("1", new SearchHits(new SearchHit[] { innerInnerHit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1f)); + innerHit1.setInnerHits(innerHits); + SearchHit innerHit2 = new SearchHit(0, "_id", null, null); + innerHit2.shard(target); + innerHit2.matchedQueries(getSampleMatchedQueries()); + SearchHit innerHit3 = new SearchHit(0, "_id", null, null); + innerHit3.shard(target); + innerHit3.matchedQueries(getSampleMatchedQueries()); + + innerHits = new HashMap<>(); + SearchHit hit1 = new SearchHit(0, "_id", null, null); + innerHits.put("1", new SearchHits(new SearchHit[] { innerHit1, innerHit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1f)); + innerHits.put("2", new SearchHits(new SearchHit[] { innerHit3 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1f)); + hit1.shard(target); + hit1.setInnerHits(innerHits); + + SearchHit hit2 = new SearchHit(0, "_id", null, null); + hit2.shard(target); + + SearchHits hits = new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(2, TotalHits.Relation.EQUAL_TO), 1f); + + SearchHits results = copyWriteable(hits, getNamedWriteableRegistry(), SearchHits::new, Version.V_3_0_0); + SearchShardTarget deserializedTarget = results.getAt(0).getShard(); + assertThat(deserializedTarget, equalTo(target)); + assertThat(results.getAt(0).getInnerHits().get("1").getAt(0).getShard(), notNullValue()); + assertThat(results.getAt(0).getInnerHits().get("1").getAt(0).getInnerHits().get("1").getAt(0).getShard(), notNullValue()); + assertThat(results.getAt(0).getInnerHits().get("1").getAt(1).getShard(), notNullValue()); + assertThat(results.getAt(0).getInnerHits().get("2").getAt(0).getShard(), notNullValue()); + String[] expectedMatchedQueries = new String[] { "query1", "query2" }; + String[] actualMatchedQueries = results.getAt(0).getInnerHits().get("1").getAt(0).getMatchedQueries(); + assertArrayEquals(expectedMatchedQueries, actualMatchedQueries); + + Map expectedMatchedQueriesAndScores = new LinkedHashMap<>(); + expectedMatchedQueriesAndScores.put("query1", 1.0f); + expectedMatchedQueriesAndScores.put("query2", 0.5f); + + Map actualMatchedQueriesAndScores = results.getAt(0).getInnerHits().get("1").getAt(0).getMatchedQueriesAndScores(); + assertEquals(expectedMatchedQueriesAndScores, actualMatchedQueriesAndScores); + for (SearchHit hit : results) { + assertEquals(clusterAlias, hit.getClusterAlias()); + if (hit.getInnerHits() != null) { + for (SearchHits innerhits : hit.getInnerHits().values()) { + for (SearchHit innerHit : innerhits) { + assertEquals(clusterAlias, innerHit.getClusterAlias()); + } + } + } + } + assertThat(results.getAt(1).getShard(), equalTo(target)); + } + public void testSerializeShardTarget() throws Exception { String clusterAlias = randomBoolean() ? null : "cluster_alias"; SearchShardTarget target = new SearchShardTarget( From 3aef42e4cb0bdebebf725399c96e9cf42d542ae4 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sun, 17 Dec 2023 20:05:26 +0100 Subject: [PATCH 02/23] fix changelog with correct PR reference Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e10e795fbec70..387324915c198 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -121,7 +121,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Adding slf4j license header to LoggerMessageFormat.java ([#11069](https://github.com/opensearch-project/OpenSearch/pull/11069)) - [BWC and API enforcement] Introduce checks for enforcing the API restrictions ([#11175](https://github.com/opensearch-project/OpenSearch/pull/11175)) - Create separate transport action for render search template action ([#11170](https://github.com/opensearch-project/OpenSearch/pull/11170)) -- Backwards compatible support for returning scores in matched queries ((https://github.com/opensearch-project/OpenSearch/pull/11549)) +- Backwards compatible support for returning scores in matched queries ((https://github.com/opensearch-project/OpenSearch/pull/11626)) ### Dependencies - Bump Lucene from 9.7.0 to 9.8.0 ([10276](https://github.com/opensearch-project/OpenSearch/pull/10276)) From 5f7740ea22e1647a9ca5cb34d9b44899ff5e66ce Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sun, 17 Dec 2023 21:27:00 +0100 Subject: [PATCH 03/23] fix spotless check in opensearch core Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../java/org/opensearch/core/common/io/stream/StreamInput.java | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 387324915c198..1ff81be2b07b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -121,7 +121,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Adding slf4j license header to LoggerMessageFormat.java ([#11069](https://github.com/opensearch-project/OpenSearch/pull/11069)) - [BWC and API enforcement] Introduce checks for enforcing the API restrictions ([#11175](https://github.com/opensearch-project/OpenSearch/pull/11175)) - Create separate transport action for render search template action ([#11170](https://github.com/opensearch-project/OpenSearch/pull/11170)) -- Backwards compatible support for returning scores in matched queries ((https://github.com/opensearch-project/OpenSearch/pull/11626)) +- Backwards compatible support for returning scores in matched queries ([#11626](https://github.com/opensearch-project/OpenSearch/pull/11626)) ### Dependencies - Bump Lucene from 9.7.0 to 9.8.0 ([10276](https://github.com/opensearch-project/OpenSearch/pull/10276)) diff --git a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java index ac4e0f50b924c..c604b0cb1d1dc 100644 --- a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java +++ b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java @@ -632,7 +632,6 @@ public String[] readOptionalStringArray() throws IOException { return null; } - /** * Reads an ordered {@link Map} from a data source using provided key and value readers. * @@ -689,7 +688,7 @@ static int capacity(int expectedSize) { * @throws IOException If an I/O error occurs during reading from the data source. */ private Map readMap(Writeable.Reader keyReader, Writeable.Reader valueReader, IntFunction> constructor) - throws IOException { + throws IOException { int size = readArraySize(); if (size == 0) { return Collections.emptyMap(); From d21e0401da843b119507b55e1245c607b43eaa7f Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Thu, 4 Jan 2024 14:43:32 +0100 Subject: [PATCH 04/23] minor refactor Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../recovery/IndexPrimaryRelocationIT.java | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java index c049c8ed2d4a6..9decd17d95eab 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java @@ -66,19 +66,16 @@ public void testPrimaryRelocationWhileIndexing() throws Exception { ensureGreen("test"); AtomicInteger numAutoGenDocs = new AtomicInteger(); final AtomicBoolean finished = new AtomicBoolean(false); - Thread indexingThread = new Thread() { - @Override - public void run() { - while (finished.get() == false && numAutoGenDocs.get() < 10_000) { - IndexResponse indexResponse = client().prepareIndex("test").setId("id").setSource("field", "value").get(); - assertEquals(DocWriteResponse.Result.CREATED, indexResponse.getResult()); - DeleteResponse deleteResponse = client().prepareDelete("test", "id").get(); - assertEquals(DocWriteResponse.Result.DELETED, deleteResponse.getResult()); - client().prepareIndex("test").setSource("auto", true).get(); - numAutoGenDocs.incrementAndGet(); - } + Thread indexingThread = new Thread(() -> { + while (finished.get() == false && numAutoGenDocs.get() < 10_000) { + IndexResponse indexResponse = client().prepareIndex("test").setId("id").setSource("field", "value").get(); + assertEquals(DocWriteResponse.Result.CREATED, indexResponse.getResult()); + DeleteResponse deleteResponse = client().prepareDelete("test", "id").get(); + assertEquals(DocWriteResponse.Result.DELETED, deleteResponse.getResult()); + client().prepareIndex("test").setSource("auto", true).get(); + numAutoGenDocs.incrementAndGet(); } - }; + }); indexingThread.start(); ClusterState initialState = client().admin().cluster().prepareState().get().getState(); From 86fb57ff74df4e09ed02fc90ea7574f15c9293b1 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Wed, 17 Jan 2024 21:44:03 +0100 Subject: [PATCH 05/23] fix version check Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- server/src/main/java/org/opensearch/search/SearchHit.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/SearchHit.java b/server/src/main/java/org/opensearch/search/SearchHit.java index 58b57b24a4bab..f12b1418bf24e 100644 --- a/server/src/main/java/org/opensearch/search/SearchHit.java +++ b/server/src/main/java/org/opensearch/search/SearchHit.java @@ -204,7 +204,7 @@ public SearchHit(StreamInput in) throws IOException { sortValues = new SearchSortValues(in); size = in.readVInt(); - if (in.getVersion().after(Version.V_2_12_0)) { + if (in.getVersion().onOrAfter(Version.V_2_12_0)) { if (size > 0) { matchedQueries = in.readOrderedMap(StreamInput::readString, StreamInput::readFloat); } @@ -262,7 +262,7 @@ public void writeTo(StreamOutput out) throws IOException { sortValues.writeTo(out); out.writeVInt(matchedQueries.size()); - if (out.getVersion().after(Version.V_2_12_0)) { + if (out.getVersion().onOrAfter(Version.V_2_12_0)) { if (!matchedQueries.isEmpty()) { out.writeMap(matchedQueries, StreamOutput::writeString, StreamOutput::writeFloat); } From 013cdd90213b61dfb84acfdfef6acaeae475711a Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sat, 27 Jan 2024 23:11:14 +0100 Subject: [PATCH 06/23] address review comments Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../core/common/io/stream/StreamInput.java | 28 --------------- .../search/functionscore/QueryRescorerIT.java | 10 ++++-- .../java/org/opensearch/search/SearchHit.java | 33 ++++++++++++++---- .../fetch/subphase/MatchedQueriesPhase.java | 2 +- .../common/io/stream/BytesStreamsTests.java | 34 ------------------- .../org/opensearch/search/SearchHitTests.java | 10 +++--- .../test/hamcrest/OpenSearchAssertions.java | 5 +++ .../test/hamcrest/OpenSearchMatchers.java | 32 +++++++++++++++++ 8 files changed, 76 insertions(+), 78 deletions(-) diff --git a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java index c604b0cb1d1dc..60442581601b2 100644 --- a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java +++ b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java @@ -632,34 +632,6 @@ public String[] readOptionalStringArray() throws IOException { return null; } - /** - * Reads an ordered {@link Map} from a data source using provided key and value readers. - * - * This method creates a `LinkedHashMap`, ensuring that the iteration order of the map - * matches the order in which the entries were read from the data source. It uses the - * `keyReader` and `valueReader` to read each key-value pair and constructs the map - * with an initial capacity optimized based on the expected size. - * - * Usage example: - * 

-     * Map<Integer, String> orderedMap = readOrderedMap(StreamInput::readInteger, StreamInput::readString);
-     *
- * - * @param keyReader The {@link Writeable.Reader} used to read the keys of the map. - * @param valueReader The {@link Writeable.Reader} used to read the values of the map. - * @return A {@link LinkedHashMap} containing the keys and values read from the data source. - * The map maintains the order in which keys and values were read. - * @throws IOException If an I/O error occurs during reading from the data source. - */ - public Map readOrderedMap(Writeable.Reader keyReader, Writeable.Reader valueReader) throws IOException { - return readMap(keyReader, valueReader, (expectedSize) -> new LinkedHashMap<>(capacity(expectedSize))); - } - - static int capacity(int expectedSize) { - assert expectedSize >= 0; - return expectedSize < 2 ? expectedSize + 1 : (int) (expectedSize / 0.75 + 1.0); - } - /** * Reads a {@link Map} from a data source using provided key and value readers and a map constructor. * diff --git a/server/src/internalClusterTest/java/org/opensearch/search/functionscore/QueryRescorerIT.java b/server/src/internalClusterTest/java/org/opensearch/search/functionscore/QueryRescorerIT.java index a7faad63e759a..309b49a9f551d 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/functionscore/QueryRescorerIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/functionscore/QueryRescorerIT.java @@ -84,6 +84,7 @@ import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSecondHit; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertThirdHit; import static org.opensearch.test.hamcrest.OpenSearchAssertions.hasId; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.hasMatchedQueries; import static org.opensearch.test.hamcrest.OpenSearchAssertions.hasScore; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -128,7 +129,7 @@ public void testEnforceWindowSize() throws InterruptedException { new QueryRescorerBuilder( functionScoreQuery(matchAllQuery(), ScoreFunctionBuilders.weightFactorFunction(100)).boostMode( CombineFunction.REPLACE - ).queryName("hello world") + ) ).setQueryWeight(0.0f).setRescoreQueryWeight(1.0f), 1 ) @@ -600,7 +601,7 @@ public void testExplain() throws Exception { SearchResponse searchResponse = client().prepareSearch() .setSearchType(SearchType.DFS_QUERY_THEN_FETCH) - .setQuery(QueryBuilders.matchQuery("field1", "the quick brown").operator(Operator.OR)) + .setQuery(QueryBuilders.matchQuery("field1", "the quick brown").operator(Operator.OR).queryName("hello-world")) .setRescorer(innerRescoreQuery, 5) .setExplain(true) .get(); @@ -608,7 +609,10 @@ public void testExplain() throws Exception { assertFirstHit(searchResponse, hasId("1")); assertSecondHit(searchResponse, hasId("2")); assertThirdHit(searchResponse, hasId("3")); - + final String[] matchedQueries = { "hello-world" }; + assertFirstHit(searchResponse, hasMatchedQueries(matchedQueries)); + assertSecondHit(searchResponse, hasMatchedQueries(matchedQueries)); + assertThirdHit(searchResponse, hasMatchedQueries(matchedQueries)); for (int j = 0; j < 3; j++) { assertThat(searchResponse.getHits().getAt(j).getExplanation().getDescription(), equalTo(descriptionModes[innerMode])); } diff --git a/server/src/main/java/org/opensearch/search/SearchHit.java b/server/src/main/java/org/opensearch/search/SearchHit.java index f12b1418bf24e..733bd76fb3f62 100644 --- a/server/src/main/java/org/opensearch/search/SearchHit.java +++ b/server/src/main/java/org/opensearch/search/SearchHit.java @@ -78,6 +78,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.stream.Collectors; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; @@ -204,9 +205,15 @@ public SearchHit(StreamInput in) throws IOException { sortValues = new SearchSortValues(in); size = in.readVInt(); - if (in.getVersion().onOrAfter(Version.V_2_12_0)) { + if (in.getVersion().onOrAfter(Version.V_3_0_0)) { if (size > 0) { - matchedQueries = in.readOrderedMap(StreamInput::readString, StreamInput::readFloat); + Map tempMap = in.readMap(StreamInput::readString, StreamInput::readFloat); + matchedQueries = tempMap.entrySet() + .stream() + .sorted(Map.Entry.comparingByKey()) + .collect( + Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (oldValue, newValue) -> oldValue, LinkedHashMap::new) + ); } } else { matchedQueries = new LinkedHashMap<>(size); @@ -262,7 +269,7 @@ public void writeTo(StreamOutput out) throws IOException { sortValues.writeTo(out); out.writeVInt(matchedQueries.size()); - if (out.getVersion().onOrAfter(Version.V_2_12_0)) { + if (out.getVersion().onOrAfter(Version.V_3_0_0)) { if (!matchedQueries.isEmpty()) { out.writeMap(matchedQueries, StreamOutput::writeString, StreamOutput::writeFloat); } @@ -536,8 +543,18 @@ public String getClusterAlias() { return clusterAlias; } - public void matchedQueries(Map matchedQueries) { - this.matchedQueries = matchedQueries; + public void matchedQueries(String[] matchedQueries) { + if (matchedQueries != null) { + for (String query : matchedQueries) { + this.matchedQueries.put(query, Float.NaN); + } + } + } + + public void matchedQueriesWithScores(Map matchedQueries) { + if (matchedQueries != null) { + this.matchedQueries = matchedQueries; + } } /** @@ -678,7 +695,7 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t builder.endObject(); } sortValues.toXContent(builder, params); - if (matchedQueries != null && !matchedQueries.isEmpty()) { + if (!matchedQueries.isEmpty()) { boolean includeMatchedQueriesScore = params.paramAsBoolean(RestSearchAction.INCLUDE_NAMED_QUERIES_SCORE_PARAM, false); if (includeMatchedQueriesScore) { builder.startObject(Fields.MATCHED_QUERIES); @@ -813,6 +830,8 @@ public static void declareInnerHitsParseFields(ObjectParser, while (p.nextToken() != XContentParser.Token.END_ARRAY) { matchedQueries.put(p.text(), Float.NaN); } + } else { + throw new IllegalStateException("expected object or array but got [" + token + "]"); } map.put(Fields.MATCHED_QUERIES, matchedQueries); }, new ParseField(Fields.MATCHED_QUERIES), ObjectParser.ValueType.OBJECT_ARRAY); @@ -860,7 +879,7 @@ public static SearchHit createFromMap(Map values) { searchHit.sourceRef(get(SourceFieldMapper.NAME, values, null)); searchHit.explanation(get(Fields._EXPLANATION, values, null)); searchHit.setInnerHits(get(Fields.INNER_HITS, values, null)); - searchHit.matchedQueries(get(Fields.MATCHED_QUERIES, values, null)); + searchHit.matchedQueriesWithScores(get(Fields.MATCHED_QUERIES, values, null)); return searchHit; } diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java index 6734bc11fd5c0..8e151dac2a7da 100644 --- a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java @@ -103,7 +103,7 @@ public void process(HitContext hitContext) throws IOException { matches.put(entry.getKey(), scorer.score()); } } - hitContext.hit().matchedQueries(matches); + hitContext.hit().matchedQueriesWithScores(matches); } }; } diff --git a/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java b/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java index 1a5118ac57d9d..370c691daf401 100644 --- a/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java +++ b/server/src/test/java/org/opensearch/common/io/stream/BytesStreamsTests.java @@ -536,40 +536,6 @@ public void testWriteMap() throws IOException { assertThat(expected, equalTo(loaded)); } - public void testReadOrderedMapEmpty() throws IOException { - BytesStreamOutput out = new BytesStreamOutput(); - out.writeVInt(0); - - StreamInput in = StreamInput.wrap(BytesReference.toBytes(out.bytes())); - Map result = in.readOrderedMap(StreamInput::readInt, StreamInput::readString); - assertTrue("Map should be empty", result.isEmpty()); - - // If the map is empty, it's not necessarily a LinkedHashMap due to optimization - assertTrue("Resulting map should be immutable", Collections.emptyMap().getClass().isInstance(result)); - - in.close(); - out.close(); - } - - public void testReadOrderedMapNonEmpty() throws IOException { - BytesStreamOutput out = new BytesStreamOutput(); - out.writeVInt(2); - out.writeInt(1); - out.writeString("One"); - out.writeInt(2); - out.writeString("Two"); - - StreamInput in = StreamInput.wrap(BytesReference.toBytes(out.bytes())); - Map result = in.readOrderedMap(StreamInput::readInt, StreamInput::readString); - assertEquals("Map should have 2 elements", 2, result.size()); - assertTrue("Resulting map should be a LinkedHashMap", result instanceof LinkedHashMap); - assertEquals("First element should be 'One'", "One", result.get(1)); - assertEquals("Second element should be 'Two'", "Two", result.get(2)); - - in.close(); - out.close(); - } - public void testWriteMapOfLists() throws IOException { final int size = randomIntBetween(0, 5); final Map> expected = new HashMap<>(size); diff --git a/server/src/test/java/org/opensearch/search/SearchHitTests.java b/server/src/test/java/org/opensearch/search/SearchHitTests.java index 8b407152e5c63..9e1aaaff6c514 100644 --- a/server/src/test/java/org/opensearch/search/SearchHitTests.java +++ b/server/src/test/java/org/opensearch/search/SearchHitTests.java @@ -92,7 +92,7 @@ public static SearchHit createTestItemWithMatchedQueriesScores(boolean withOptio for (int i = 0; i < size; i++) { matchedQueries.put(randomAlphaOfLength(5), randomFloat()); } - searchHit.matchedQueries(matchedQueries); + searchHit.matchedQueriesWithScores(matchedQueries); return searchHit; } @@ -153,7 +153,7 @@ public static SearchHit createTestItem(final MediaType mediaType, boolean withOp for (int i = 0; i < size; i++) { matchedQueries.put(randomAlphaOfLength(5), Float.NaN); } - hit.matchedQueries(matchedQueries); + hit.matchedQueriesWithScores(matchedQueries); } if (randomBoolean()) { hit.explanation(createExplanation(randomIntBetween(0, 5))); @@ -336,17 +336,17 @@ public void testSerializeShardTargetWithNewVersionAndMatchedQueries() throws Exc Map innerHits = new HashMap<>(); SearchHit innerHit1 = new SearchHit(0, "_id", null, null); innerHit1.shard(target); - innerHit1.matchedQueries(getSampleMatchedQueries()); + innerHit1.matchedQueriesWithScores(getSampleMatchedQueries()); SearchHit innerInnerHit2 = new SearchHit(0, "_id", null, null); innerInnerHit2.shard(target); innerHits.put("1", new SearchHits(new SearchHit[] { innerInnerHit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1f)); innerHit1.setInnerHits(innerHits); SearchHit innerHit2 = new SearchHit(0, "_id", null, null); innerHit2.shard(target); - innerHit2.matchedQueries(getSampleMatchedQueries()); + innerHit2.matchedQueriesWithScores(getSampleMatchedQueries()); SearchHit innerHit3 = new SearchHit(0, "_id", null, null); innerHit3.shard(target); - innerHit3.matchedQueries(getSampleMatchedQueries()); + innerHit3.matchedQueriesWithScores(getSampleMatchedQueries()); innerHits = new HashMap<>(); SearchHit hit1 = new SearchHit(0, "_id", null, null); diff --git a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java index 183214c159c14..ee5570301c428 100644 --- a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java +++ b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java @@ -528,6 +528,11 @@ public static Matcher hasScore(final float score) { return new OpenSearchMatchers.SearchHitHasScoreMatcher(score); } + public static Matcher hasMatchedQueries(final String[] matchedQueries) { + return new OpenSearchMatchers.SearchHitMatchedQueriesMatcher(matchedQueries); + } + + public static CombinableMatcher hasProperty(Function property, Matcher valueMatcher) { return OpenSearchMatchers.HasPropertyLambdaMatcher.hasProperty(property, valueMatcher); } diff --git a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchMatchers.java b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchMatchers.java index 5889b7e269ed2..5f661d7145ea5 100644 --- a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchMatchers.java +++ b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchMatchers.java @@ -31,6 +31,7 @@ package org.opensearch.test.hamcrest; +import java.util.Arrays; import org.opensearch.search.SearchHit; import org.hamcrest.Description; import org.hamcrest.FeatureMatcher; @@ -111,6 +112,37 @@ public void describeTo(final Description description) { } } + public static class SearchHitMatchedQueriesMatcher extends TypeSafeMatcher { + private String[] matchedQueries; + + public SearchHitMatchedQueriesMatcher(String[] matchedQueries) { + this.matchedQueries = matchedQueries; + } + + @Override + protected boolean matchesSafely(SearchHit searchHit) { + String[] searchHitQueries = searchHit.getMatchedQueries(); + if (matchedQueries == null) { + return false; + } + Arrays.sort(searchHitQueries); + Arrays.sort(matchedQueries); + return Arrays.equals(searchHitQueries, matchedQueries); + } + + @Override + public void describeMismatchSafely(final SearchHit searchHit, final Description mismatchDescription) { + mismatchDescription.appendText(" matched queries were ") + .appendValue(Arrays.toString(searchHit.getMatchedQueries())); + } + + @Override + public void describeTo(final Description description) { + description.appendText("searchHit matched queries should be ") + .appendValue(Arrays.toString(matchedQueries)); + } + } + public static class HasPropertyLambdaMatcher extends FeatureMatcher { private final Function property; From 9938f111ce98d14eb6955fde06f703fa5847b100 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sat, 27 Jan 2024 23:14:36 +0100 Subject: [PATCH 07/23] remove unused method Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../core/common/io/stream/StreamInput.java | 42 ------------------- 1 file changed, 42 deletions(-) diff --git a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java index 60442581601b2..3e996bdee83a2 100644 --- a/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java +++ b/libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java @@ -632,48 +632,6 @@ public String[] readOptionalStringArray() throws IOException { return null; } - /** - * Reads a {@link Map} from a data source using provided key and value readers and a map constructor. - * - * This method is a flexible utility for reading a map from a data source, such as a file or network stream. - * It uses the provided `keyReader` and `valueReader` to read each key-value pair. The size of the map is - * determined first by reading the array size. A map constructor is also provided to create a map of the - * appropriate size, allowing for optimized performance based on the expected number of entries. - * - * If the read size is zero, an empty map is returned. Otherwise, the method iterates over the size, - * reading keys and values and adding them to the map. - * - * Usage example: - * 

-     * Map<Integer, String> map = readMap(StreamInput::readInteger, StreamInput::readString, HashMap::new);
-     *
- * - * Note: If the map is empty, an immutable empty map is returned. Otherwise, a mutable map is created. - * - * @param keyReader The {@link Writeable.Reader} used to read the keys of the map. - * @param valueReader The {@link Writeable.Reader} used to read the values of the map. - * @param constructor A function that takes an integer (the initial size) and returns a new map. - * This allows for the creation of a map with an initial capacity matching the - * expected size, optimizing performance. - * @return A {@link Map} containing the keys and values read from the data source. This map is either - * empty (and immutable) or mutable and filled with the read data. - * @throws IOException If an I/O error occurs during reading from the data source. - */ - private Map readMap(Writeable.Reader keyReader, Writeable.Reader valueReader, IntFunction> constructor) - throws IOException { - int size = readArraySize(); - if (size == 0) { - return Collections.emptyMap(); - } - Map map = constructor.apply(size); - for (int i = 0; i < size; i++) { - K key = keyReader.read(this); - V value = valueReader.read(this); - map.put(key, value); - } - return map; - } - /** * If the returned map contains any entries it will be mutable. If it is empty it might be immutable. */ From 6ac87ef3a56ab3b7cc9cf6794fe446b50ceeebee Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sat, 27 Jan 2024 23:24:29 +0100 Subject: [PATCH 08/23] spotless check Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../opensearch/test/hamcrest/OpenSearchAssertions.java | 1 - .../org/opensearch/test/hamcrest/OpenSearchMatchers.java | 8 +++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java index ee5570301c428..9b0de13c35ec8 100644 --- a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java +++ b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java @@ -532,7 +532,6 @@ public static Matcher hasMatchedQueries(final String[] matchedQueries return new OpenSearchMatchers.SearchHitMatchedQueriesMatcher(matchedQueries); } - public static CombinableMatcher hasProperty(Function property, Matcher valueMatcher) { return OpenSearchMatchers.HasPropertyLambdaMatcher.hasProperty(property, valueMatcher); } diff --git a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchMatchers.java b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchMatchers.java index 5f661d7145ea5..2be94bd53e3c1 100644 --- a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchMatchers.java +++ b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchMatchers.java @@ -31,7 +31,6 @@ package org.opensearch.test.hamcrest; -import java.util.Arrays; import org.opensearch.search.SearchHit; import org.hamcrest.Description; import org.hamcrest.FeatureMatcher; @@ -39,6 +38,7 @@ import org.hamcrest.TypeSafeMatcher; import org.hamcrest.core.CombinableMatcher; +import java.util.Arrays; import java.util.function.Function; public class OpenSearchMatchers { @@ -132,14 +132,12 @@ protected boolean matchesSafely(SearchHit searchHit) { @Override public void describeMismatchSafely(final SearchHit searchHit, final Description mismatchDescription) { - mismatchDescription.appendText(" matched queries were ") - .appendValue(Arrays.toString(searchHit.getMatchedQueries())); + mismatchDescription.appendText(" matched queries were ").appendValue(Arrays.toString(searchHit.getMatchedQueries())); } @Override public void describeTo(final Description description) { - description.appendText("searchHit matched queries should be ") - .appendValue(Arrays.toString(matchedQueries)); + description.appendText("searchHit matched queries should be ").appendValue(Arrays.toString(matchedQueries)); } } From 5b447e7a91e01031f33c7ed4e635e21a063273ac Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sun, 28 Jan 2024 00:29:54 +0100 Subject: [PATCH 09/23] increase cluster timeout for flaky test Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../opensearch/indices/recovery/IndexPrimaryRelocationIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java index 9decd17d95eab..cbd3637da69b9 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java @@ -98,7 +98,7 @@ public void testPrimaryRelocationWhileIndexing() throws Exception { ClusterHealthResponse clusterHealthResponse = client().admin() .cluster() .prepareHealth() - .setTimeout(TimeValue.timeValueSeconds(60)) + .setTimeout(TimeValue.timeValueSeconds(120)) .setWaitForEvents(Priority.LANGUID) .setWaitForNoRelocatingShards(true) .execute() From 09257a37c528f1075878dab9f12116a8c940fbbf Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sun, 28 Jan 2024 13:27:17 +0100 Subject: [PATCH 10/23] add backwards compatible test for searchHit Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../test/java/org/opensearch/search/SearchHitTests.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/SearchHitTests.java b/server/src/test/java/org/opensearch/search/SearchHitTests.java index 9e1aaaff6c514..f9ed972695f7d 100644 --- a/server/src/test/java/org/opensearch/search/SearchHitTests.java +++ b/server/src/test/java/org/opensearch/search/SearchHitTests.java @@ -246,6 +246,14 @@ public void testSerializationDeserializationWithMatchedQueriesScores() throws IO assertEquals(searchHit.getMatchedQueriesAndScores(), deserializedSearchHit.getMatchedQueriesAndScores()); } + public void testSerializationDeserializationWithMatchedQueriesList() throws IOException { + SearchHit searchHit = createTestItem(true, true); + SearchHit deserializedSearchHit = copyWriteable(searchHit, getNamedWriteableRegistry(), SearchHit::new, Version.V_2_12_0); + assertEquals(searchHit, deserializedSearchHit); + assertEquals(searchHit.getMatchedQueriesAndScores(), deserializedSearchHit.getMatchedQueriesAndScores()); + assertEquals(searchHit.getMatchedQueries(), deserializedSearchHit.getMatchedQueries()); + } + /** * When e.g. with "stored_fields": "_none_", only "_index" and "_score" are returned. */ From f661895a77c125e02b68e87b8afc39907f731690 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sun, 28 Jan 2024 17:02:48 +0100 Subject: [PATCH 11/23] fix assertion by removing deprecated function Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- server/src/test/java/org/opensearch/search/SearchHitTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/search/SearchHitTests.java b/server/src/test/java/org/opensearch/search/SearchHitTests.java index f9ed972695f7d..2823852e3dbeb 100644 --- a/server/src/test/java/org/opensearch/search/SearchHitTests.java +++ b/server/src/test/java/org/opensearch/search/SearchHitTests.java @@ -34,6 +34,7 @@ import org.apache.lucene.search.Explanation; import org.apache.lucene.search.TotalHits; +import org.junit.Assert; import org.opensearch.Version; import org.opensearch.action.OriginalIndices; import org.opensearch.common.document.DocumentField; @@ -251,7 +252,7 @@ public void testSerializationDeserializationWithMatchedQueriesList() throws IOEx SearchHit deserializedSearchHit = copyWriteable(searchHit, getNamedWriteableRegistry(), SearchHit::new, Version.V_2_12_0); assertEquals(searchHit, deserializedSearchHit); assertEquals(searchHit.getMatchedQueriesAndScores(), deserializedSearchHit.getMatchedQueriesAndScores()); - assertEquals(searchHit.getMatchedQueries(), deserializedSearchHit.getMatchedQueries()); + Assert.assertArrayEquals(searchHit.getMatchedQueries(), deserializedSearchHit.getMatchedQueries()); } /** From 38c8546fd0164acca086be4d4123d10f4b12bac6 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Sun, 28 Jan 2024 17:29:15 +0100 Subject: [PATCH 12/23] spotless check fix Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- server/src/test/java/org/opensearch/search/SearchHitTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/search/SearchHitTests.java b/server/src/test/java/org/opensearch/search/SearchHitTests.java index 2823852e3dbeb..13b4d9f976ed5 100644 --- a/server/src/test/java/org/opensearch/search/SearchHitTests.java +++ b/server/src/test/java/org/opensearch/search/SearchHitTests.java @@ -34,7 +34,6 @@ import org.apache.lucene.search.Explanation; import org.apache.lucene.search.TotalHits; -import org.junit.Assert; import org.opensearch.Version; import org.opensearch.action.OriginalIndices; import org.opensearch.common.document.DocumentField; @@ -57,6 +56,7 @@ import org.opensearch.test.AbstractWireSerializingTestCase; import org.opensearch.test.RandomObjects; import org.opensearch.test.VersionUtils; +import org.junit.Assert; import java.io.IOException; import java.util.ArrayList; From b4ca4025443faae6b828cfee5c4c6fe915f177e7 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Mon, 29 Jan 2024 19:04:11 +0100 Subject: [PATCH 13/23] add javadoc comment to getMatchedQueryScore Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../indices/recovery/IndexPrimaryRelocationIT.java | 2 +- .../src/main/java/org/opensearch/search/SearchHit.java | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java index cbd3637da69b9..9decd17d95eab 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/recovery/IndexPrimaryRelocationIT.java @@ -98,7 +98,7 @@ public void testPrimaryRelocationWhileIndexing() throws Exception { ClusterHealthResponse clusterHealthResponse = client().admin() .cluster() .prepareHealth() - .setTimeout(TimeValue.timeValueSeconds(120)) + .setTimeout(TimeValue.timeValueSeconds(60)) .setWaitForEvents(Priority.LANGUID) .setWaitForNoRelocatingShards(true) .execute() diff --git a/server/src/main/java/org/opensearch/search/SearchHit.java b/server/src/main/java/org/opensearch/search/SearchHit.java index 733bd76fb3f62..1ab88793a68f6 100644 --- a/server/src/main/java/org/opensearch/search/SearchHit.java +++ b/server/src/main/java/org/opensearch/search/SearchHit.java @@ -565,7 +565,14 @@ public String[] getMatchedQueries() { } /** - * @return The score of the provided named query if it matches, {@code null} otherwise. + * Returns the score of the provided named query if it matches. + *

+ * If the 'include_named_queries_score' is not set, this method will return {@link Float#NaN} + * for each named query instead of a numerical score. + *

+ * + * @param name The name of the query to retrieve the score for. + * @return The score of the named query, or {@link Float#NaN} if 'include_named_queries_score' is not set. */ public Float getMatchedQueryScore(String name) { return getMatchedQueriesAndScores().get(name); From 8226d2b8bcf5fc9c3a4b1009a7d5855ee69abace Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Wed, 31 Jan 2024 20:18:35 +0100 Subject: [PATCH 14/23] add includeNamedQueriesScore and check to generate scores during MatchedQueriesPhase Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../rest/action/search/RestSearchAction.java | 4 +++ .../search/DefaultSearchContext.java | 13 ++++++++++ .../search/builder/SearchSourceBuilder.java | 25 +++++++++++++++++++ .../opensearch/search/fetch/FetchContext.java | 4 +++ .../opensearch/search/fetch/FetchPhase.java | 1 + .../fetch/subphase/MatchedQueriesPhase.java | 4 ++- .../internal/FilteredSearchContext.java | 2 ++ .../search/internal/SearchContext.java | 4 +++ .../search/internal/SubSearchContext.java | 13 ++++++++++ .../opensearch/test/TestSearchContext.java | 12 +++++++++ 10 files changed, 81 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java index 9b1bea362ffae..80dc34c4d5d68 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java @@ -290,6 +290,10 @@ private static void parseSearchSource(final SearchSourceBuilder searchSourceBuil searchSourceBuilder.trackScores(request.paramAsBoolean("track_scores", false)); } + if (request.hasParam("include_named_queries_score")) { + searchSourceBuilder.includeNamedQueriesScores(request.paramAsBoolean("include_named_queries_score", false)); + } + if (request.hasParam("track_total_hits")) { if (Booleans.isBoolean(request.param("track_total_hits"))) { searchSourceBuilder.trackTotalHits(request.paramAsBoolean("track_total_hits", true)); diff --git a/server/src/main/java/org/opensearch/search/DefaultSearchContext.java b/server/src/main/java/org/opensearch/search/DefaultSearchContext.java index 960b46d68977b..ff6f21625361e 100644 --- a/server/src/main/java/org/opensearch/search/DefaultSearchContext.java +++ b/server/src/main/java/org/opensearch/search/DefaultSearchContext.java @@ -149,6 +149,8 @@ final class DefaultSearchContext extends SearchContext { private SortAndFormats sort; private Float minimumScore; private boolean trackScores = false; // when sorting, track scores as well... + + private boolean includeNamedQueriesScore = false; private int trackTotalHitsUpTo = SearchContext.DEFAULT_TRACK_TOTAL_HITS_UP_TO; private FieldDoc searchAfter; private CollapseContext collapse; @@ -636,6 +638,17 @@ public boolean trackScores() { return this.trackScores; } + @Override + public SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore) { + this.includeNamedQueriesScore = includeNamedQueriesScore; + return this; + } + + @Override + public boolean includeNamedQueriesScore() { + return includeNamedQueriesScore; + } + @Override public SearchContext trackTotalHitsUpTo(int trackTotalHitsUpTo) { this.trackTotalHitsUpTo = trackTotalHitsUpTo; diff --git a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java index 434e630893f25..e7cfc6e7f7783 100644 --- a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java @@ -117,6 +117,7 @@ public final class SearchSourceBuilder implements Writeable, ToXContentObject, R public static final ParseField IGNORE_FAILURE_FIELD = new ParseField("ignore_failure"); public static final ParseField SORT_FIELD = new ParseField("sort"); public static final ParseField TRACK_SCORES_FIELD = new ParseField("track_scores"); + public static final ParseField INCLUDE_NAMED_QUERIES_SCORE = new ParseField("include_named_queries_score"); public static final ParseField TRACK_TOTAL_HITS_FIELD = new ParseField("track_total_hits"); public static final ParseField INDICES_BOOST_FIELD = new ParseField("indices_boost"); public static final ParseField AGGREGATIONS_FIELD = new ParseField("aggregations"); @@ -175,6 +176,8 @@ public static HighlightBuilder highlight() { private boolean trackScores = false; + private boolean includeNamedQueriesScore = false; + private Integer trackTotalHitsUpTo; private SearchAfterBuilder searchAfterBuilder; @@ -568,6 +571,22 @@ public SearchSourceBuilder trackScores(boolean trackScores) { return this; } + /** + * Applies when there are named queries, to return the scores along as well + * Defaults to {@code false}. + */ + public SearchSourceBuilder includeNamedQueriesScores(boolean includeNamedQueriesScore) { + this.includeNamedQueriesScore = includeNamedQueriesScore; + return this; + } + + /** + * Indicates whether scores will be returned as part of every search matched query.s + */ + public boolean includeNamedQueriesScore() { + return includeNamedQueriesScore; + } + /** * Indicates whether scores will be tracked for this request. */ @@ -1155,6 +1174,8 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th explain = parser.booleanValue(); } else if (TRACK_SCORES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { trackScores = parser.booleanValue(); + } else if (INCLUDE_NAMED_QUERIES_SCORE.match(currentFieldName, parser.getDeprecationHandler())) { + includeNamedQueriesScore = parser.booleanValue(); } else if (TRACK_TOTAL_HITS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { if (token == XContentParser.Token.VALUE_BOOLEAN || (token == XContentParser.Token.VALUE_STRING && Booleans.isBoolean(parser.text()))) { @@ -1418,6 +1439,10 @@ public XContentBuilder innerToXContent(XContentBuilder builder, Params params) t builder.field(TRACK_SCORES_FIELD.getPreferredName(), true); } + if (includeNamedQueriesScore) { + builder.field(INCLUDE_NAMED_QUERIES_SCORE.getPreferredName(), true); + } + if (trackTotalHitsUpTo != null) { builder.field(TRACK_TOTAL_HITS_FIELD.getPreferredName(), trackTotalHitsUpTo); } 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 7e36ace9e2112..5be3733106655 100644 --- a/server/src/main/java/org/opensearch/search/fetch/FetchContext.java +++ b/server/src/main/java/org/opensearch/search/fetch/FetchContext.java @@ -188,6 +188,10 @@ public boolean fetchScores() { return searchContext.sort() != null && searchContext.trackScores(); } + public boolean includeNamedQueriesScore() { + return searchContext.includeNamedQueriesScore(); + } + /** * Configuration for returning inner hits */ diff --git a/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java b/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java index a842c0f1adc6e..bd95002658cfa 100644 --- a/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java @@ -68,6 +68,7 @@ import org.opensearch.search.SearchHits; import org.opensearch.search.SearchShardTarget; import org.opensearch.search.fetch.FetchSubPhase.HitContext; +import org.opensearch.search.fetch.subphase.FetchScorePhase; import org.opensearch.search.fetch.subphase.FetchSourceContext; import org.opensearch.search.fetch.subphase.InnerHitsContext; import org.opensearch.search.fetch.subphase.InnerHitsPhase; diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java index 8e151dac2a7da..89a94b130eeaa 100644 --- a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java @@ -69,7 +69,9 @@ public FetchSubPhaseProcessor getProcessor(FetchContext context) throws IOExcept for (Map.Entry entry : namedQueries.entrySet()) { weights.put( entry.getKey(), - context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), ScoreMode.COMPLETE, 1) + context.includeNamedQueriesScore() + ? context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), ScoreMode.COMPLETE, 1) + : context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), ScoreMode.COMPLETE_NO_SCORES, 1) ); } return new FetchSubPhaseProcessor() { diff --git a/server/src/main/java/org/opensearch/search/internal/FilteredSearchContext.java b/server/src/main/java/org/opensearch/search/internal/FilteredSearchContext.java index 151ef97a2a141..a6ffe9ca5f649 100644 --- a/server/src/main/java/org/opensearch/search/internal/FilteredSearchContext.java +++ b/server/src/main/java/org/opensearch/search/internal/FilteredSearchContext.java @@ -340,6 +340,8 @@ public FieldDoc searchAfter() { return in.searchAfter(); } + public abstract SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore); + @Override public SearchContext parsedPostFilter(ParsedQuery postFilter) { return in.parsedPostFilter(postFilter); 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 02837da64dafd..28783a89be829 100644 --- a/server/src/main/java/org/opensearch/search/internal/SearchContext.java +++ b/server/src/main/java/org/opensearch/search/internal/SearchContext.java @@ -305,6 +305,10 @@ public final void assignRescoreDocIds(RescoreDocIds rescoreDocIds) { public abstract boolean trackScores(); + public abstract SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore); + + public abstract boolean includeNamedQueriesScore(); + public abstract SearchContext trackTotalHitsUpTo(int trackTotalHits); /** diff --git a/server/src/main/java/org/opensearch/search/internal/SubSearchContext.java b/server/src/main/java/org/opensearch/search/internal/SubSearchContext.java index 55315013ea8c9..b2c97baf78d91 100644 --- a/server/src/main/java/org/opensearch/search/internal/SubSearchContext.java +++ b/server/src/main/java/org/opensearch/search/internal/SubSearchContext.java @@ -82,6 +82,8 @@ public class SubSearchContext extends FilteredSearchContext { private boolean explain; private boolean trackScores; + + private boolean includeNamedQueriesScore; private boolean version; private boolean seqNoAndPrimaryTerm; @@ -234,6 +236,17 @@ public boolean trackScores() { return trackScores; } + @Override + public SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore) { + this.includeNamedQueriesScore = includeNamedQueriesScore; + return this; + } + + @Override + public boolean includeNamedQueriesScore() { + return includeNamedQueriesScore; + } + @Override public SearchContext parsedPostFilter(ParsedQuery postFilter) { throw new UnsupportedOperationException("Not supported"); diff --git a/test/framework/src/main/java/org/opensearch/test/TestSearchContext.java b/test/framework/src/main/java/org/opensearch/test/TestSearchContext.java index 2fb345f73fb06..09a72dcdc3641 100644 --- a/test/framework/src/main/java/org/opensearch/test/TestSearchContext.java +++ b/test/framework/src/main/java/org/opensearch/test/TestSearchContext.java @@ -107,6 +107,7 @@ public class TestSearchContext extends SearchContext { SearchShardTask task; SortAndFormats sort; boolean trackScores = false; + boolean includeNamedQueriesScore = false; int trackTotalHitsUpTo = SearchContext.DEFAULT_TRACK_TOTAL_HITS_UP_TO; ContextIndexSearcher searcher; @@ -409,6 +410,17 @@ public boolean trackScores() { return trackScores; } + @Override + public SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore) { + this.includeNamedQueriesScore = includeNamedQueriesScore; + return this; + } + + @Override + public boolean includeNamedQueriesScore() { + return includeNamedQueriesScore; + } + @Override public SearchContext trackTotalHitsUpTo(int trackTotalHitsUpTo) { this.trackTotalHitsUpTo = trackTotalHitsUpTo; From 580773e1d4be9ea676788ad0722a9b4be241bbde Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Wed, 31 Jan 2024 21:06:48 +0100 Subject: [PATCH 15/23] spotless java fix Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- server/src/main/java/org/opensearch/search/fetch/FetchPhase.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java b/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java index bd95002658cfa..a842c0f1adc6e 100644 --- a/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java @@ -68,7 +68,6 @@ import org.opensearch.search.SearchHits; import org.opensearch.search.SearchShardTarget; import org.opensearch.search.fetch.FetchSubPhase.HitContext; -import org.opensearch.search.fetch.subphase.FetchScorePhase; import org.opensearch.search.fetch.subphase.FetchSourceContext; import org.opensearch.search.fetch.subphase.InnerHitsContext; import org.opensearch.search.fetch.subphase.InnerHitsPhase; From bb0c0d8e9e592c38b392de93a71c04523d13bde1 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Wed, 31 Jan 2024 23:21:53 +0100 Subject: [PATCH 16/23] refactor MatchedQueriesPhase to return different processors depending on context Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../opensearch/search/fetch/FetchPhase.java | 2 +- .../fetch/subphase/MatchedQueriesPhase.java | 147 ++++++++++-------- .../search/internal/SearchContext.java | 23 ++- 3 files changed, 103 insertions(+), 69 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java b/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java index a842c0f1adc6e..1698f41caaf2b 100644 --- a/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/FetchPhase.java @@ -91,7 +91,7 @@ /** * Fetch phase of a search request, used to fetch the actual top matching documents to be returned to the client, identified - * after reducing all of the matches returned by the query phase + * after reducing all the matches returned by the query phase * * @opensearch.api */ diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java index 89a94b130eeaa..11afeaca1c4e8 100644 --- a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java @@ -1,34 +1,3 @@ -/* - * 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. - */ - -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -/* - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - package org.opensearch.search.fetch.subphase; import org.apache.lucene.index.LeafReaderContext; @@ -37,71 +6,69 @@ import org.apache.lucene.search.Scorer; import org.apache.lucene.search.ScorerSupplier; import org.apache.lucene.search.Weight; +import org.apache.lucene.util.Bits; +import org.opensearch.common.lucene.Lucene; import org.opensearch.search.fetch.FetchContext; import org.opensearch.search.fetch.FetchSubPhase; import org.opensearch.search.fetch.FetchSubPhaseProcessor; import java.io.IOException; +import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; +import java.util.Optional; -/** - * Fetches queries that match the document during search phase - * - * @opensearch.internal - */ public final class MatchedQueriesPhase implements FetchSubPhase { @Override public FetchSubPhaseProcessor getProcessor(FetchContext context) throws IOException { - Map namedQueries = new HashMap<>(); - if (context.parsedQuery() != null) { - namedQueries.putAll(context.parsedQuery().namedFilters()); - } - if (context.parsedPostFilter() != null) { - namedQueries.putAll(context.parsedPostFilter().namedFilters()); - } + Map namedQueries = collectNamedQueries(context); if (namedQueries.isEmpty()) { return null; } + + Map weights = prepareWeights(context, namedQueries); + + return context.includeNamedQueriesScore() ? createScoringProcessor(weights) : createNonScoringProcessor(weights); + } + + private Map collectNamedQueries(FetchContext context) { + Map namedQueries = new HashMap<>(); + Optional.ofNullable(context.parsedQuery()).ifPresent(parsedQuery -> namedQueries.putAll(parsedQuery.namedFilters())); + Optional.ofNullable(context.parsedPostFilter()).ifPresent(parsedPostFilter -> namedQueries.putAll(parsedPostFilter.namedFilters())); + return namedQueries; + } + + private Map prepareWeights(FetchContext context, Map namedQueries) throws IOException { Map weights = new HashMap<>(); for (Map.Entry entry : namedQueries.entrySet()) { - weights.put( - entry.getKey(), - context.includeNamedQueriesScore() - ? context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), ScoreMode.COMPLETE, 1) - : context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), ScoreMode.COMPLETE_NO_SCORES, 1) - ); + ScoreMode scoreMode = context.includeNamedQueriesScore() ? ScoreMode.COMPLETE : ScoreMode.COMPLETE_NO_SCORES; + weights.put(entry.getKey(), context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), scoreMode, 1)); } - return new FetchSubPhaseProcessor() { + return weights; + } - final Map matchingIterators = new HashMap<>(); + private FetchSubPhaseProcessor createScoringProcessor(Map weights) { + return new FetchSubPhaseProcessor() { + final Map matchingScorers = new HashMap<>(); @Override public void setNextReader(LeafReaderContext readerContext) throws IOException { - matchingIterators.clear(); - for (Map.Entry entry : weights.entrySet()) { - ScorerSupplier ss = entry.getValue().scorerSupplier(readerContext); - if (ss != null) { - Scorer scorer = ss.get(0L); - if (scorer != null) { - matchingIterators.put(entry.getKey(), scorer); - } - } - } + setupScorers(readerContext, weights, matchingScorers); } @Override public void process(HitContext hitContext) throws IOException { Map matches = new LinkedHashMap<>(); - int doc = hitContext.docId(); - for (Map.Entry entry : matchingIterators.entrySet()) { + int docId = hitContext.docId(); + for (Map.Entry entry : matchingScorers.entrySet()) { Scorer scorer = entry.getValue(); - if (scorer.iterator().docID() < doc) { - scorer.iterator().advance(doc); + if (scorer.iterator().docID() < docId) { + scorer.iterator().advance(docId); } - if (scorer.iterator().docID() == doc) { + if (scorer.iterator().docID() == docId) { matches.put(entry.getKey(), scorer.score()); } } @@ -110,4 +77,52 @@ public void process(HitContext hitContext) throws IOException { }; } + private FetchSubPhaseProcessor createNonScoringProcessor(Map weights) { + return new FetchSubPhaseProcessor() { + final Map matchingBits = new HashMap<>(); + + @Override + public void setNextReader(LeafReaderContext readerContext) throws IOException { + setupMatchingBits(readerContext, weights, matchingBits); + } + + @Override + public void process(HitContext hitContext) { + List matches = new ArrayList<>(); + int docId = hitContext.docId(); + for (Map.Entry entry : matchingBits.entrySet()) { + if (entry.getValue().get(docId)) { + matches.add(entry.getKey()); + } + } + hitContext.hit().matchedQueries(matches.toArray(new String[0])); + } + }; + } + + private void setupScorers(LeafReaderContext readerContext, Map weights, Map scorers) + throws IOException { + scorers.clear(); + for (Map.Entry entry : weights.entrySet()) { + ScorerSupplier scorerSupplier = entry.getValue().scorerSupplier(readerContext); + if (scorerSupplier != null) { + Scorer scorer = scorerSupplier.get(0L); + if (scorer != null) { + scorers.put(entry.getKey(), scorer); + } + } + } + } + + private void setupMatchingBits(LeafReaderContext readerContext, Map weights, Map bitsMap) + throws IOException { + bitsMap.clear(); + for (Map.Entry entry : weights.entrySet()) { + ScorerSupplier scorerSupplier = entry.getValue().scorerSupplier(readerContext); + if (scorerSupplier != null) { + Bits bits = Lucene.asSequentialAccessBits(readerContext.reader().maxDoc(), scorerSupplier); + bitsMap.put(entry.getKey(), bits); + } + } + } } 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 28783a89be829..cd8f9f8410d50 100644 --- a/server/src/main/java/org/opensearch/search/internal/SearchContext.java +++ b/server/src/main/java/org/opensearch/search/internal/SearchContext.java @@ -305,9 +305,28 @@ public final void assignRescoreDocIds(RescoreDocIds rescoreDocIds) { public abstract boolean trackScores(); - public abstract SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore); + /** + * Determines whether named queries' scores should be included in the search results. + * By default, this is set to return false, indicating that scores from named queries are not included. + * + * @param includeNamedQueriesScore true to include scores from named queries, false otherwise. + */ + public SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore) { + // Default implementation does nothing and returns this for chaining. + // Implementations of SearchContext should override this method to actually store the value. + return this; + } - public abstract boolean includeNamedQueriesScore(); + /** + * Checks if scores from named queries are included in the search results. + * + * @return true if scores from named queries are included, false otherwise. + */ + public boolean includeNamedQueriesScore() { + // Default implementation returns false. + // Implementations of SearchContext should override this method to return the actual value. + return false; + } public abstract SearchContext trackTotalHitsUpTo(int trackTotalHits); From 860d2aa76576a64f6a0db6cf31e1c8dfded2cfe0 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Wed, 31 Jan 2024 23:34:59 +0100 Subject: [PATCH 17/23] cleanup MatchedQueriesPhase based on review comments Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../fetch/subphase/MatchedQueriesPhase.java | 64 ++++++++----------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java index 11afeaca1c4e8..5975c331d6aef 100644 --- a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java @@ -18,13 +18,18 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Optional; public final class MatchedQueriesPhase implements FetchSubPhase { @Override public FetchSubPhaseProcessor getProcessor(FetchContext context) throws IOException { - Map namedQueries = collectNamedQueries(context); + Map namedQueries = new HashMap<>(); + if (context.parsedQuery() != null) { + namedQueries.putAll(context.parsedQuery().namedFilters()); + } + if (context.parsedPostFilter() != null) { + namedQueries.putAll(context.parsedPostFilter().namedFilters()); + } if (namedQueries.isEmpty()) { return null; } @@ -34,17 +39,10 @@ public FetchSubPhaseProcessor getProcessor(FetchContext context) throws IOExcept return context.includeNamedQueriesScore() ? createScoringProcessor(weights) : createNonScoringProcessor(weights); } - private Map collectNamedQueries(FetchContext context) { - Map namedQueries = new HashMap<>(); - Optional.ofNullable(context.parsedQuery()).ifPresent(parsedQuery -> namedQueries.putAll(parsedQuery.namedFilters())); - Optional.ofNullable(context.parsedPostFilter()).ifPresent(parsedPostFilter -> namedQueries.putAll(parsedPostFilter.namedFilters())); - return namedQueries; - } - private Map prepareWeights(FetchContext context, Map namedQueries) throws IOException { Map weights = new HashMap<>(); + ScoreMode scoreMode = context.includeNamedQueriesScore() ? ScoreMode.COMPLETE : ScoreMode.COMPLETE_NO_SCORES; for (Map.Entry entry : namedQueries.entrySet()) { - ScoreMode scoreMode = context.includeNamedQueriesScore() ? ScoreMode.COMPLETE : ScoreMode.COMPLETE_NO_SCORES; weights.put(entry.getKey(), context.searcher().createWeight(context.searcher().rewrite(entry.getValue()), scoreMode, 1)); } return weights; @@ -56,7 +54,16 @@ private FetchSubPhaseProcessor createScoringProcessor(Map weight @Override public void setNextReader(LeafReaderContext readerContext) throws IOException { - setupScorers(readerContext, weights, matchingScorers); + matchingScorers.clear(); + for (Map.Entry entry : weights.entrySet()) { + ScorerSupplier scorerSupplier = entry.getValue().scorerSupplier(readerContext); + if (scorerSupplier != null) { + Scorer scorer = scorerSupplier.get(0L); + if (scorer != null) { + matchingScorers.put(entry.getKey(), scorer); + } + } + } } @Override @@ -83,7 +90,14 @@ private FetchSubPhaseProcessor createNonScoringProcessor(Map wei @Override public void setNextReader(LeafReaderContext readerContext) throws IOException { - setupMatchingBits(readerContext, weights, matchingBits); + matchingBits.clear(); + for (Map.Entry entry : weights.entrySet()) { + ScorerSupplier scorerSupplier = entry.getValue().scorerSupplier(readerContext); + if (scorerSupplier != null) { + Bits bits = Lucene.asSequentialAccessBits(readerContext.reader().maxDoc(), scorerSupplier); + matchingBits.put(entry.getKey(), bits); + } + } } @Override @@ -99,30 +113,4 @@ public void process(HitContext hitContext) { } }; } - - private void setupScorers(LeafReaderContext readerContext, Map weights, Map scorers) - throws IOException { - scorers.clear(); - for (Map.Entry entry : weights.entrySet()) { - ScorerSupplier scorerSupplier = entry.getValue().scorerSupplier(readerContext); - if (scorerSupplier != null) { - Scorer scorer = scorerSupplier.get(0L); - if (scorer != null) { - scorers.put(entry.getKey(), scorer); - } - } - } - } - - private void setupMatchingBits(LeafReaderContext readerContext, Map weights, Map bitsMap) - throws IOException { - bitsMap.clear(); - for (Map.Entry entry : weights.entrySet()) { - ScorerSupplier scorerSupplier = entry.getValue().scorerSupplier(readerContext); - if (scorerSupplier != null) { - Bits bits = Lucene.asSequentialAccessBits(readerContext.reader().maxDoc(), scorerSupplier); - bitsMap.put(entry.getKey(), bits); - } - } - } } From 0bdfa7d346c6b286baaf90ba54d2a6228c3130b0 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Wed, 31 Jan 2024 23:45:21 +0100 Subject: [PATCH 18/23] revert back java docs and header Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../fetch/subphase/MatchedQueriesPhase.java | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java index 5975c331d6aef..406d9c8b4bc03 100644 --- a/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java +++ b/server/src/main/java/org/opensearch/search/fetch/subphase/MatchedQueriesPhase.java @@ -1,3 +1,33 @@ +/* + * 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. + */ + +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/* + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ package org.opensearch.search.fetch.subphase; import org.apache.lucene.index.LeafReaderContext; @@ -19,6 +49,11 @@ import java.util.List; import java.util.Map; +/** + * Fetches queries that match the document during search phase + * + * @opensearch.internal + */ public final class MatchedQueriesPhase implements FetchSubPhase { @Override From 34186479e6cb6d010a03aac1504f66a5a070332a Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Thu, 1 Feb 2024 22:21:24 +0100 Subject: [PATCH 19/23] remove immutable map init Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- server/src/main/java/org/opensearch/search/SearchHit.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/search/SearchHit.java b/server/src/main/java/org/opensearch/search/SearchHit.java index 1ab88793a68f6..a3db7e0893a3d 100644 --- a/server/src/main/java/org/opensearch/search/SearchHit.java +++ b/server/src/main/java/org/opensearch/search/SearchHit.java @@ -122,7 +122,7 @@ public final class SearchHit implements Writeable, ToXContentObject, Iterable matchedQueries = Collections.emptyMap(); + private Map matchedQueries = new HashMap<>(); private Explanation explanation; From 6881e104f515c9b19123570e7fd8e7328a946bf8 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Fri, 2 Feb 2024 00:17:40 +0100 Subject: [PATCH 20/23] fix include_named_queries_score propogation through request Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../search/fetch/subphase/MatchedQueriesIT.java | 9 +++++++-- .../opensearch/action/search/SearchRequestBuilder.java | 9 +++++++++ .../main/java/org/opensearch/search/SearchService.java | 1 + .../opensearch/search/builder/SearchSourceBuilder.java | 5 +++++ .../search/internal/FilteredSearchContext.java | 8 +++++++- 5 files changed, 29 insertions(+), 3 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/search/fetch/subphase/MatchedQueriesIT.java b/server/src/internalClusterTest/java/org/opensearch/search/fetch/subphase/MatchedQueriesIT.java index 5c3ee0b16c654..0ffdfff8c36f5 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/fetch/subphase/MatchedQueriesIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/fetch/subphase/MatchedQueriesIT.java @@ -103,6 +103,7 @@ public void testSimpleMatchedQueryFromFilteredQuery() throws Exception { .should(rangeQuery("number").gte(2).queryName("test2")) ) ) + .setIncludeNamedQueriesScore(true) .get(); assertHitCount(searchResponse, 3L); for (SearchHit hit : searchResponse.getHits()) { @@ -123,6 +124,7 @@ public void testSimpleMatchedQueryFromFilteredQuery() throws Exception { .setQuery( boolQuery().should(rangeQuery("number").lte(2).queryName("test1")).should(rangeQuery("number").gt(2).queryName("test2")) ) + .setIncludeNamedQueriesScore(true) .get(); assertHitCount(searchResponse, 3L); for (SearchHit hit : searchResponse.getHits()) { @@ -251,6 +253,7 @@ public void testRegExpQuerySupportsName() throws InterruptedException { SearchResponse searchResponse = client().prepareSearch() .setQuery(QueryBuilders.regexpQuery("title", "title1").queryName("regex")) + .setIncludeNamedQueriesScore(true) .get(); assertHitCount(searchResponse, 1L); @@ -273,9 +276,10 @@ public void testPrefixQuerySupportsName() throws InterruptedException { refresh(); indexRandomForConcurrentSearch("test1"); - SearchResponse searchResponse = client().prepareSearch() + var query = client().prepareSearch() .setQuery(QueryBuilders.prefixQuery("title", "title").queryName("prefix")) - .get(); + .setIncludeNamedQueriesScore(true); + var searchResponse = query.get(); assertHitCount(searchResponse, 1L); for (SearchHit hit : searchResponse.getHits()) { @@ -323,6 +327,7 @@ public void testWildcardQuerySupportsName() throws InterruptedException { SearchResponse searchResponse = client().prepareSearch() .setQuery(QueryBuilders.wildcardQuery("title", "titl*").queryName("wildcard")) + .setIncludeNamedQueriesScore(true) .get(); assertHitCount(searchResponse, 1L); diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestBuilder.java b/server/src/main/java/org/opensearch/action/search/SearchRequestBuilder.java index e949c5e0bea29..9dac827e7d518 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestBuilder.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestBuilder.java @@ -406,6 +406,15 @@ public SearchRequestBuilder setTrackScores(boolean trackScores) { return this; } + /** + * Applies when fetching scores with named queries, and controls if scores will be tracked as well. + * Defaults to {@code false}. + */ + public SearchRequestBuilder setIncludeNamedQueriesScore(boolean includeNamedQueriesScore) { + sourceBuilder().includeNamedQueriesScores(includeNamedQueriesScore); + return this; + } + /** * Indicates if the total hit count for the query should be tracked. Requests will count total hit count accurately * up to 10,000 by default, see {@link #setTrackTotalHitsUpTo(int)} to change this value or set to true/false to always/never diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index 2c85fcbb25f35..8eb960e735390 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -1274,6 +1274,7 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc } } context.trackScores(source.trackScores()); + context.includeNamedQueriesScore(source.includeNamedQueriesScore()); if (source.trackTotalHitsUpTo() != null && source.trackTotalHitsUpTo() != SearchContext.TRACK_TOTAL_HITS_ACCURATE && context.scrollContext() != null) { diff --git a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java index e7cfc6e7f7783..ab4a0c4f070c2 100644 --- a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java @@ -262,6 +262,7 @@ public SearchSourceBuilder(StreamInput in) throws IOException { terminateAfter = in.readVInt(); timeout = in.readOptionalTimeValue(); trackScores = in.readBoolean(); + includeNamedQueriesScore = in.readBoolean(); version = in.readOptionalBoolean(); seqNoAndPrimaryTerm = in.readOptionalBoolean(); extBuilders = in.readNamedWriteableList(SearchExtBuilder.class); @@ -325,6 +326,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeVInt(terminateAfter); out.writeOptionalTimeValue(timeout); out.writeBoolean(trackScores); + out.writeBoolean(includeNamedQueriesScore); out.writeOptionalBoolean(version); out.writeOptionalBoolean(seqNoAndPrimaryTerm); out.writeNamedWriteableList(extBuilders); @@ -1122,6 +1124,7 @@ private SearchSourceBuilder shallowCopy( rewrittenBuilder.terminateAfter = terminateAfter; rewrittenBuilder.timeout = timeout; rewrittenBuilder.trackScores = trackScores; + rewrittenBuilder.includeNamedQueriesScore = includeNamedQueriesScore; rewrittenBuilder.trackTotalHitsUpTo = trackTotalHitsUpTo; rewrittenBuilder.version = version; rewrittenBuilder.seqNoAndPrimaryTerm = seqNoAndPrimaryTerm; @@ -1774,6 +1777,7 @@ public int hashCode() { terminateAfter, timeout, trackScores, + includeNamedQueriesScore, version, seqNoAndPrimaryTerm, profile, @@ -1816,6 +1820,7 @@ public boolean equals(Object obj) { && Objects.equals(terminateAfter, other.terminateAfter) && Objects.equals(timeout, other.timeout) && Objects.equals(trackScores, other.trackScores) + && Objects.equals(includeNamedQueriesScore, other.includeNamedQueriesScore) && Objects.equals(version, other.version) && Objects.equals(seqNoAndPrimaryTerm, other.seqNoAndPrimaryTerm) && Objects.equals(profile, other.profile) diff --git a/server/src/main/java/org/opensearch/search/internal/FilteredSearchContext.java b/server/src/main/java/org/opensearch/search/internal/FilteredSearchContext.java index a6ffe9ca5f649..3a3b46366a6d2 100644 --- a/server/src/main/java/org/opensearch/search/internal/FilteredSearchContext.java +++ b/server/src/main/java/org/opensearch/search/internal/FilteredSearchContext.java @@ -340,7 +340,13 @@ public FieldDoc searchAfter() { return in.searchAfter(); } - public abstract SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore); + public SearchContext includeNamedQueriesScore(boolean includeNamedQueriesScore) { + return in.includeNamedQueriesScore(includeNamedQueriesScore); + } + + public boolean includeNamedQueriesScore() { + return in.includeNamedQueriesScore(); + } @Override public SearchContext parsedPostFilter(ParsedQuery postFilter) { From 1c589491e4cea8cece6cba12df6f18c7e5e78937 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Fri, 2 Feb 2024 01:11:05 +0100 Subject: [PATCH 21/23] fix backwards incompatible io stream Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../opensearch/search/builder/SearchSourceBuilder.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java index ab4a0c4f070c2..56712153000dd 100644 --- a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java @@ -176,7 +176,7 @@ public static HighlightBuilder highlight() { private boolean trackScores = false; - private boolean includeNamedQueriesScore = false; + private Boolean includeNamedQueriesScore = false; private Integer trackTotalHitsUpTo; @@ -262,7 +262,7 @@ public SearchSourceBuilder(StreamInput in) throws IOException { terminateAfter = in.readVInt(); timeout = in.readOptionalTimeValue(); trackScores = in.readBoolean(); - includeNamedQueriesScore = in.readBoolean(); + includeNamedQueriesScore = in.readOptionalBoolean(); version = in.readOptionalBoolean(); seqNoAndPrimaryTerm = in.readOptionalBoolean(); extBuilders = in.readNamedWriteableList(SearchExtBuilder.class); @@ -326,7 +326,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeVInt(terminateAfter); out.writeOptionalTimeValue(timeout); out.writeBoolean(trackScores); - out.writeBoolean(includeNamedQueriesScore); + out.writeOptionalBoolean(includeNamedQueriesScore); out.writeOptionalBoolean(version); out.writeOptionalBoolean(seqNoAndPrimaryTerm); out.writeNamedWriteableList(extBuilders); @@ -586,7 +586,7 @@ public SearchSourceBuilder includeNamedQueriesScores(boolean includeNamedQueries * Indicates whether scores will be returned as part of every search matched query.s */ public boolean includeNamedQueriesScore() { - return includeNamedQueriesScore; + return includeNamedQueriesScore != null && includeNamedQueriesScore; } /** From 9487a23aea584642385836674662cfa86a013028 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Fri, 2 Feb 2024 02:07:37 +0100 Subject: [PATCH 22/23] fix backwards compatible tests Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../opensearch/search/builder/SearchSourceBuilder.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java index 56712153000dd..d543b70b27092 100644 --- a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java @@ -176,7 +176,7 @@ public static HighlightBuilder highlight() { private boolean trackScores = false; - private Boolean includeNamedQueriesScore = false; + private Boolean includeNamedQueriesScore; private Integer trackTotalHitsUpTo; @@ -262,7 +262,6 @@ public SearchSourceBuilder(StreamInput in) throws IOException { terminateAfter = in.readVInt(); timeout = in.readOptionalTimeValue(); trackScores = in.readBoolean(); - includeNamedQueriesScore = in.readOptionalBoolean(); version = in.readOptionalBoolean(); seqNoAndPrimaryTerm = in.readOptionalBoolean(); extBuilders = in.readNamedWriteableList(SearchExtBuilder.class); @@ -280,6 +279,9 @@ public SearchSourceBuilder(StreamInput in) throws IOException { searchPipelineSource = in.readMap(); } } + if (in.getVersion().onOrAfter(Version.V_3_0_0)) { + includeNamedQueriesScore = in.readOptionalBoolean(); + } } @Override @@ -326,7 +328,6 @@ public void writeTo(StreamOutput out) throws IOException { out.writeVInt(terminateAfter); out.writeOptionalTimeValue(timeout); out.writeBoolean(trackScores); - out.writeOptionalBoolean(includeNamedQueriesScore); out.writeOptionalBoolean(version); out.writeOptionalBoolean(seqNoAndPrimaryTerm); out.writeNamedWriteableList(extBuilders); @@ -346,6 +347,9 @@ public void writeTo(StreamOutput out) throws IOException { out.writeMap(searchPipelineSource); } } + if (out.getVersion().onOrAfter(Version.V_3_0_0)) { + out.writeOptionalBoolean(includeNamedQueriesScore); + } } /** From 67b0bc3f68ca522cf0e8edae48b580b53fb8f877 Mon Sep 17 00:00:00 2001 From: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> Date: Fri, 2 Feb 2024 02:25:27 +0100 Subject: [PATCH 23/23] fix boolean null check Signed-off-by: Dharin Shah <8616130+Dharin-shah@users.noreply.github.com> --- .../org/opensearch/search/builder/SearchSourceBuilder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java index d543b70b27092..bdd92a5baa115 100644 --- a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java @@ -1446,8 +1446,8 @@ public XContentBuilder innerToXContent(XContentBuilder builder, Params params) t builder.field(TRACK_SCORES_FIELD.getPreferredName(), true); } - if (includeNamedQueriesScore) { - builder.field(INCLUDE_NAMED_QUERIES_SCORE.getPreferredName(), true); + if (includeNamedQueriesScore != null) { + builder.field(INCLUDE_NAMED_QUERIES_SCORE.getPreferredName(), includeNamedQueriesScore); } if (trackTotalHitsUpTo != null) {