Ensure consistent filter query in KNNQueryBuilder across multiple shards #2359

buddharajusahil · 2024-12-30T23:41:28Z

Description

This change is to resolve issue #2339. This PR addresses it by changing some flawed logic in the rewrite function of the KNNQueryBuilder class, introduced in this PR: #1874. What was happening before was the filter for the KNNQueryBuilder object was being rewritten upon contact with first shard, and since this instance is getting shared across all shards, other shards are using the rewritten filter from the first shard.

What this causes is, if shard id 0 has no documents and shard id 1 has 1 document, if query lands on shard 0, the filter is rewritten to return none, as shard id 0 has no documents, so the filter is adjusted. However, since we were changing the filter in the KNN query object, rather than creating a new rewritten object for shard 0, when the query lands on shard 1, the filter has been rewritten to return none from shard 0, so even though the document is on shard 1, we will return no hits.

Now it is changed so that the rewrite class returns a new KNNQueryBuilder with the rewritten filter per shard.

Related Issues

Resolves #2339

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

Signed-off-by: Sahil Buddharaju <[email protected]>

navneet1v · 2024-12-31T01:46:11Z

What was happening before was the filter for the KNNQueryBuilder object was being rewritten upon contact with first shard, and since this instance is getting shared across all shards, other shards are using the rewritten filter from the first shard.

Hi @buddharajusahil thanks for creating the PR. can you please add some details why above thing is impacting the queries?

src/test/java/org/opensearch/knn/index/FaissIT.java

CHANGELOG.md

jmazanec15 · 2024-12-31T16:33:10Z

Thanks for fix @buddharajusahil - seems this is consistent with how bool query does it - https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java#L367-L401.

For the test cases, did you confirm that they fail before change is introduced and not after just to validate the fix?

Signed-off-by: Sahil Buddharaju <[email protected]>

buddharajusahil · 2024-12-31T21:01:25Z

@jmazanec15 Hi Jack, I decided to add new IT tests, as martin's were moreso checking if unknown filters were being properly written. With these new IT's, they are able to properly fail with the old rewrite, and succeed with the change.

src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java

shatejas

Should we make the fields in KNNQueryBuilder final?

navneet1v · 2025-01-02T20:33:05Z

What this causes is, if shard id 0 has no documents and shard id 1 has 1 document, if query lands on shard 0, the filter is rewritten to return none, as shard id 0 has no documents, so the filter is adjusted. However, since we were changing the filter in the KNN query object, rather than creating a new rewritten object for shard 0, when the query lands on shard 1, the filter has been rewritten to return none from shard 0, so even though the document is on shard 1, we will return no hits.

@buddharajusahil This was a great find. Thanks for the deep-dive.

navneet1v · 2025-01-02T20:36:55Z

Should we make the fields in KNNQueryBuilder final?

not sure how making things final will help here? Final helps only in case when references are changing. Do you think any other reason here for making things final

navneet1v

@buddharajusahil
Code looks good to me. Please fix the comments added by @shatejas , as I have no more extra comments apart from his comments

Signed-off-by: Sahil Buddharaju <[email protected]>

shatejas · 2025-01-03T00:52:21Z

Should we make the fields in KNNQueryBuilder final?

not sure how making things final will help here? Final helps only in case when references are changing. Do you think any other reason here for making things final

@navneet1v here the filter was getting reassigned in 1 shard which was affecting another shard. Making all fields final will makes sure that KNNQueryBuilder becomes immutable (sort of) and any change where the final is reverted can be identified

src/test/java/org/opensearch/knn/index/query/KNNQueryBuilderTests.java

shatejas

Looks good overall

Signed-off-by: Sahil Buddharaju <[email protected]>

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

…rds (#2359) * Changed filter logic Signed-off-by: Sahil Buddharaju <[email protected]> * spotless Signed-off-by: Sahil Buddharaju <[email protected]> * Changelog Signed-off-by: Sahil Buddharaju <[email protected]> * Added unit tests to test multi shard filters Signed-off-by: Sahil Buddharaju <[email protected]> * Changed unit tests and used builder instead of constructor Signed-off-by: Sahil Buddharaju <[email protected]> * Spotless apply Signed-off-by: Sahil Buddharaju <[email protected]> * slight unit test adjustment Signed-off-by: Sahil Buddharaju <[email protected]> * changelog Signed-off-by: Sahil Buddharaju <[email protected]> --------- Signed-off-by: Sahil Buddharaju <[email protected]> Signed-off-by: sahil <[email protected]> Co-authored-by: Sahil Buddharaju <[email protected]> (cherry picked from commit c969f1d)

…rds (opensearch-project#2359) * Changed filter logic Signed-off-by: Sahil Buddharaju <[email protected]> * spotless Signed-off-by: Sahil Buddharaju <[email protected]> * Changelog Signed-off-by: Sahil Buddharaju <[email protected]> * Added unit tests to test multi shard filters Signed-off-by: Sahil Buddharaju <[email protected]> * Changed unit tests and used builder instead of constructor Signed-off-by: Sahil Buddharaju <[email protected]> * Spotless apply Signed-off-by: Sahil Buddharaju <[email protected]> * slight unit test adjustment Signed-off-by: Sahil Buddharaju <[email protected]> * changelog Signed-off-by: Sahil Buddharaju <[email protected]> --------- Signed-off-by: Sahil Buddharaju <[email protected]> Signed-off-by: sahil <[email protected]> Co-authored-by: Sahil Buddharaju <[email protected]>

buddharajusahil requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan, luyuncheng and shatejas as code owners December 30, 2024 23:41

Sahil Buddharaju added 3 commits December 30, 2024 15:59

Changed filter logic

e179b3c

Signed-off-by: Sahil Buddharaju <[email protected]>

spotless

aa00c5c

Signed-off-by: Sahil Buddharaju <[email protected]>

Changelog

bdd753d

Signed-off-by: Sahil Buddharaju <[email protected]>

buddharajusahil force-pushed the filterbug branch from 2d874cb to bdd753d Compare December 31, 2024 00:13

navneet1v reviewed Dec 31, 2024

View reviewed changes

src/test/java/org/opensearch/knn/index/FaissIT.java Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

Added unit tests to test multi shard filters

84e9c74

Signed-off-by: Sahil Buddharaju <[email protected]>

buddharajusahil force-pushed the filterbug branch from 4124d06 to 84e9c74 Compare December 31, 2024 20:58

shatejas reviewed Jan 1, 2025

View reviewed changes

shatejas changed the title ~~Filterbug~~ Ensure consistent filter queries in KNNQueryBuilder across multiple shards. Jan 1, 2025

shatejas reviewed Jan 1, 2025

View reviewed changes

shatejas changed the title ~~Ensure consistent filter queries in KNNQueryBuilder across multiple shards.~~ Ensure consistent filter query in KNNQueryBuilder across multiple shards. Jan 1, 2025

shatejas changed the title ~~Ensure consistent filter query in KNNQueryBuilder across multiple shards.~~ Ensure consistent filter query in KNNQueryBuilder across multiple shards Jan 1, 2025

navneet1v reviewed Jan 2, 2025

View reviewed changes

Changed unit tests and used builder instead of constructor

17d7a24

Signed-off-by: Sahil Buddharaju <[email protected]>

buddharajusahil force-pushed the filterbug branch from 30734d7 to 17d7a24 Compare January 2, 2025 21:51

Spotless apply

d6a2ebb

Signed-off-by: Sahil Buddharaju <[email protected]>

buddharajusahil force-pushed the filterbug branch from 22b27dc to d6a2ebb Compare January 2, 2025 22:12

shatejas reviewed Jan 3, 2025

View reviewed changes

src/test/java/org/opensearch/knn/index/query/KNNQueryBuilderTests.java Outdated Show resolved Hide resolved

shatejas previously approved these changes Jan 3, 2025

View reviewed changes

slight unit test adjustment

9a03e20

Signed-off-by: Sahil Buddharaju <[email protected]>

buddharajusahil dismissed shatejas’s stale review via 9a03e20 January 3, 2025 17:20

buddharajusahil force-pushed the filterbug branch from 85f362d to 9a03e20 Compare January 3, 2025 17:20

changelog

887af64

Signed-off-by: Sahil Buddharaju <[email protected]>

buddharajusahil force-pushed the filterbug branch from 4986cae to 887af64 Compare January 3, 2025 17:58

Merge branch 'main' into filterbug

18c8665

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

kotwanikunal approved these changes Jan 3, 2025

View reviewed changes

shatejas approved these changes Jan 3, 2025

View reviewed changes

navneet1v approved these changes Jan 3, 2025

View reviewed changes

navneet1v merged commit c969f1d into opensearch-project:main Jan 3, 2025
31 checks passed

navneet1v added Bug Fixes Changes to a system or product designed to handle a programming bug/glitch v2.19.0 backport 2.x labels Jan 3, 2025

opensearch-trigger-bot bot mentioned this pull request Jan 3, 2025

[Backport 2.x] Ensure consistent filter query in KNNQueryBuilder across multiple shards #2362

Merged

heemin32 mentioned this pull request Jan 6, 2025

[BUG] Insufficient number of hits for nested knn queries with efficient filter #2347

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure consistent filter query in KNNQueryBuilder across multiple shards #2359

Ensure consistent filter query in KNNQueryBuilder across multiple shards #2359

buddharajusahil commented Dec 30, 2024 •

edited

Loading

navneet1v commented Dec 31, 2024

jmazanec15 commented Dec 31, 2024

buddharajusahil commented Dec 31, 2024

shatejas left a comment

navneet1v commented Jan 2, 2025

navneet1v commented Jan 2, 2025

navneet1v left a comment •

edited

Loading

shatejas commented Jan 3, 2025 •

edited

Loading

shatejas left a comment

Ensure consistent filter query in KNNQueryBuilder across multiple shards #2359

Ensure consistent filter query in KNNQueryBuilder across multiple shards #2359

Conversation

buddharajusahil commented Dec 30, 2024 • edited Loading

Description

Related Issues

Check List

navneet1v commented Dec 31, 2024

jmazanec15 commented Dec 31, 2024

buddharajusahil commented Dec 31, 2024

shatejas left a comment

Choose a reason for hiding this comment

navneet1v commented Jan 2, 2025

navneet1v commented Jan 2, 2025

navneet1v left a comment • edited Loading

Choose a reason for hiding this comment

shatejas commented Jan 3, 2025 • edited Loading

shatejas left a comment

Choose a reason for hiding this comment

buddharajusahil commented Dec 30, 2024 •

edited

Loading

navneet1v left a comment •

edited

Loading

shatejas commented Jan 3, 2025 •

edited

Loading