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

Add builder constructor that takes algo params #289

Merged

Conversation

jmazanec15
Copy link
Member

Issue #, if available:
#288

Description of changes:
In this PR, I added a constructor for KNNVectorFieldMapper.Builder that takes the algorithm parameters that it typically reads from settings. This constructor is used during merging of mappers. It allows an existing KNNVectorFieldMapper to pass settings to a KNNVectorFieldMapper.Builder for merge. It plays a similar role to the ParametrizedFieldMapper.init method, which copies mapping parameters from an already existing FieldMapper to the FieldMapper.Builder that will be used for merging. I decided to not override this method because it is more focused on mapping parameters, but I am open to feedback on this.

Verified that with this change, I am unable to reproduce the issue following the steps in #288.

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

@jmazanec15 jmazanec15 added the Bug Fixes Change that fixes a bug label Dec 29, 2020
@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #289 (72f247d) into master (c41f375) will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #289      +/-   ##
============================================
+ Coverage     80.38%   80.51%   +0.13%     
  Complexity      388      388              
============================================
  Files            62       62              
  Lines          1458     1468      +10     
  Branches        127      130       +3     
============================================
+ Hits           1172     1182      +10     
  Misses          239      239              
  Partials         47       47              
Impacted Files Coverage Δ Complexity Δ
...relasticsearch/knn/index/KNNVectorFieldMapper.java 80.68% <100.00%> (+1.43%) 16.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 c41f375...72f247d. Read the comment docs.

Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

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

Good catch. LGTM!

@VijayanB
Copy link
Member

Is it possible to add integration test or unit test for this scenario? May be as another commit since this is critical.

@jmazanec15
Copy link
Member Author

Is it possible to add integration test or unit test for this scenario? May be as another commit since this is critical.

Yes, I think we need to add more end to end testing in our integration tests. We can take that as another task.

@jmazanec15 jmazanec15 merged commit aa5d1d4 into opendistro-for-elasticsearch:master Dec 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fixes Change that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants