-
Notifications
You must be signed in to change notification settings - Fork 113
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
Adding KNN codec that is based on Lucene92 codec #444
Adding KNN codec that is based on Lucene92 codec #444
Conversation
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
30964a9
to
86c32b6
Compare
Codecov Report
@@ Coverage Diff @@
## main #444 +/- ##
============================================
- Coverage 83.97% 83.82% -0.15%
- Complexity 902 933 +31
============================================
Files 131 133 +2
Lines 3863 3828 -35
Branches 359 349 -10
============================================
- Hits 3244 3209 -35
Misses 457 457
Partials 162 162
Continue to review full report at Codecov.
|
@@ -8,6 +8,7 @@ | |||
import org.apache.lucene.codecs.Codec; | |||
import org.opensearch.index.mapper.MapperService; | |||
import org.opensearch.knn.index.codec.KNN910Codec.KNN910Codec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to keep 910 codec around in this builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each codec has it's own builder as an inner class in CodecBuilder, as those builders are short I think it makes sense to have them inside the abstract class at least for now.
Those builders are used by codec factory, it should be able to build any codec from set of supported versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After taking a second thought it may be a good point, customer can build codec directly using no arg constructor as having such constructor is a requirement from core OS. In such case the only place that uses/requires codec factory is our own code, and we can simplify factory logic and return only the latest version of codec. I've pushed commit for this, @jmazanec15 please check if that is something you had in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
src/main/java/org/opensearch/knn/index/codec/KNN920Codec/KNN920Codec.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks
* Adding KNN codec that is based on Lucene92 codec Signed-off-by: Martin Gaievski <gaievski@amazon.com> (cherry picked from commit 7499375)
Signed-off-by: Martin Gaievski gaievski@amazon.com
Description
Adding new KNN codec that is based on Lucene92 and making it new default.
This is required because 1. we follow core OpenSearch 2.x that is using Lucene92 and 2. hnsw implementation improved comparing to 9.1, it is required if we want to open Lucene hnsw through plugin.
As part of this change I've done a code cleanup and simplified the codec factory. We need some simple mechanism to get only the latest version of KNN codec, and we don't need to support creation of previous KNN codec versions
Check List
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.