Minor refactoring and refactored some unit test #2167
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
VijayanB
requested review from
heemin32,
navneet1v,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski,
ryanbogan and
luyuncheng
as code owners
VijayanB
force-pushed
the
update-unit-test
branch
from
06480d8 to
e4a8e37
Compare
VijayanB
changed the title
VijayanB
force-pushed
the
update-unit-test
branch
from
e4a8e37 to
56a27f8
Compare
jmazanec15
reviewed
Sep 30, 2024
CHANGELOG.md
Outdated
| @@ -31,3 +31,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
| ### Maintenance | |||
| * Remove benchmarks folder from k-NN repo [#2127](https://github.com/opensearch-project/k-NN/pull/2127) | |||
| ### Refactoring | |||
| * Update unit test to include field with no live docs and minor refactor [#2167](https://github.com/opensearch-project/k-NN/pull/2167) | |||
There was a problem hiding this comment.
I would reverse this to describe the minor refact and then say "and refactor some tests"
shatejas
approved these changes
Sep 30, 2024
| verify(knn990QuantWriterMockedConstruction.constructed().get(0)).writeState(i, quantizationState); | ||
| verify(nativeIndexWriter).flushIndex(expectedVectorValues.get(i), vectorsPerField.get(i).size()); | ||
| if (vectorsPerField.get(i).isEmpty()) { | ||
| verify(knn990QuantWriterMockedConstruction.constructed().get(0), never()).writeState(i, quantizationState); |
There was a problem hiding this comment.
We don't want any interactions with quantwriter in this case right?
Suggested change
| verify(knn990QuantWriterMockedConstruction.constructed().get(0), never()).writeState(i, quantizationState); | |
| verifyNoInteractions(knn990QuantWriterMockedConstruction); |
| verify(nativeIndexWriter).flushIndex(expectedVectorValues.get(i), vectorsPerField.get(i).size()); | ||
| if (vectorsPerField.get(i).isEmpty()) { | ||
| verify(knn990QuantWriterMockedConstruction.constructed().get(0), never()).writeState(i, quantizationState); | ||
| verify(nativeIndexWriter, never()).flushIndex(expectedVectorValues.get(i), vectorsPerField.get(i).size()); |
There was a problem hiding this comment.
same here, verifying no interactions is a much stronger check
Suggested change
| verify(nativeIndexWriter, never()).flushIndex(expectedVectorValues.get(i), vectorsPerField.get(i).size()); | |
| verifyNoInteractions(nativeIndexWriter); |
There was a problem hiding this comment.
I am not sure will that work. We have interactions from other fields. I only want to verify that it doesn't interact when there are empty fields.
There was a problem hiding this comment.
Makes sense, I missed that it has other values apart from empty vector
VijayanB
changed the title
Refactored if/else to reduce nesting. Added unit test when one of the field doesn't have live docs. Signed-off-by: Vijayan Balasubramanian <[email protected]>
VijayanB
force-pushed
the
update-unit-test
branch
from
a5c6bc3 to
ee6e7ee
Compare
|
CI failure is not related to this PR. Created GH issue for flaky test #2169 |
junqiu-lei
approved these changes
Sep 30, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Sep 30, 2024
Refactored if/else to reduce nesting. Added unit test when one of the field doesn't have live docs. Signed-off-by: Vijayan Balasubramanian <[email protected]> (cherry picked from commit f16f225)
VijayanB
added a commit
that referenced
this pull request
Oct 1, 2024
…) (#2170) Refactored if/else to reduce nesting. Added unit test when one of the field doesn't have live docs. Signed-off-by: Vijayan Balasubramanian <[email protected]> (cherry picked from commit f16f225) Co-authored-by: Vijayan Balasubramanian <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
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.