Rework RRF to be evaluated during rewrite phase #112648
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rough_rewrite_phase
…rough_rewrite_phase
…rough_rewrite_phase
…:pmpailis/elasticsearch into rework_rrf_to_work_through_rewrite_phase
…rough_rewrite_phase
…csRetrieverBuilderTests.java Co-authored-by: Benjamin Trent <[email protected]>
… the newly introduced cluster feature
Merged all different yaml tests files in ff05d13 :) |
benwtrent
approved these changes
Sep 25, 2024
| int rank = 1; | ||
| for (ScoreDoc scoreDoc : rrfRankResult) { | ||
| final int findex = index; | ||
| final int frank = rank; |
x-pack/plugin/rank-rrf/src/test/java/org/elasticsearch/xpack/rank/rrf/RRFRankDocTests.java
Outdated
Show resolved
Hide resolved
.../rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/rrf/350_rrf_retriever_pagination.yml
Show resolved
Hide resolved
…uring different values in mutation tests
|
@elasticmachine update branch |
|
@elasticmachine update branch |
pmpailis
added
the
auto-backport
|
@benwtrent and @jimczi a huge thank you for reviewing and all support throughout ❤️ |
💚 Backport successful
|
pmpailis
added a commit
to pmpailis/elasticsearch
that referenced
this pull request
Oct 3, 2024
pmpailis
added a commit
that referenced
this pull request
Oct 3, 2024
matthewabbott
pushed a commit
to matthewabbott/elasticsearch
that referenced
this pull request
Oct 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR we introduce a different way to evaluate the
rrfretriever, by leveraging the query rewrite step at the beginning of query execution.The main points of this PR are:
CompoundRetrieverBuilderclass that defines a class of retrievers that operate on the top results of other retriever(s) and combine their results appropriately. All nested retrievers, after being rewritten themselves, are executed asynchronously throughmsearchand then the compound retriever is responsible for merging the top results from each sub-retriever through the abstractcombineInnerRetrieverResultsmethod.RankDocsRetrieverBuilderwhen extracting to source. The idea now is that we enhance this to also compute topDocs for the innerRetrievers when eitherprofileorexplainare present.profileandexplaincurrently require for the inner retrievers to be evaluated twice. This is something that we can build upon and improve maybe in subsequent PRs.RankDocsSortFieldto encoderankandscorefor each result (now namedRankDocsAndScoreSortField)The high level idea now is that all compound retrievers (including
rrf) will try to rewrite themselves into a "simpler"RankDocsRetrieverBuilderby rewriting and executing asynchronously all nested retrievers. This gives us the option to support any arbitrary nesting of retrievers, as well as more fine-grained search params likecollapse,highlight, and nested queries.If any of the subsearches fail, we fail the request and return a 5xx response.
Note: We still support running
rrfrank withsub_searchesbut this uses the old approach which is a bit different as no nesting is supported (explain/profile are also evaluated differently).