-
Notifications
You must be signed in to change notification settings - Fork 114
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
Refactor knn type and codecs #439
Refactor knn type and codecs #439
Conversation
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
5e7cfff
to
053bd6e
Compare
Codecov Report
@@ Coverage Diff @@
## main #439 +/- ##
============================================
- Coverage 84.02% 83.97% -0.05%
+ Complexity 911 902 -9
============================================
Files 130 131 +1
Lines 3880 3863 -17
Branches 359 359
============================================
- Hits 3260 3244 -16
+ Misses 458 457 -1
Partials 162 162
Continue to review full report at Codecov.
|
@Getter private final MethodComponent methodComponent; | ||
@Getter private final Set<SpaceType> spaces; |
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.
move this on the top of the class
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.
ack
@Getter | ||
private final KNNEngine knnEngine; | ||
@Getter | ||
private final SpaceType spaceType; | ||
@Getter |
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.
same as above.
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.
ack
private final MapperService mapperService; | ||
|
||
@Override | ||
public Codec build() { | ||
return new KNN910Codec(userCodec); | ||
} |
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.
not sure what is the use of the parameter MapperService here? Are we expecting when the dense_vector field we come we will use the mapper service to find out what exact codec to use?
Please improve the documentation with what we want achieve here.
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.
In this PR I'm making it available for codec and codec factory. In next PR I'm going to change the codec and extend knnVector format through the PerFieldMappingPostingFormatCodec mechanism, similar to what we did for core in dense_vector PR
https://github.com/opensearch-project/OpenSearch/pull/3659/files#r915126068. Let me update the description to reflect exact purpose of mapping service param
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.
updated PR description
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.
Just make sure that you are providing enough documentation and reason behind the changes in the Java doc.
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.
ack
5487268
to
34119de
Compare
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
34119de
to
4d4e813
Compare
int dimension; | ||
String modelId; | ||
KNNMethodContext knnMethodContext; |
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.
Will changing this impact BWC?
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.
It shouldn't, the only case I can think of is if do older clusters after migration this field is null. I can add extra checks in next PR when we add lucene engine. Can we run any specific bwc test to make sure there are no side-effects?
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.
I dont think it will be a problem. When mapping reaches new node, the parser will parse it. Since we arent changing the parser, it should be okay. The MappedFieldType is not serialized, so it will be okay.
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.
yes, exactly
* Refactor knn type and codecs Signed-off-by: Martin Gaievski <gaievski@amazon.com>
The backport to
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-439-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2fc09ba7a7fd8f2cf38678ee38058530994a8b12
# Push it to GitHub
git push --set-upstream origin backport/backport-439-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 |
Signed-off-by: Martin Gaievski gaievski@amazon.com
Description
Refactor few things in plugin in order to open possibilities for future extensions required for #39
Issues Resolved
building block for #39
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.