From 3b7077c8367b5d895ca52f49a9e4f8bcd6420493 Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 22 May 2025 12:31:00 +0100 Subject: [PATCH 01/12] Fix scoring and sort handling in pinned retriever --- .../retriever/PinnedRetrieverBuilder.java | 15 +---- .../searchbusinessrules/retriever/books.es | 58 +++++++++++++++++++ .../PinnedRetrieverBuilderTests.java | 11 +++- 3 files changed, 71 insertions(+), 13 deletions(-) create mode 100644 x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es diff --git a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java index 60ac7019d6a90..0e254b4fcd5b3 100644 --- a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java +++ b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java @@ -18,9 +18,7 @@ import org.elasticsearch.search.retriever.RetrieverBuilder; import org.elasticsearch.search.retriever.RetrieverBuilderWrapper; import org.elasticsearch.search.retriever.RetrieverParserContext; -import org.elasticsearch.search.sort.FieldSortBuilder; import org.elasticsearch.search.sort.ScoreSortBuilder; -import org.elasticsearch.search.sort.ShardDocSortField; import org.elasticsearch.search.sort.SortBuilder; import org.elasticsearch.xcontent.ConstructingObjectParser; import org.elasticsearch.xcontent.ParseField; @@ -106,16 +104,9 @@ private void validateSort(SearchSourceBuilder source) { if (sorts == null || sorts.isEmpty()) { return; } - for (SortBuilder sort : sorts) { - if (sort instanceof ScoreSortBuilder) { - continue; - } - if (sort instanceof FieldSortBuilder) { - FieldSortBuilder fieldSort = (FieldSortBuilder) sort; - if (ShardDocSortField.NAME.equals(fieldSort.getFieldName())) { - continue; - } - } + + SortBuilder sort = sorts.get(0); + if (sort instanceof ScoreSortBuilder == false) { throw new IllegalArgumentException( "[" + NAME + "] retriever only supports sorting by score, invalid sort criterion: " + sort.toString() ); diff --git a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es new file mode 100644 index 0000000000000..4843b1af7da5a --- /dev/null +++ b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es @@ -0,0 +1,58 @@ +PUT books +{ + "mappings": { + "properties": { + "title": { + "type": "text" + }, + "author": { + "type": "text" + }, + "publication_year": { + "type": "integer" + }, + "genre": { + "type": "keyword" + } + } + } +} + +# Add some sample books +POST books/_bulk +{ "index": { "_id": "1" } } +{ "title": "Neuromancer", "author": "William Gibson", "publication_year": 1984, "genre": "science_fiction" } +{ "index": { "_id": "2" } } +{ "title": "The Alienist", "author": "Caleb Carr", "publication_year": 1994, "genre": "mystery" } +{ "index": { "_id": "3" } } +{ "title": "Alien: River of Pain", "author": "Tim Lebbon", "publication_year": 2014, "genre": "science_fiction" } + + +# Test query with pinned retriever +GET books/_search +{ + "search.retriever": { + "pinned": { + "retriever": { + "standard": { + "query": { + "multi_match": { + "query": "alien" + } + }, + "sort": [ + { + "publication_year": "desc" + } + ] + } + }, + "ids": [ + { + "_id": "1", + "_index": "books" + } + ] + } + } +} \ No newline at end of file diff --git a/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java b/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java index 38a2299ca26f9..3cd9a39fb19cb 100644 --- a/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java +++ b/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java @@ -209,7 +209,16 @@ public void testValidateSort() { multipleSortsSource.query(dummyQuery); multipleSortsSource.sort("_score"); multipleSortsSource.sort("field1"); - e = expectThrows(IllegalArgumentException.class, () -> builder.finalizeSourceBuilder(multipleSortsSource)); + builder.finalizeSourceBuilder(multipleSortsSource); + assertThat(multipleSortsSource.sorts().size(), equalTo(2)); + assertThat(multipleSortsSource.sorts().get(0).toString(), equalTo("{\n \"_score\" : {\n \"order\" : \"desc\"\n }\n}")); + assertThat(multipleSortsSource.sorts().get(1).toString(), equalTo("{\n \"field1\" : {\n \"order\" : \"asc\"\n }\n}")); + + SearchSourceBuilder fieldFirstSource = new SearchSourceBuilder(); + fieldFirstSource.query(dummyQuery); + fieldFirstSource.sort("field1"); + fieldFirstSource.sort("_score"); + e = expectThrows(IllegalArgumentException.class, () -> builder.finalizeSourceBuilder(fieldFirstSource)); assertThat( e.getMessage(), equalTo( From d6e2c22043af21abaf0958bf4fa5257e1e58b003 Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 22 May 2025 16:29:34 +0100 Subject: [PATCH 02/12] Remove books.es from version control and add to .gitignore --- .gitignore | 1 + .../searchbusinessrules/retriever/books.es | 58 ------------------- 2 files changed, 1 insertion(+), 58 deletions(-) delete mode 100644 x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es diff --git a/.gitignore b/.gitignore index 8b2da4dc0832a..2de31da7b3b62 100644 --- a/.gitignore +++ b/.gitignore @@ -72,3 +72,4 @@ x-pack/plugin/esql/src/main/generated-src/generated/ # JEnv .java-version +x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es diff --git a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es deleted file mode 100644 index 4843b1af7da5a..0000000000000 --- a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es +++ /dev/null @@ -1,58 +0,0 @@ -PUT books -{ - "mappings": { - "properties": { - "title": { - "type": "text" - }, - "author": { - "type": "text" - }, - "publication_year": { - "type": "integer" - }, - "genre": { - "type": "keyword" - } - } - } -} - -# Add some sample books -POST books/_bulk -{ "index": { "_id": "1" } } -{ "title": "Neuromancer", "author": "William Gibson", "publication_year": 1984, "genre": "science_fiction" } -{ "index": { "_id": "2" } } -{ "title": "The Alienist", "author": "Caleb Carr", "publication_year": 1994, "genre": "mystery" } -{ "index": { "_id": "3" } } -{ "title": "Alien: River of Pain", "author": "Tim Lebbon", "publication_year": 2014, "genre": "science_fiction" } - - -# Test query with pinned retriever -GET books/_search -{ - "search.retriever": { - "pinned": { - "retriever": { - "standard": { - "query": { - "multi_match": { - "query": "alien" - } - }, - "sort": [ - { - "publication_year": "desc" - } - ] - } - }, - "ids": [ - { - "_id": "1", - "_index": "books" - } - ] - } - } -} \ No newline at end of file From 5ea054c6cd6eccb73d85218a9f012318db601ef1 Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 22 May 2025 16:32:39 +0100 Subject: [PATCH 03/12] Remove books.es from version control and add to .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 2de31da7b3b62..87c963e14ead9 100644 --- a/.gitignore +++ b/.gitignore @@ -73,3 +73,4 @@ x-pack/plugin/esql/src/main/generated-src/generated/ # JEnv .java-version x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es +x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es From 0c7e5a9a611623f4ef125d35a24fd306042938d1 Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 22 May 2025 16:34:22 +0100 Subject: [PATCH 04/12] Remove books.es entries from .gitignore --- .gitignore | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 87c963e14ead9..1a47ed167d8dd 100644 --- a/.gitignore +++ b/.gitignore @@ -71,6 +71,4 @@ checkstyle_ide.xml x-pack/plugin/esql/src/main/generated-src/generated/ # JEnv -.java-version -x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es -x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/books.es +.java-version \ No newline at end of file From 4686f62dd72088d5b4f548c7e4722f8c42e54575 Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 22 May 2025 16:35:12 +0100 Subject: [PATCH 05/12] fixed the mess --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 1a47ed167d8dd..8b2da4dc0832a 100644 --- a/.gitignore +++ b/.gitignore @@ -71,4 +71,4 @@ checkstyle_ide.xml x-pack/plugin/esql/src/main/generated-src/generated/ # JEnv -.java-version \ No newline at end of file +.java-version From d3eafef25abea784007d844dc794f76d48234a67 Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 22 May 2025 16:47:56 +0100 Subject: [PATCH 06/12] Update x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../retriever/PinnedRetrieverBuilderTests.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java b/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java index 3cd9a39fb19cb..28d7dd0c03bc4 100644 --- a/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java +++ b/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java @@ -211,8 +211,10 @@ public void testValidateSort() { multipleSortsSource.sort("field1"); builder.finalizeSourceBuilder(multipleSortsSource); assertThat(multipleSortsSource.sorts().size(), equalTo(2)); - assertThat(multipleSortsSource.sorts().get(0).toString(), equalTo("{\n \"_score\" : {\n \"order\" : \"desc\"\n }\n}")); - assertThat(multipleSortsSource.sorts().get(1).toString(), equalTo("{\n \"field1\" : {\n \"order\" : \"asc\"\n }\n}")); + assertThat(multipleSortsSource.sorts().get(0).getField(), equalTo("_score")); + assertThat(multipleSortsSource.sorts().get(0).getOrder(), equalTo(SortOrder.DESC)); + assertThat(multipleSortsSource.sorts().get(1).getField(), equalTo("field1")); + assertThat(multipleSortsSource.sorts().get(1).getOrder(), equalTo(SortOrder.ASC)); SearchSourceBuilder fieldFirstSource = new SearchSourceBuilder(); fieldFirstSource.query(dummyQuery); From 0a81ba5b6fb6e6851f92958f97abac2eb07eb21e Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 22 May 2025 16:48:03 +0100 Subject: [PATCH 07/12] Update x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../searchbusinessrules/retriever/PinnedRetrieverBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java index 0e254b4fcd5b3..ef61fc1cfa759 100644 --- a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java +++ b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java @@ -106,7 +106,7 @@ private void validateSort(SearchSourceBuilder source) { } SortBuilder sort = sorts.get(0); - if (sort instanceof ScoreSortBuilder == false) { + if (!(sort instanceof ScoreSortBuilder)) { throw new IllegalArgumentException( "[" + NAME + "] retriever only supports sorting by score, invalid sort criterion: " + sort.toString() ); From 87c067301846e279c705f18e4463593c6d00858f Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 22 May 2025 16:49:15 +0100 Subject: [PATCH 08/12] Update docs/changelog/128323.yaml --- docs/changelog/128323.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/128323.yaml diff --git a/docs/changelog/128323.yaml b/docs/changelog/128323.yaml new file mode 100644 index 0000000000000..b050826f3fd9a --- /dev/null +++ b/docs/changelog/128323.yaml @@ -0,0 +1,5 @@ +pr: 128323 +summary: "Fix: Allow non-score sorts in pinned retriever sub-retrievers" +area: Relevance +type: bug +issues: [] From 56dde7eef28beb2cc0f7c49cebba89e07a6acbad Mon Sep 17 00:00:00 2001 From: Mridula Date: Fri, 23 May 2025 17:13:35 +0100 Subject: [PATCH 09/12] Update docs/changelog/128323.yaml Co-authored-by: Kathleen DeRusso --- docs/changelog/128323.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog/128323.yaml b/docs/changelog/128323.yaml index b050826f3fd9a..b6114c26ddc6e 100644 --- a/docs/changelog/128323.yaml +++ b/docs/changelog/128323.yaml @@ -1,5 +1,5 @@ pr: 128323 -summary: "Fix: Allow non-score sorts in pinned retriever sub-retrievers" +summary: "Fix: Allow non-score secondary sorts in pinned retriever sub-retrievers" area: Relevance type: bug issues: [] From 3e191158174079d1a56890ddb398b11530be174a Mon Sep 17 00:00:00 2001 From: Mridula Date: Fri, 23 May 2025 17:22:39 +0100 Subject: [PATCH 10/12] Update PinnedRetrieverBuilder.java --- .../searchbusinessrules/retriever/PinnedRetrieverBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java index ef61fc1cfa759..0e254b4fcd5b3 100644 --- a/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java +++ b/x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java @@ -106,7 +106,7 @@ private void validateSort(SearchSourceBuilder source) { } SortBuilder sort = sorts.get(0); - if (!(sort instanceof ScoreSortBuilder)) { + if (sort instanceof ScoreSortBuilder == false) { throw new IllegalArgumentException( "[" + NAME + "] retriever only supports sorting by score, invalid sort criterion: " + sort.toString() ); From 22e9f9f130efa60231e21b4f0892cb4da99031a9 Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 29 May 2025 11:04:39 +0100 Subject: [PATCH 11/12] Fixed the PinnedRetrieverBuilderTests --- .../retriever/PinnedRetrieverBuilderTests.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java b/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java index 28d7dd0c03bc4..4f313d17ce708 100644 --- a/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java +++ b/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java @@ -25,6 +25,9 @@ import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.json.JsonXContent; import org.elasticsearch.xpack.searchbusinessrules.SpecifiedDocument; +import org.elasticsearch.search.sort.SortOrder; +import org.elasticsearch.search.sort.FieldSortBuilder; +import org.elasticsearch.search.sort.ScoreSortBuilder; import java.io.IOException; import java.util.List; @@ -210,11 +213,13 @@ public void testValidateSort() { multipleSortsSource.sort("_score"); multipleSortsSource.sort("field1"); builder.finalizeSourceBuilder(multipleSortsSource); + assertThat(multipleSortsSource.sorts().size(), equalTo(2)); - assertThat(multipleSortsSource.sorts().get(0).getField(), equalTo("_score")); - assertThat(multipleSortsSource.sorts().get(0).getOrder(), equalTo(SortOrder.DESC)); - assertThat(multipleSortsSource.sorts().get(1).getField(), equalTo("field1")); - assertThat(multipleSortsSource.sorts().get(1).getOrder(), equalTo(SortOrder.ASC)); + assertThat(multipleSortsSource.sorts().get(0), instanceOf(ScoreSortBuilder.class)); + assertThat(((ScoreSortBuilder) multipleSortsSource.sorts().get(0)).order(), equalTo(SortOrder.DESC)); + assertThat(multipleSortsSource.sorts().get(1), instanceOf(FieldSortBuilder.class)); + assertThat(((FieldSortBuilder) multipleSortsSource.sorts().get(1)).getFieldName(), equalTo("field1")); + assertThat(((FieldSortBuilder) multipleSortsSource.sorts().get(1)).order(), equalTo(SortOrder.ASC)); SearchSourceBuilder fieldFirstSource = new SearchSourceBuilder(); fieldFirstSource.query(dummyQuery); From 1bf998161e3cf29c335a483caa96299172b9847e Mon Sep 17 00:00:00 2001 From: Mridula Date: Thu, 29 May 2025 11:09:41 +0100 Subject: [PATCH 12/12] Spotless and checkstyle fix --- .../retriever/PinnedRetrieverBuilderTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java b/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java index 4f313d17ce708..0228a54320d8e 100644 --- a/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java +++ b/x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java @@ -16,6 +16,9 @@ import org.elasticsearch.search.retriever.RetrieverBuilder; import org.elasticsearch.search.retriever.RetrieverParserContext; import org.elasticsearch.search.retriever.TestRetrieverBuilder; +import org.elasticsearch.search.sort.FieldSortBuilder; +import org.elasticsearch.search.sort.ScoreSortBuilder; +import org.elasticsearch.search.sort.SortOrder; import org.elasticsearch.test.AbstractXContentTestCase; import org.elasticsearch.usage.SearchUsage; import org.elasticsearch.usage.SearchUsageHolder; @@ -25,9 +28,6 @@ import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.json.JsonXContent; import org.elasticsearch.xpack.searchbusinessrules.SpecifiedDocument; -import org.elasticsearch.search.sort.SortOrder; -import org.elasticsearch.search.sort.FieldSortBuilder; -import org.elasticsearch.search.sort.ScoreSortBuilder; import java.io.IOException; import java.util.List;