Pagination in Hybrid query #1048
Conversation
Signed-off-by: Varun Jain <[email protected]>
Signed-off-by: Varun Jain <[email protected]>
vibrantvarun
requested review from
heemin32,
navneet1v,
VijayanB,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski,
sean-zheng-amazon,
model-collapse,
zane-neo,
zhichao-aws,
yuye-aws and
minalsha
as code owners
|
Integ test and BWC tests are failing due to opensearch-project/ml-commons#3321 |
| // get fetched hit content by doc_id | ||
| SearchHit searchHit = docIdToSearchHit.get(scoreDoc.doc); | ||
| // update score to normalized/combined value (3) | ||
| searchHit.score(scoreDoc.score); | ||
| return searchHit; | ||
| }).toArray(SearchHit[]::new); | ||
| updatedSearchHitArray[i - fromValueForSingleShard] = searchHit; |
There was a problem hiding this comment.
we need to check if i - fromValueForSingleShard is a valid index, within [0, array.length- 1] boundaries
There was a problem hiding this comment.
Well, if you look at the code it is just putting values in the fresh array updateSearchHitArray. i starts from fromValueForSingleShard. So i-fromValueForSingleShard will be 0. i can maximum go to array.length- 1 so it has already been taken care off. Do you still want me to put extra assert condition here?
There was a problem hiding this comment.
Understood, my intention is that we have array index iterator that goes from 0 to the length -1, we can adjust the logic for the for loop temp variable, something like this:
for (int i = 0; i < Math.min(topDocs.scoreDocs.length - topDocs.scoreDocs.length, updatedSearchHitArray.length); i++) {
ScoreDoc scoreDoc = topDocs.scoreDocs[i + fromValueForSingleShard];
....
updatedSearchHitArray[i] = searchHit;
src/main/java/org/opensearch/neuralsearch/query/HybridQuery.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/search/query/HybridCollectorManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/search/query/HybridCollectorManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/query/HybridQuery.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Varun Jain <[email protected]>
| @AllArgsConstructor | ||
| @Builder | ||
| @Getter | ||
| public class HybridQueryContext { |
There was a problem hiding this comment.
please add class level java doc
| /** | ||
| * Extract hybrid query from generic query object retrieved from search context | ||
| */ | ||
| private static HybridQuery extractHybridQueryFromAbstractQuery(Query query) { |
There was a problem hiding this comment.
| private static HybridQuery extractHybridQueryFromAbstractQuery(Query query) { | |
| /** | |
| * Unwraps a HybridQuery from either a direct query or a nested BooleanQuery | |
| */ | |
| private static HybridQuery unwrapHybridQuery(Query query) { |
| import lombok.Builder; | ||
| import lombok.Getter; | ||
|
|
||
| @AllArgsConstructor |
There was a problem hiding this comment.
We need to use either constructor or builder, but not both. You need to either remove constructor annotation or make it private.
To enforce setting of required pagination param use NonNull annotation
Description
This PR contains changes for enabling support for pagination in hybrid query.
The highlight of this PR are
Related Issues
#280
Check List
--signoff.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.