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

Update tests for backwards codecs #710

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

jmazanec15
Copy link
Member

Description

Updates tests for backwards codecs to prevent backwards codecs from writing with a read only codec.

Right now, testKnnVectorIndex requires that the Hnsw Lucene Format have the capability to write. For backwards codecs, however, they are not able to write (see https://github.com/apache/lucene/blob/main/lucene/backward-codecs/src/java/org/apache/lucene/backward_codecs/lucene94/Lucene94HnswVectorsFormat.java#L150).

To resolve this, we are removing the "testKnnVectorIndex" from backwards compatibility tests. For reading/writing backwards compatibility testing, we will rely on Lucene, where tests can be found in https://github.com/apache/lucene/tree/main/lucene/backward-codecs/src/test/org/apache/lucene/backward_codecs/lucene94.

It adds a test that checks that our custom format is correctly overriden.

Issues Resolved

https://build.ci.opensearch.org/job/distribution-build-opensearch/6804/

Check List

  • 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.

Updates tests for backwards codecs to prevent backwards codecs from
writing with a read only codec.

Signed-off-by: John Mazanec <[email protected]>
@jmazanec15 jmazanec15 added Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. backport 2.x v2.5.0 'Issues and PRs related to version v2.5.0' labels Jan 5, 2023
@jmazanec15 jmazanec15 requested a review from a team January 5, 2023 18:51
.build();

testKnnVectorIndex(knnCodecProvider, perFieldKnnVectorsFormatProvider);
assertTrue(codec.knnVectorsFormat() instanceof KNN940PerFieldKnnVectorsFormat);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use class of format class the is defined in the version enum, something like:

assertThat(codec.knnVectorsFormat(), isA(V_9_4_0.getPerFieldKnnVectorsFormat().getClass()));

Copy link
Member Author

@jmazanec15 jmazanec15 Jan 5, 2023

Choose a reason for hiding this comment

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

I believe that assertThat is deprecated

Copy link
Member

Choose a reason for hiding this comment

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

org.hamcrest.MatcherAssert.assertThat should be ok

.knnVectorsFormat(V_9_4_0.getPerFieldKnnVectorsFormat())
.build();

assertTrue(codec.knnVectorsFormat() instanceof KNN940PerFieldKnnVectorsFormat);
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above

navneet1v
navneet1v previously approved these changes Jan 5, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #710 (ed0a642) into main (6bbf086) will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main     #710      +/-   ##
============================================
+ Coverage     84.37%   84.39%   +0.02%     
- Complexity     1066     1067       +1     
============================================
  Files           151      151              
  Lines          4345     4345              
  Branches        389      389              
============================================
+ Hits           3666     3667       +1     
+ Misses          499      498       -1     
  Partials        180      180              
Impacted Files Coverage Δ
...ec/KNN940Codec/KNN940PerFieldKnnVectorsFormat.java 50.00% <0.00%> (-25.00%) ⬇️
...earch/knn/index/codec/KNN920Codec/KNN920Codec.java 90.90% <0.00%> (+9.09%) ⬆️
...earch/knn/index/codec/KNN910Codec/KNN910Codec.java 100.00% <0.00%> (+12.50%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: John Mazanec <[email protected]>
@jmazanec15 jmazanec15 merged commit 8b16831 into opensearch-project:main Jan 5, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

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-710-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8b168311d345228c30408bbb8078943b13477823
# Push it to GitHub
git push --set-upstream origin backport/backport-710-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-710-to-2.x.

naveentatikonda pushed a commit to naveentatikonda/k-NN that referenced this pull request Feb 2, 2023
Updates tests for backwards codecs to prevent backwards codecs from
writing with a read only codec.

Signed-off-by: John Mazanec <[email protected]>
@naveentatikonda naveentatikonda mentioned this pull request Feb 2, 2023
2 tasks
naveentatikonda added a commit that referenced this pull request Feb 3, 2023
* Update lucene94 package

Signed-off-by: Naveen Tatikonda <[email protected]>

* Add Lucene 9.5 codec and make it new default (#700)

* Add Lucene 9.5 codec and make it new default

Signed-off-by: Martin Gaievski <[email protected]>

* Update tests for backwards codecs (#710)

Updates tests for backwards codecs to prevent backwards codecs from
writing with a read only codec.

Signed-off-by: John Mazanec <[email protected]>

---------

Signed-off-by: Naveen Tatikonda <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
Co-authored-by: Martin Gaievski <[email protected]>
Co-authored-by: John Mazanec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. v2.5.0 'Issues and PRs related to version v2.5.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants