Support k-NN similarity functions in painless scripting #281

VijayanB · 2020-12-11T23:59:34Z

*Issue #213

Implement IndexFieldDataBuilder to allow painless scripting to indenitfy KNNScriptDocValues
Add dependency to PainlessExtension to add custom scoring methods to be exposed for usage by users inside painless scripting.

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

Painless scripting uses IndexFieldDataBuilder to load field data values. Hence, override fielddataBuilder to return instance of KNNVectorIndexFieldData.Builder. Subsequently, this builder is responsible for returning ScriptDocValues which will be used as argument in our Whitlisting knn custome methods.

Added new methods which will be whitelisted for users to call in source field for painless scripting.

Add Extension class and return list of whitelisted method. Update gradle to add dependencies to painless.

VijayanB · 2020-12-12T00:04:28Z

Following methods are whitelisted:
l2Squared
cosineSimilarity
cosineSimilarityOptimized

@vamshin @jmazanec15 we can discuss here if we want to rename the method name for better reach

codecov · 2020-12-12T00:08:16Z

Codecov Report

Merging #281 (44b2ace) into master (8c802d9) will increase coverage by 1.11%.
The diff coverage is 91.13%.

@@             Coverage Diff              @@
##             master     #281      +/-   ##
============================================
+ Coverage     79.27%   80.38%   +1.11%     
- Complexity      359      388      +29     
============================================
  Files            58       62       +4     
  Lines          1404     1458      +54     
  Branches        126      127       +1     
============================================
+ Hits           1113     1172      +59     
+ Misses          243      239       -4     
+ Partials         48       47       -1

Impacted Files	Coverage Δ	Complexity Δ
...sticsearch/knn/index/KNNVectorDVLeafFieldData.java	`72.72% <72.72%> (ø)`	`4.00 <4.00> (?)`
...sticsearch/knn/index/KNNVectorScriptDocValues.java	`81.81% <81.81%> (ø)`	`8.00 <8.00> (?)`
...relasticsearch/knn/index/KNNVectorFieldMapper.java	`79.25% <100.00%> (+0.31%)`	`16.00 <0.00> (ø)`
...asticsearch/knn/index/KNNVectorIndexFieldData.java	`100.00% <100.00%> (ø)`	`7.00 <7.00> (?)`
...lasticsearch/knn/plugin/script/KNNScoreScript.java	`100.00% <100.00%> (+25.58%)`	`1.00 <0.00> (ø)`
...lasticsearch/knn/plugin/script/KNNScoringUtil.java	`98.03% <100.00%> (+1.26%)`	`18.00 <15.00> (+7.00)`
...earch/knn/plugin/script/KNNWhitelistExtension.java	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
... and 2 more

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 8c802d9...44b2ace. Read the comment docs.

test annotation is forbidden

jmazanec15

A few comments. Still need to dig into the test cases more. One note, once #222 is resolved, we will need to add a section on this.

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorFieldMapper.java

jmazanec15 · 2020-12-14T17:59:54Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java

