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

Add ignore_unmapped support in KNNQueryBuilder #1071

Merged
merged 16 commits into from
Sep 29, 2023

Conversation

ryanbogan
Copy link
Member

@ryanbogan ryanbogan commented Aug 29, 2023

Description

Add ignore_unmapped support in KNNQueryBuilder

Issues Resolved

#1018

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.

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #1071 (c3211e8) into main (ca5e483) will decrease coverage by 0.06%.
The diff coverage is 75.86%.

@@             Coverage Diff              @@
##               main    #1071      +/-   ##
============================================
- Coverage     85.01%   84.96%   -0.06%     
- Complexity     1184     1192       +8     
============================================
  Files           159      159              
  Lines          4813     4842      +29     
  Branches        433      440       +7     
============================================
+ Hits           4092     4114      +22     
- Misses          525      530       +5     
- Partials        196      198       +2     
Files Coverage Δ
.../main/java/org/opensearch/knn/index/IndexUtil.java 57.97% <75.00%> (+2.23%) ⬆️
...rg/opensearch/knn/index/query/KNNQueryBuilder.java 83.23% <76.19%> (-0.98%) ⬇️

@ryanbogan
Copy link
Member Author

This isn't quite ready for review yet, I still need to add tests

@martin-gaievski
Copy link
Member

This isn't quite ready for review yet, I still need to add tests

you can create PR as "draft" so CI will be triggered, and when you're done with coding just flip status to "Ready for review"

@ryanbogan
Copy link
Member Author

This isn't quite ready for review yet, I still need to add tests

you can create PR as "draft" so CI will be triggered, and when you're done with coding just flip status to "Ready for review"

I had it as draft, then accidentally clicked on ready for review before I remembered about adding tests.

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
@ryanbogan
Copy link
Member Author

Ready for review now

@navneet1v
Copy link
Collaborator

@ryanbogan before review start can you see why the github WFs are failings. and fix them

@ryanbogan
Copy link
Member Author

@ryanbogan before review start can you see why the github WFs are failings. and fix them

@navneet1v The Linux failure is for the test org.opensearch.knn.index.KNNCircuitBreakerIT.testCbUntrips, and the same test/seed is passing on my local. Could it potentially be a flaky test?

For the others, I was informed during standup that the tests with the security plugin enabled are currently broken.

@navneet1v
Copy link
Collaborator

navneet1v commented Aug 30, 2023

@ryanbogan before review start can you see why the github WFs are failings. and fix them

@navneet1v The Linux failure is for the test org.opensearch.knn.index.KNNCircuitBreakerIT.testCbUntrips, and the same test/seed is passing on my local. Could it potentially be a flaky test?

For the others, I was informed during standup that the tests with the security plugin enabled are currently broken.

Can we rerun the workflows. Secure cluster I know is broken. But lets fix the changelog step, by adding this change in CHANGELOG.md file

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have doubt about this change being backward compatible in its current form, please make sure BWC tests are passing

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
navneet1v
navneet1v previously approved these changes Sep 27, 2023
Copy link
Collaborator

@navneet1v navneet1v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment overall code looks good.

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@martin-gaievski martin-gaievski merged commit 5dd9780 into opensearch-project:main Sep 29, 2023
35 of 42 checks passed
@ryanbogan ryanbogan deleted the ignore_unmapped branch September 29, 2023 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants