Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

k-NN plugin support for Elasticsearch version 7.10.0 #271

Merged
merged 5 commits into from
Nov 25, 2020
Merged

k-NN plugin support for Elasticsearch version 7.10.0 #271

merged 5 commits into from
Nov 25, 2020

Conversation

vamshin
Copy link
Member

@vamshin vamshin commented Nov 25, 2020

Issue #, if available:
#270

Description of changes:
k-NN plugin support for Elasticsearch version 7.10.0

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Nov 25, 2020 •
edited
Loading

Codecov Report

Merging #271 (6442253) into master (8a64b49) will decrease coverage by 0.32%.
The diff coverage is 88.23%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #271      +/-   ##
============================================
- Coverage     78.72%   78.39%   -0.33%     
- Complexity      338      342       +4     
============================================
  Files            53       54       +1     
  Lines          1335     1361      +26     
  Branches        121      122       +1     
============================================
+ Hits           1051     1067      +16     
- Misses          235      243       +8     
- Partials         49       51       +2
Impacted Files Coverage Δ Complexity Δ
...relasticsearch/knn/index/KNNVectorFieldMapper.java 78.78% <50.00%> (+0.16%) 16.00 <0.00> (+1.00)
...csearch/knn/index/codec/KNN87Codec/KNN87Codec.java 96.00% <96.00%> (ø) 15.00 <15.00> (?)
...roforelasticsearch/knn/plugin/KNNCodecService.java 75.00% <100.00%> (ø) 3.00 <2.00> (ø)
...oforelasticsearch/knn/plugin/KNNEngineFactory.java 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
...csearch/knn/index/codec/KNN86Codec/KNN86Codec.java 28.00% <0.00%> (-68.00%) 2.00% <0.00%> (-13.00%)
...troforelasticsearch/knn/index/KNNQueryBuilder.java 66.66% <0.00%> (+9.19%) 17.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a64b49...6442253. Read the comment docs.

VijayanB
VijayanB reviewed Nov 25, 2020
View reviewed changes
src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorFieldMapper.java Outdated
int value = XContentMapValues.nodeIntegerValue(o);
private final Parameter<Integer> dimension = new Parameter<>(KNNConstants.DIMENSION, false, () -> -1,
(n, c, o) -> {
int value = (o == null ? null : XContentMapValues.nodeIntegerValue(o));
Copy link
Member

Choose a reason for hiding this comment

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

if o is null, then i don't think we can assign null to value since it is primitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

VijayanB
VijayanB reviewed Nov 25, 2020
View reviewed changes
src/main/java/com/amazon/opendistroforelasticsearch/knn/index/codec/KNN87Codec/KNN87Codec.java Outdated
@@ -0,0 +1,138 @@
/*
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Copyright 2020

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thanks

jmazanec15
jmazanec15 reviewed Nov 25, 2020
View reviewed changes
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

Just to confirm, this PR is not meant to upgrade ODFE to 1.12, correct? That will be done in a different PR?

gradle.properties
@@ -13,4 +13,4 @@
# permissions and limitations under the License.
#

version=1.6
version=1.11.0
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this variable is not in use. It was pointing to older version. So i updated to latest version. This variable could be used in build.gradle file.

Copy link
Member

Choose a reason for hiding this comment

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

I see makes sense.

@vamshin
Copy link
Member Author

vamshin commented Nov 25, 2020

Just to confirm, this PR is not meant to upgrade ODFE to 1.12, correct? That will be done in a different PR?

right. This was to upgrade to 7.10.

VijayanB
VijayanB approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@VijayanB VijayanB left a comment

Choose a reason for hiding this comment

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

LGTM

@vamshin vamshin merged commit c517644 into opendistro-for-elasticsearch:master Nov 25, 2020
@vamshin vamshin added the Enhancements Improvement on existing component label Nov 26, 2020
@jmazanec15 jmazanec15 added Maintenance A change to add support for new versions from upstream and removed Enhancements Improvement on existing component labels Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jmazanec15 jmazanec15 jmazanec15 approved these changes

@VijayanB VijayanB VijayanB approved these changes

Assignees
No one assigned
Labels
Maintenance A change to add support for new versions from upstream
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vamshin @VijayanB @jmazanec15