-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Fix: Allow non-score sorts in pinned retriever sub-retrievers #128323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Allow non-score sorts in pinned retriever sub-retrievers #128323
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR relaxes the pinned retriever's sort criteria by allowing additional non-score sort fields after ensuring that the first sort criterion remains score-based. Key changes include modifying the sort validation logic in PinnedRetrieverBuilder and updating the associated tests to confirm the behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java | Updated sort validation to only check that the first sort is score-based |
x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java | Adjusted tests to account for the new validated behavior and added assertions to verify sort order |
.../java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java
Show resolved
Hide resolved
…earch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java Co-authored-by: Copilot <[email protected]>
…earch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java Co-authored-by: Copilot <[email protected]>
Pinging @elastic/search-eng (Team:SearchOrg) |
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Hi @mridula-s109, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM.
This is a good bugfix to have, but note that this doesn't get at what we were hoping for - which was the ability to pin a single document at the top of an otherwise sorted list of results (think, pinning a result at the top of a result set that's otherwise sorted by publication date). Looking into it it's a larger issue to support that because it's a fundamental limitation of the pinned query DSL, which the retriever defers to. More advanced sub-sorting options would be a (non trivial) enhancement request CC: @serenachou
Co-authored-by: Kathleen DeRusso <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
…etrievers-sort-on-score
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the pinned retriever to only enforce score-based sorting on the first criterion, allowing additional custom sorts thereafter.
- Only the first sort is validated to be by
_score
- Test added to verify multiple-sort support
- Changelog entry for the bug fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java | Simplified validateSort to only check the first sort criterion |
x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java | Updated tests to allow custom secondary sorts and assert their presence |
docs/changelog/128323.yaml | Added changelog entry for non-score secondary sort support |
Comments suppressed due to low confidence (1)
x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java:223
- Add a test case where the first sort is the pinned-docs sort field (
ShardDocSortField.NAME
) followed by_score
to ensure thatvalidateSort
allows the injected shard-doc sort.
e = expectThrows(IllegalArgumentException.class, () -> builder.finalizeSourceBuilder(fieldFirstSource));
.../main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java
Show resolved
Hide resolved
💚 Backport successful
|
…c#128323) * Fix scoring and sort handling in pinned retriever * Remove books.es from version control and add to .gitignore * Remove books.es from version control and add to .gitignore * Remove books.es entries from .gitignore * fixed the mess * Update x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java Co-authored-by: Copilot <[email protected]> * Update x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
…c#128323) * Fix scoring and sort handling in pinned retriever * Remove books.es from version control and add to .gitignore * Remove books.es from version control and add to .gitignore * Remove books.es entries from .gitignore * fixed the mess * Update x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java Co-authored-by: Copilot <[email protected]> * Update x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
…c#128323) * Fix scoring and sort handling in pinned retriever * Remove books.es from version control and add to .gitignore * Remove books.es from version control and add to .gitignore * Remove books.es entries from .gitignore * fixed the mess * Update x-pack/plugin/search-business-rules/src/test/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilderTests.java Co-authored-by: Copilot <[email protected]> * Update x-pack/plugin/search-business-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/retriever/PinnedRetrieverBuilder.java Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
Summary
Fixes an issue where the pinned retriever was too restrictive with sort criteria in sub-retrievers, only allowing score-based sorting. This change allows custom sort criteria while maintaining the pinned document functionality.
Changes
validateSort
inPinnedRetrieverBuilder
to only check the first sort criterionTesting
Notes
Test Results
Tested with the following query:
Response: