Support script score when doc value is disabled and fix misusing DISI #1696

bugmakerrrrrr · 2024-05-09T16:36:35Z

Description

The function was initially introduced in #1573, but was later reverted due to #1647. The root cause of the issue can be found in this comment. This PR reintroduces the function and fixes misusing DISI.

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

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.

navneet1v · 2024-05-09T17:35:27Z

@bugmakerrrrrr can you add the entry for this feature in changelog.

navneet1v · 2024-05-09T17:38:28Z

@bugmakerrrrrr couple of things:

Can you add PR comment on the piece code that you have added to fix the bug. This will help me review the PR faster.
Did we add the IT which replicate the bug scenario to see the bug is not happening again. If possible can you add a comment in the PR for that too.

ryanbogan · 2024-05-09T18:13:08Z

Code looks good to me if we can add that integration test. For the IT, the exact scenario was two vectors with data ingested, then run script score on either.

navneet1v · 2024-05-09T20:50:26Z

Code looks good to me if we can add that integration test. For the IT, the exact scenario was two vectors with data ingested, then run script score on either.

can you paste the query and the index mapping @ryanbogan for better understanding

bugmakerrrrrr · 2024-05-10T15:45:31Z

src/main/java/org/opensearch/knn/index/KNNVectorScriptDocValues.java

    private final String fieldName;
    @Getter
    private final VectorDataType vectorDataType;
    private boolean docExists = false;
+    private int lastDocID = -1;

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


this method fix the bug in #1647

bugmakerrrrrr · 2024-05-10T15:48:04Z

src/test/java/org/opensearch/knn/plugin/script/KNNScriptScoringIT.java

