Skip to content
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

Make the HitQueue size more appropriate for exact search #1549

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

bugmakerrrrrr
Copy link
Contributor

Description

Currently, when performing KNN exact search, we consistently set the HitQueue size to k. However, there may be instances where the number of candidates is actually lower than k.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as per the DCO using --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.

Copy link

codecov bot commented Mar 16, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.90%. Comparing base (e8c9ced) to head (a1becdd).

Files Patch % Lines
...java/org/opensearch/knn/index/query/KNNWeight.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1549      +/-   ##
============================================
- Coverage     85.15%   84.90%   -0.26%     
+ Complexity     1371     1365       -6     
============================================
  Files           172      172              
  Lines          5566     5566              
  Branches        546      546              
============================================
- Hits           4740     4726      -14     
- Misses          596      607      +11     
- Partials        230      233       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@navneet1v
Copy link
Collaborator

navneet1v commented Mar 16, 2024

@bugmakerrrrrr does setting higher values of hitqueue creates any issue or this is more of an optimization?

Otherwise code looks good to me.

@bugmakerrrrrr
Copy link
Contributor Author

@bugmakerrrrrr does setting higher values of hitqueue creates any issue or this is more of an optimization?

just an optimization

navneet1v
navneet1v previously approved these changes Mar 17, 2024
jmazanec15
jmazanec15 previously approved these changes Mar 18, 2024
@navneet1v
Copy link
Collaborator

navneet1v commented Mar 18, 2024

@bugmakerrrrrr can you resolve the conflicts so that we can merge it to 2.13 The code freeze it today.

@jmazanec15 jmazanec15 dismissed stale reviews from navneet1v and themself via 32a477d March 18, 2024 21:14
@jmazanec15 jmazanec15 requested a review from navneet1v March 18, 2024 21:15
jmazanec15
jmazanec15 previously approved these changes Mar 18, 2024
@jmazanec15
Copy link
Member

@navneet1v I was able to resolve conflict - interestingly, I was able to add to @bugmakerrrrrr branch without issue, which seems somewhat risky: bugmakerrrrrr@32a477d.

@navneet1v
Copy link
Collaborator

Code freeze for 2.13 is already done. I can still see conflicts. Lets resolve this and merge the code. We can release this in 2.14.

@navneet1v
Copy link
Collaborator

@bugmakerrrrrr can you fix the GH actions so that we can merge the code. As there is no comments on the code,

@bugmakerrrrrr
Copy link
Contributor Author

@navneet1v it appears that the failure of GH actions is not connected to this PR

@navneet1v
Copy link
Collaborator

@navneet1v it appears that the failure of GH actions is not connected to this PR

Started a rerun..

@bugmakerrrrrr
Copy link
Contributor Author

@navneet1v can we merge this?

@navneet1v
Copy link
Collaborator

@jmazanec15 can you approve the PR and we can merge this PR then.

@navneet1v navneet1v merged commit c861966 into opensearch-project:main Mar 27, 2024
46 of 51 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 27, 2024
navneet1v pushed a commit that referenced this pull request Apr 3, 2024
Signed-off-by: panguixin <[email protected]>
(cherry picked from commit c861966)

Co-authored-by: panguixin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants