Preprocess search request in Result Transformers - #26
Conversation
…than the default 10. * Add preprocess request interface to ResultTransformer * Override from and size parameters durig preprocess, to fetch more hits from OpenSearch * Limit the number of documents sent to Kendra Ranking (Will make this number configurable in a follow up) * Populate response documents from re-ranked hits (upto Kendra limit) and OpenSearch hits beyond the limit Signed-off-by: Mahita Mahesh <mahitam@amazon.com>
Signed-off-by: Mahita Mahesh <mahitam@amazon.com>
| final SearchRequest searchRequest = (SearchRequest) request; | ||
| SearchRequest searchRequest = (SearchRequest) request; | ||
| // Make a copy of the search request that will not be modified by any transformer | ||
| SearchRequest originalSearchRequest = new SearchRequest(searchRequest.indices()); |
There was a problem hiding this comment.
This is just a SearchRequest with the same indices right? What about the other fields?
| // Source is returned in response hits by default. If disabled by the user, overwrite and enable | ||
| // in order to access document contents for reranking, then suppress at response time. | ||
| if (request.source().fetchSource() != null && !request.source().fetchSource().fetchSource()) { | ||
| originalSearchRequest.source().fetchSource(request.source().fetchSource()); |
There was a problem hiding this comment.
A little misleading to update originalSearchRequest inside this method. If originalSearchRequest is the original request, do we need this line?
There was a problem hiding this comment.
(Responding to 3 of your comments)
Good catch, this is is not a very clean solution, and I can probably name variables better. The limitation is that the SearchRequest class in OpenSearch doesn’t have a copy constructor (I found an issue requesting one, but work doesn’t seem to have been started yet). I spent 2-3 hours trying to create one, but it wasn’t straightforward.
Problem: If we preprocess the request (e.g. to override _source: false or the number of documents fetched), we need a mechanism to reverse this at response time.
Ideal Solution:
- Make a copy of the SearchRequest as it was received
- Preprocess to override params
- Run through OpenSearch dataflow with the request with modified params
- Post-process using the copy so that the customer response is as expect
Since we can’t make a true copy
- I’m creating a basic SearchRequest object with only indices set
- Whatever attributes I modify in
preprocessRequest, I only update the original values in the “originalRequest” - I didn’t want to name itsearchRequestCopybecause it’s not a true copy - open to other suggestions, though. - I use the “originalRequest” to do the post-processing
I thought this was better than passing individual variables, or a map or some sort. And given that the copy constructor issue seems to be getting some attention, I thought we could update the code sometime soon to use that.
Let me know if you have other suggestions.
There was a problem hiding this comment.
If we can't make a copy, then I think I prefer passing variables. Calling it a originalSearchRequest is super confusing and it seems like we only use a few fields in "originalSearchRequest" (fetchSource, from, and size). I think putting these three fields in a custom object will be better.
There was a problem hiding this comment.
What do you think of
void preprocessRequest(SearchRequest, ResultTransformerConfiguration)
and an OriginalRequestFields object that's initialized from the SearchRequest in the SearchActionFilter apply method. I'll add comments stating that we can remove that once we have a copy constructor.
| originalSearchRequest.source().from(from); | ||
| originalSearchRequest.source().size(size); |
There was a problem hiding this comment.
Just for my understanding, why do need to manually set this if originalSearchRequest is the original request?
| String titleFieldName = queryParserResult.getTitleFieldName(); | ||
| if (docSourceMap.get(bodyFieldName) == null) { | ||
| String errorMessage = String.format(Locale.ENGLISH, | ||
| "Kendra Intelligent Ranking cannot be applied when documents are missing %s [%s]. Document ID [].", |
There was a problem hiding this comment.
Missing the third %s
| import org.opensearch.index.query.MultiMatchQueryBuilder; | ||
| import org.opensearch.index.query.QueryBuilder; | ||
| import org.opensearch.search.relevance.transformer.kendraintelligentranking.configuration.Constants; | ||
| import org.opensearch.search.relevance.transformer.kendraintelligentranking.model.KendraIntelligentRankingException; |
Signed-off-by: Mahita Mahesh <mahitam@amazon.com>
|
@mahitamahesh - Are you planning to add unit tests? How unit testable is this change? |
|
Closing out this PR since the changes were added as part of #32 |
3 participants
Description
preprocessRequestinterface to ResultTransformer.Issues Resolved
N/A
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.