Update pagination_depth datatype from int to Integer #1094
Conversation
Signed-off-by: Varun Jain <varunudr@amazon.com>
|
@martin-gaievski and @heemin32 Please review the PR. Thanks |
src/main/java/org/opensearch/neuralsearch/query/HybridQuery.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/neuralsearch/query/HybridQueryBuilderTests.java
Show resolved
Hide resolved
| @@ -109,8 +109,8 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep | |||
| queryBuilder.toXContent(builder, params); | |||
| } | |||
| builder.endArray(); | |||
| if (isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery()) { | |||
There was a problem hiding this comment.
do we still need method isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery in MinClusterVersionUtil? Looks like you're using isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery == false and paginationDepth == null interchangeably
There was a problem hiding this comment.
So how this works is older versions of OS when there is no pagination_depth sent then we will not set that field in the builder object. But when the same method runs for version>=2.19 then in the fromXContent method we give default value 10 when we don't get pagination_depth by the user.
There was a problem hiding this comment.
in this case it's a combination of the version check and values being not null. Should you do OR of two? What I'm essentially trying to say - we are checking one condition in one place and another condition in another scenario, and it's not that great.
There was a problem hiding this comment.
The thing is we do not use isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery in doXContent of any other query builder. Every other place we use the same non null check. Recently expandNested param has been added by heemin in the neuralquerybuilder and it also follow the same non null check.
There was a problem hiding this comment.
I will address this in #1097
Signed-off-by: Varun Jain <varunudr@amazon.com>
| @@ -78,7 +78,7 @@ public HybridQueryBuilder(StreamInput in) throws IOException { | |||
| protected void doWriteTo(StreamOutput out) throws IOException { | |||
| writeQueries(out, queries); | |||
| if (isClusterOnOrAfterMinReqVersionForPaginationInHybridQuery()) { | |||
| out.writeInt(paginationDepth); | |||
| out.writeOptionalInt(paginationDepth); | |||
There was a problem hiding this comment.
I ran into a similar error as this with the StreamOutput on k-NN. Using out.getVersion() to see what version you're writing to might help avoid null checks elsewhere. StreamInput has a similar method too so you can set a default for the value.
There was a problem hiding this comment.
I will address this in #1097
|
We need to merge this PR for now to unblock merging on 2.x |
Signed-off-by: Varun Jain <varunudr@amazon.com>
bff857d
into
opensearch-project:main
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1094-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 bff857d6b2995f5a80d54a02af54be42fe90d5f3
# Push it to GitHub
git push --set-upstream origin backport/backport-1094-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.xThen, create a pull request where the |
…ect#1094) * Update pagination_depth datatype from int to Integer Signed-off-by: Varun Jain <varunudr@amazon.com>
* Pagination in Hybrid query (#1048) * Pagination in Hybrid query Signed-off-by: Varun Jain <varunudr@amazon.com> * Remove unwanted code Signed-off-by: Varun Jain <varunudr@amazon.com> * Adding hybrid query context dto Signed-off-by: Varun Jain <varunudr@amazon.com> * Adding javadoc in hybridquerycontext and addressing few comments from review Signed-off-by: Varun Jain <varunudr@amazon.com> * rename hybrid query extraction method Signed-off-by: Varun Jain <varunudr@amazon.com> * Refactoring to optimize extractHybridQuery method calls Signed-off-by: Varun Jain <varunudr@amazon.com> * Changes in tests to adapt with builder pattern in querybuilder Signed-off-by: Varun Jain <varunudr@amazon.com> * Add mapper service mock in tests Signed-off-by: Varun Jain <varunudr@amazon.com> * Fix error message of index.max_result_window setting Signed-off-by: Varun Jain <varunudr@amazon.com> * Fix error message of index.max_result_window setting Signed-off-by: Varun Jain <varunudr@amazon.com> * Fixing validation condition for lower bound Signed-off-by: Varun Jain <varunudr@amazon.com> * fix tests Signed-off-by: Varun Jain <varunudr@amazon.com> * Removing version check from doEquals and doHashCode method Signed-off-by: Varun Jain <varunudr@amazon.com> --------- Signed-off-by: Varun Jain <varunudr@amazon.com> * Update pagination_depth datatype from int to Integer (#1094) * Update pagination_depth datatype from int to Integer Signed-off-by: Varun Jain <varunudr@amazon.com> --------- Signed-off-by: Varun Jain <varunudr@amazon.com>
Description
This PR changes pagination_depth datatype from int to Integer. We are doing as the current code is breaking the bwc on 2.x by sending pagination_depth in the older versions of Opensearch.
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.