Move engine and lib components into separate files #438
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
Refactors library and engine structure to be less coupled. Pulls classes and interfaces out into individual files. Additionally, renames package from util to engine. Signed-off-by: John Mazanec <jmazane@amazon.com>
Removes complex versioning related to the engines. Simplifies it to a single string. Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Codecov Report
@@ Coverage Diff @@
## main #438 +/- ##
============================================
- Coverage 84.02% 83.79% -0.24%
- Complexity 911 934 +23
============================================
Files 130 133 +3
Lines 3880 3831 -49
Branches 359 349 -10
============================================
- Hits 3260 3210 -50
- Misses 458 459 +1
Partials 162 162
Continue to review full report at Codecov.
|
VijayanB
reviewed
Jul 11, 2022
VijayanB
reviewed
Jul 11, 2022
VijayanB
reviewed
Jul 11, 2022
| class Nmslib extends NativeLibrary { | ||
|
|
||
| final static String HNSW_LIB_NAME = "hnsw"; | ||
| final static String EXTENSION = ".hnsw"; |
There was a problem hiding this comment.
Suggested change
| final static String EXTENSION = ".hnsw"; | |
| final static String HNSW_LIB_EXTENSION = ".hnsw"; |
There was a problem hiding this comment.
So this is a little bit confusing. HNSW_LIB_NAME is redundant with METHOD_HNSW, so I will delete that.
EXTENSION is actually the libraries extension. Given that we are in the Nmslib class, I think it can be kept as EXTENSION. Ill add a comment.
navneet1v
reviewed
Jul 11, 2022
vamshin
reviewed
Jul 11, 2022
Signed-off-by: John Mazanec <jmazane@amazon.com>
jmazanec15
added
2.2.0
Refactoring
jmazanec15
requested review from
navneet1v,
vamshin,
martin-gaievski and
VijayanB
Signed-off-by: John Mazanec <jmazane@amazon.com>
navneet1v
approved these changes
Jul 13, 2022
VijayanB
approved these changes
Jul 13, 2022
| */ | ||
| @SuppressWarnings("unchecked") | ||
| MethodAsMapBuilder addParameter(String parameterName, String prefix, String suffix) { | ||
| indexDescription += prefix; |
There was a problem hiding this comment.
nit: maybe we can refactor in one of next PRs to something more efficient like StringBuilder. Seems we're accumulating multiple small strings and concatenate all of them into one final string object.
There was a problem hiding this comment.
That makes sense. Will take for future PR.
martin-gaievski
approved these changes
Jul 13, 2022
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Jul 13, 2022
Refactors library and engine structure to be less coupled. Pulls classes and interfaces out into individual files. Cleans up implementation. Signed-off-by: John Mazanec <jmazane@amazon.com> (cherry picked from commit fe9f293)
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
Refactors current implementation of the KNNEngine and KNNLIbrary abstractions. It pulls out the different components into their own files so that they are more readable and easier to modify.
Additionally, refactors lib versioning logic. Right now, we have two methods related to versioning: getLatestBuildVersion and getLatestLibVersion. The build version is used to build the filename for the engine. However, the getLatestLibVersion is not used. So, this PR simplifies it to just maintain the current lib version as a string.
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.