@@ -669,7 +689,8 @@ private void testKNNScriptScore(SpaceType spaceType) throws Exception {
        final float[] queryVector = randomVector(dims);
        final BiFunction<float[], float[], Float> scoreFunction = getScoreFunction(spaceType, queryVector);
        for (String mapper : createMappers(dims)) {
-            createIndexAndAssertScriptScore(mapper, spaceType, scoreFunction, dims, queryVector);
+            createIndexAndAssertScriptScore(mapper, spaceType, scoreFunction, dims, queryVector, true);
+            createIndexAndAssertScriptScore(mapper, spaceType, scoreFunction, dims, queryVector, false);


the issue in #1647 can be replicated using the non-dense test case.

ryanbogan

LGTM! Thanks for the contribution!

jmazanec15

LGTM

jmazanec15 · 2024-05-15T18:53:27Z

@bugmakerrrrrr can you fix conflict? Also, I have some good benchmarks for script scoring between lucene and plugin dv, Ill share those over on the issue.

…nsearch-project#1662)" This reverts commit bd2f403. Signed-off-by: panguixin <[email protected]>

Signed-off-by: panguixin <[email protected]>

bugmakerrrrrr · 2024-05-18T16:42:55Z

outcome

@bugmakerrrrrr can you fix conflict? Also, I have some good benchmarks for script scoring between lucene and plugin dv, Ill share those over on the issue.

Fixed. Expecting to see these results.

Signed-off-by: panguixin <[email protected]>

jmazanec15 · 2024-05-20T21:23:07Z

@bugmakerrrrrr Posted here: #1709.

…#1696) * Revert "Revert 'Support script score when doc value is disabled' (#1662)" This reverts commit bd2f403. Signed-off-by: panguixin <[email protected]> * fix misusing doc value Signed-off-by: panguixin <[email protected]> * add changelog Signed-off-by: panguixin <[email protected]> --------- Signed-off-by: panguixin <[email protected]> (cherry picked from commit 4d59d4c)

…opensearch-project#1696) * Revert "Revert 'Support script score when doc value is disabled' (opensearch-project#1662)" This reverts commit bd2f403. Signed-off-by: panguixin <[email protected]> * fix misusing doc value Signed-off-by: panguixin <[email protected]> * add changelog Signed-off-by: panguixin <[email protected]> --------- Signed-off-by: panguixin <[email protected]>

* Fix flaky test in Faiss JNI range search (#1705) Signed-off-by: Junqiu Lei <[email protected]> * Support script score when doc value is disabled and fix misusing DISI (#1696) * Revert "Revert 'Support script score when doc value is disabled' (#1662)" This reverts commit bd2f403. Signed-off-by: panguixin <[email protected]> * fix misusing doc value Signed-off-by: panguixin <[email protected]> * add changelog Signed-off-by: panguixin <[email protected]> --------- Signed-off-by: panguixin <[email protected]> * --- (#1712) updated-dependencies: - dependency-name: requests dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update threshold value after new result is added (#1715) Signed-off-by: Heemin Kim <[email protected]> * Use the Lucene Distance Calculation Function in Script Scoring for doing exact search (#1699) * Use the Lucene Distance Calculation Function in Script Scoring for doing exact search Signed-off-by: Ryan Bogan <[email protected]> * Add Changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Fix failing test Signed-off-by: Ryan Bogan <[email protected]> * fix test Signed-off-by: Ryan Bogan <[email protected]> * Fix test bug and remove unnecessary validation Signed-off-by: Ryan Bogan <[email protected]> * Remove cosineSimilOptimized Signed-off-by: Ryan Bogan <[email protected]> * Revert "Remove cosineSimilOptimized" This reverts commit f872d83. Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> * Add validation for pq m parameter before training starts (#1713) * Add validation for pq code count before training starts Signed-off-by: Ryan Bogan <[email protected]> * Add integration test Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Clean up code Signed-off-by: Ryan Bogan <[email protected]> * Remove unnecessary lines Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Change framework to add validation with data Signed-off-by: Ryan Bogan <[email protected]> * Remove unused error message Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> * Change space type check name for readability Signed-off-by: Ryan Bogan <[email protected]> * Add javadocs Signed-off-by: Ryan Bogan <[email protected]> * Modify validation error wording and add json structure to tests Signed-off-by: Ryan Bogan <[email protected]> * Change TrainingDataSpec to VectorSpaceInfo Signed-off-by: Ryan Bogan <[email protected]> * Add unit tests Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> * Updating the BWC test config after 2.14 release (#1724) Signed-off-by: Navneet Verma <[email protected]> --------- Signed-off-by: Junqiu Lei <[email protected]> Signed-off-by: panguixin <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Heemin Kim <[email protected]> Signed-off-by: Ryan Bogan <[email protected]> Signed-off-by: Navneet Verma <[email protected]> Co-authored-by: Junqiu Lei <[email protected]> Co-authored-by: panguixin <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Heemin Kim <[email protected]> Co-authored-by: Ryan Bogan <[email protected]> Co-authored-by: Navneet Verma <[email protected]>

bugmakerrrrrr requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners May 9, 2024 16:36

bugmakerrrrrr commented May 10, 2024

View reviewed changes

ryanbogan previously approved these changes May 13, 2024

View reviewed changes

jmazanec15 previously approved these changes May 15, 2024

View reviewed changes

bugmakerrrrrr added 2 commits May 19, 2024 00:40

Revert "Revert 'Support script score when doc value is disabled' (ope…

07374f0

…nsearch-project#1662)" This reverts commit bd2f403. Signed-off-by: panguixin <[email protected]>

fix misusing doc value

25c90d7

Signed-off-by: panguixin <[email protected]>

bugmakerrrrrr dismissed stale reviews from jmazanec15 and ryanbogan via ebcc5a4 May 18, 2024 16:41

bugmakerrrrrr force-pushed the fix_doc_value branch from 1c6c395 to ebcc5a4 Compare May 18, 2024 16:41

add changelog

aca1147

Signed-off-by: panguixin <[email protected]>

bugmakerrrrrr force-pushed the fix_doc_value branch from ebcc5a4 to aca1147 Compare May 18, 2024 16:47

jmazanec15 approved these changes May 20, 2024

View reviewed changes

jmazanec15 requested a review from ryanbogan May 20, 2024 21:29

ryanbogan approved these changes May 20, 2024

View reviewed changes

jmazanec15 merged commit 4d59d4c into opensearch-project:main May 20, 2024
49 of 51 checks passed

jmazanec15 added backport 2.x Enhancements Increases software capabilities beyond original client specifications labels May 20, 2024

opensearch-trigger-bot bot mentioned this pull request May 20, 2024

[Backport 2.x] Support script score when doc value is disabled and fix misusing DISI #1711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support script score when doc value is disabled and fix misusing DISI #1696

Support script score when doc value is disabled and fix misusing DISI #1696

bugmakerrrrrr commented May 9, 2024

navneet1v commented May 9, 2024

navneet1v commented May 9, 2024

ryanbogan commented May 9, 2024

navneet1v commented May 9, 2024

bugmakerrrrrr May 10, 2024

bugmakerrrrrr May 10, 2024

ryanbogan left a comment •

edited

Loading

jmazanec15 left a comment

jmazanec15 commented May 15, 2024

bugmakerrrrrr commented May 18, 2024

jmazanec15 commented May 20, 2024

Support script score when doc value is disabled and fix misusing DISI #1696

Support script score when doc value is disabled and fix misusing DISI #1696

Conversation

bugmakerrrrrr commented May 9, 2024

Description

Issues Resolved

Check List

navneet1v commented May 9, 2024

navneet1v commented May 9, 2024

ryanbogan commented May 9, 2024

navneet1v commented May 9, 2024

bugmakerrrrrr May 10, 2024

Choose a reason for hiding this comment

bugmakerrrrrr May 10, 2024

Choose a reason for hiding this comment

ryanbogan left a comment • edited Loading

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

jmazanec15 commented May 15, 2024

bugmakerrrrrr commented May 18, 2024

jmazanec15 commented May 20, 2024

ryanbogan left a comment •

edited

Loading