@@ -50,7 +90,7 @@ public static float l2Squared(float[] queryVector, float[] inputVector) {
     * @return cosine score
     */
    public static float cosinesimilOptimized(float[] queryVector, float[] inputVector, float normQueryVector) {
-        float dotProduct = 0.0f;
+            float dotProduct = 0.0f;


Why is this indented?

ACk. Will apply reformat code in next commit.

src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java

jmazanec15 · 2020-12-14T18:18:33Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java

+     * @param normQueryVector normalized query vector value.
+     * @return cosine score
+     */
+    public static float cosineSimilarityOptimized(


I think this interface is vague: cosineSimilarityOptimized does not give a user any information about how this function differs from cosineSimilarity. If cosineSimilarity can be optimized, why even provide cosineSimilarity?

The Cosine similarity is A . B / (||A|| x ||B||). The difference between the two functions is that cosineSimilarityOptimized has the user pass in both the query vector and the magnitude of the query vector. Assuming the query vector is A, ||A|| does not change throughout the query. So time is saved by computing ||A|| separately and then passing it into the function. Also, I think normQueryVector may not be the appropriate term. Normalization refers to the process of making a vector have a magnitude of one.

My proposal is to switch it to the following interface:

cosineSimilarity(List <Number> queryVector, KNNVectorScriptDocValues docValues, Number queryVectorMagnitude)

src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorScriptDocValues.java

...test/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorDVLeafFieldDataTests.java

src/test/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorIndexFieldDataTests.java

...test/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorScriptDocValuesTests.java

1. Made KNNScriptDocValues thread safe 2. reformat code 3. rename cosinesimiloptimized.

Add painless scripting score to verify whether following methods are available for users. 1. l2Squared 2. cosineSimilarity(queryVector,doc[field]) 3. cosineSimilarity(queryVector,doc[field],normalizedVector)

jmazanec15

I think test cases look pretty good. I added a few additional comments in implementation.

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorFieldMapper.java

jmazanec15 · 2020-12-16T17:26:28Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorScriptDocValues.java

+
+    public synchronized float[] getValue() throws IOException {
+        if (!docExists) {
+            throw new IllegalArgumentException("no value found for the corresponding doc ID");


Why throw IllegalArgumentException? It seems like it maybe should be an IllegalStateException.

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorScriptDocValues.java

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorDVLeafFieldData.java

jmazanec15 · 2020-12-16T17:46:16Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java

+        float[] knnDocVector;
+        try {
+            knnDocVector = docValues.getValue();
+        } catch (Exception e) {


Should we catch the specific exception here?

getValue can throw RuntimeException & IOException, i see three option here

Let docValues throw exception as it is and catch all individual exception here and return Float.Min like now,

Update getValue to throw only RuntimeException, and catch only RuntimeException here

Just update catch (Exception e) to catch ( RuntimeException | IOException e )

i don't see any difference in all three since at the end outcome is same, but if you have strong opinion on one vs other, i can change it . if you see any other option i can make change it as well.

Makes sense, Option 1 us good with me

src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java

src/test/java/com/amazon/opendistroforelasticsearch/knn/KNNRestTestCase.java

1. updated exception type. 2. Added test

jmazanec15

All looks good to me!

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorScriptDocValues.java

jmazanec15 · 2020-12-17T00:19:12Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/KNNScoringUtil.java

+        float[] knnDocVector;
+        try {
+            knnDocVector = docValues.getValue();
+        } catch (Exception e) {


Makes sense, Option 1 us good with me

vamshin · 2020-12-17T05:49:27Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorScriptDocValues.java

+
+    @Override
+    public void setNextDocId(int docId) throws IOException {
+        synchronized (this) {


do we need synchronized here?

I added thread safe to prevent any race condition based on feedback, since docExists setting is happening in one place and reading is happening on another place. If there is not threat with race condition then definitely we can remove this. At the end, it comes to do we need to make this instance thread safe or not? What do you think?

Good point. Do you have pointers to any other base classes extending ScriptDocValues<> doing synchronization? My understanding is each search request should have its own instance and documents are iterated sequentially so synchronization should not be required. If you see any base class doing this we can definitely consider.

Removed synchronization.

vamshin · 2020-12-17T06:02:37Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNVectorScriptDocValues.java

+
+    @Override
+    public int size() {
+        synchronized (this) {


Do we need synchronized here? Avoid synchronization in the places not needed as it could hamper performance.

I added so that docExists is synchronized. Since it is just one instruction, i think i can remove this.

Yes please. I have not seen other places doing this. We can remove this.

vamshin · 2020-12-17T06:34:30Z

...st/java/com/amazon/opendistroforelasticsearch/knn/plugin/script/PainlessScriptScoringIT.java

+
+    private Map<String, Float[]> getL2TestData() {
+        Map<String, Float[]> data = new HashMap<>();
+        data.put("1", new Float[]{6.0f, 6.0f});


How about we have a document without the vector and then assert that particular doc comes at the end of the result? Same for both CosineTestData as well.

Added test data.

1. Requires dimension must be consistent between query vector and input vector 2. Remove thread safe for KNNScriptDocValues 3. Throw exception if document doesn't contains vector field.

To avoid exception, user's can use size method to validate.

Include error message with additional details on how to fix the problem.

Add field name as part of error message

jmazanec15

LGTM!

Use scriptdoc to extract values from segment.

Use KNNScriptDocValues to retrieve float[] from the document instead of explicity retrieval.

vamshin

LGTM! Thanks for adding painless support. This will help users to have more customization on the scoring.

VijayanB added 3 commits December 10, 2020 16:48

Add new methods for whitelisting

21f8010

Added new methods which will be whitelisted for users to call in source field for painless scripting.

Create Whitelist text file

10dd927

Add Extension class and return list of whitelisted method. Update gradle to add dependencies to painless.

VijayanB requested review from jmazanec15 and vamshin December 11, 2020 23:59

Remove Test Annotation

94d2277

test annotation is forbidden

jmazanec15 reviewed Dec 14, 2020

View reviewed changes

VijayanB added 3 commits December 14, 2020 14:49

Fixed code review comments

dfe7e91

1. Made KNNScriptDocValues thread safe 2. reformat code 3. rename cosinesimiloptimized.

Remove nonnull check

690c5a9

Fixed formatting errors

5f1a9a0

VijayanB requested a review from jmazanec15 December 15, 2020 02:43

Add Integration test

aa7a486

Add painless scripting score to verify whether following methods are available for users. 1. l2Squared 2. cosineSimilarity(queryVector,doc[field]) 3. cosineSimilarity(queryVector,doc[field],normalizedVector)

jmazanec15 reviewed Dec 16, 2020

View reviewed changes

Fixed review comments

71f4a60

1. updated exception type. 2. Added test

VijayanB requested a review from jmazanec15 December 16, 2020 21:58

jmazanec15 approved these changes Dec 17, 2020

View reviewed changes

vamshin reviewed Dec 17, 2020

View reviewed changes

VijayanB added 2 commits December 18, 2020 14:00

Fix review comments

436aff3

1. Requires dimension must be consistent between query vector and input vector 2. Remove thread safe for KNNScriptDocValues 3. Throw exception if document doesn't contains vector field.

Fixed lint errors

cf32fec

VijayanB mentioned this pull request Dec 20, 2020

Added section for painless scripting opendistro/for-elasticsearch-docs#381

Closed

VijayanB added 3 commits December 21, 2020 16:22

Add test case to check size

ff846d9

To avoid exception, user's can use size method to validate.

Extract constant

dedba6d

Update error message

9722abb

Include error message with additional details on how to fix the problem.

VijayanB requested review from vamshin and jmazanec15 December 22, 2020 23:18

Update error message

db29a85

Add field name as part of error message

jmazanec15 approved these changes Dec 23, 2020

View reviewed changes

Refactor ScoreScirpt

06c6601

Use scriptdoc to extract values from segment.

Refactor ScoreScript

44b2ace

Use KNNScriptDocValues to retrieve float[] from the document instead of explicity retrieval.

vamshin approved these changes Dec 28, 2020

View reviewed changes

VijayanB merged commit c41f375 into opendistro-for-elasticsearch:master Dec 29, 2020

VijayanB added the Features New functionality added label Feb 1, 2021

VijayanB changed the title ~~Whitelist scoring methods~~ Use k-NN similarity functions in painless scripting Feb 1, 2021

VijayanB changed the title ~~Use k-NN similarity functions in painless scripting~~ Support k-NN similarity functions in painless scripting Feb 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support k-NN similarity functions in painless scripting #281

Support k-NN similarity functions in painless scripting #281

VijayanB commented Dec 11, 2020

VijayanB commented Dec 12, 2020

codecov bot commented Dec 12, 2020 •

edited

Loading

jmazanec15 left a comment

jmazanec15 Dec 14, 2020

VijayanB Dec 14, 2020

jmazanec15 Dec 14, 2020

VijayanB Dec 14, 2020

jmazanec15 left a comment

jmazanec15 Dec 16, 2020

VijayanB Dec 16, 2020

jmazanec15 Dec 16, 2020

VijayanB Dec 16, 2020 •

edited

Loading

jmazanec15 Dec 17, 2020

jmazanec15 left a comment

jmazanec15 Dec 17, 2020

vamshin Dec 17, 2020

VijayanB Dec 17, 2020

vamshin Dec 17, 2020 •

edited

Loading

VijayanB Dec 18, 2020

vamshin Dec 17, 2020

VijayanB Dec 17, 2020

vamshin Dec 17, 2020

VijayanB Dec 18, 2020

vamshin Dec 17, 2020

VijayanB Dec 19, 2020

jmazanec15 left a comment

vamshin left a comment •

edited

Loading

Support k-NN similarity functions in painless scripting #281

Support k-NN similarity functions in painless scripting #281

Conversation

VijayanB commented Dec 11, 2020

VijayanB commented Dec 12, 2020

codecov bot commented Dec 12, 2020 • edited Loading

Codecov Report

jmazanec15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VijayanB Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vamshin Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

vamshin left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2020 •

edited

Loading

VijayanB Dec 16, 2020 •

edited

Loading

vamshin Dec 17, 2020 •

edited

Loading

vamshin left a comment •

edited

Loading