Adds in lazy execution for Lucene kNN queries #2305

kotwanikunal · 2024-12-03T17:43:20Z

Description

Optimizes Lucene kNN query execution path in case of rewrites
The caveat here is that the rewrites would be deferred until weight creation. Any underlying changes to Lucene rewrite method would potentially be skipped with this execution mechanism.

Setup and Dataset

Opensearch docker container on a M1 Macbook Pro - Image ID: e1d9151cce32
Single node, default config
Load Gen: OSB vectorsearch workload running on local node hosting the docker container
Dataset: sift-128-euclidean.hdf5 (Params: https://github.com/opensearch-project/opensearch-benchmark-workloads/blob/main/vectorsearch/params/faiss-sift-128-l2.json)
Num docs: 1M, num shards: 2

Flame Graph

Default 2.18 code

Patched 2.18 code

Benchmarks

2.18 Default Code:

Metric	Task	Value	Unit
Min Throughput	prod-queries	20.93	ops/s
Mean Throughput	prod-queries	63.23	ops/s
Median Throughput	prod-queries	69.54	ops/s
Max Throughput	prod-queries	75.17	ops/s
50th percentile latency	prod-queries	10.6464	ms
90th percentile latency	prod-queries	13.7257	ms
99th percentile latency	prod-queries	21.3663	ms
99.9th percentile latency	prod-queries	71.429	ms
100th percentile latency	prod-queries	269.424	ms
50th percentile service time	prod-queries	10.6464	ms
90th percentile service time	prod-queries	13.7257	ms
99th percentile service time	prod-queries	21.3663	ms
99.9th percentile service time	prod-queries	71.429	ms
100th percentile service time	prod-queries	269.424	ms
error rate	prod-queries	0	%
Mean recall@k	prod-queries	0.97
Mean recall@1	prod-queries	0.98

Patched new 2.18 code:

Metric	Task	Value	Unit
Min Throughput	prod-queries	75.97	ops/s
Mean Throughput	prod-queries	79.29	ops/s
Median Throughput	prod-queries	78.68	ops/s
Max Throughput	prod-queries	83.04	ops/s
50th percentile latency	prod-queries	8.66315	ms
90th percentile latency	prod-queries	11.4286	ms
99th percentile latency	prod-queries	21.4076	ms
99.9th percentile latency	prod-queries	34.8355	ms
100th percentile latency	prod-queries	53.219	ms
50th percentile service time	prod-queries	8.66315	ms
90th percentile service time	prod-queries	11.4286	ms
99th percentile service time	prod-queries	21.4076	ms
99.9th percentile service time	prod-queries	34.8355	ms
100th percentile service time	prod-queries	53.219	ms
error rate	prod-queries	0	%
Mean recall@k	prod-queries	0.97
Mean recall@1	prod-queries	0.98

JFRs

JFR-patched-default.zip

Related Issues

Resolves #2115

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.

navneet1v · 2024-12-03T23:46:24Z

@kotwanikunal can you fix the CIs. Change looks good to me as it is minial in terms of changing the code and getting the best performance out of the system

kotwanikunal · 2024-12-04T19:18:43Z

@kotwanikunal can you fix the CIs. Change looks good to me as it is minial in terms of changing the code and getting the best performance out of the system

@navneet1v I have updated the description with updated flame graphs and benchmarking numbers.
There are some fixes for the CI - I'll fix them shortly.

navneet1v · 2024-12-04T22:18:27Z

@kotwanikunal thanks. This p99 seems interesting. Rest all latency have decreased as expected.

navneet1v · 2024-12-05T08:01:31Z

@kotwanikunal can you please add the details like:

Setup.
Dataset used
for the benchmarks

kotwanikunal · 2024-12-05T09:43:04Z

@kotwanikunal can you please add the details like:

Setup.

Dataset used
for the benchmarks

Added to the description.

navneet1v

overall code looks good to me. One question I have is why we are not changing the DiversifyingChildrenByteKnnVectorQuery and DiversifyingChildrenFloatKnnVectorQuery in this PR? Is there some other PR that is taking care of this?

@kotwanikunal

navneet1v · 2024-12-09T03:54:19Z

overall code looks good to me. One question I have is why we are not changing the DiversifyingChildrenByteKnnVectorQuery and DiversifyingChildrenFloatKnnVectorQuery in this PR? Is there some other PR that is taking care of this?

@kotwanikunal

NVM, I see this PR: https://github.com/opensearch-project/k-NN/pull/2283/files#diff-d6bb9cc296becc98be3ac4bddc74c2b1520d09ff8b7d77467b3a765df17c4c11 is handling that case.

martin-gaievski

Overall code looks good, added few minor comments.
Did you see impact of efficient filtering with Lucene, can you run a benchmark to check if there is an impact?

martin-gaievski · 2024-12-09T16:30:30Z

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

@@ -512,7 +512,7 @@ public void testDoToQuery_KnnQueryWithFilter_Lucene() throws Exception {

        // Then
        assertNotNull(query);
-        assertTrue(query.getClass().isAssignableFrom(KnnFloatVectorQuery.class));
+        assertTrue(query.getClass().isAssignableFrom(LuceneEngineKnnVectorQuery.class));


is there a way to check the type of the underlying lucene query, looks like this is what we're asserting in today's version? How about adding a package private getter for the lucene query, or a public method that returns the type of the query class.

src/main/java/org/opensearch/knn/index/query/lucene/LuceneEngineKnnVectorQuery.java

martin-gaievski

Please address my comments in follow up PRs

navneet1v · 2024-12-11T19:19:14Z

Conflicts fixed. Will wait for CI to complete

navneet1v · 2024-12-11T20:37:27Z

The BWC test: k-NN Rolling-Upgrade BWC Tests (21, ubuntu-latest, 2.19.0-SNAPSHOT, 3.0.0-SNAPSHOT) will fixed once we have code for innerhits merges and added in 2.19 snapshot.

Signed-off-by: Kunal Kotwani <[email protected]>

opensearch-trigger-bot · 2024-12-11T22:05:06Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2305-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2b9a741a7cfb8c9a55c7412a6e19ea699b7a921d
# Push it to GitHub
git push --set-upstream origin backport/backport-2305-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2305-to-2.x.

Signed-off-by: Kunal Kotwani <[email protected]> (cherry picked from commit 2b9a741)

Signed-off-by: Kunal Kotwani <[email protected]> (cherry picked from commit 2b9a741) Co-authored-by: Kunal Kotwani <[email protected]>

Signed-off-by: Kunal Kotwani <[email protected]>

kotwanikunal requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan, luyuncheng and shatejas as code owners December 3, 2024 17:43

navneet1v mentioned this pull request Dec 3, 2024

[FEATURE] Improving Lucene Engine Query Performance by reducing number of times a single Lucene k-NN query gets executed #2115

Closed

kotwanikunal force-pushed the lazy-lucene-query branch from e9b5456 to 38dc781 Compare December 4, 2024 19:17

kotwanikunal force-pushed the lazy-lucene-query branch from 38dc781 to 564c783 Compare December 5, 2024 05:25

kotwanikunal mentioned this pull request Dec 7, 2024

[FEATURE] Unify LuceneEngineKnnVectorQuery and NativeEngineKnnVectorQuery within KNNQuery #2316

Open

navneet1v previously approved these changes Dec 9, 2024

View reviewed changes

navneet1v added backport 2.x v2.19.0 search-improvements Enhancements Increases software capabilities beyond original client specifications labels Dec 9, 2024

martin-gaievski reviewed Dec 9, 2024

View reviewed changes

martin-gaievski previously approved these changes Dec 11, 2024

View reviewed changes

navneet1v dismissed stale reviews from martin-gaievski and themself via 355a060 December 11, 2024 19:18

navneet1v force-pushed the lazy-lucene-query branch from 564c783 to 355a060 Compare December 11, 2024 19:18

navneet1v previously approved these changes Dec 11, 2024

View reviewed changes

navneet1v dismissed their stale review via f1381a3 December 11, 2024 20:13

navneet1v force-pushed the lazy-lucene-query branch from 355a060 to f1381a3 Compare December 11, 2024 20:13

navneet1v previously approved these changes Dec 11, 2024

View reviewed changes

navneet1v dismissed their stale review via b265e45 December 11, 2024 20:53

navneet1v force-pushed the lazy-lucene-query branch from f1381a3 to b265e45 Compare December 11, 2024 20:53

Adds in lazy execution for Lucene kNN queries

c348fcf

Signed-off-by: Kunal Kotwani <[email protected]>

navneet1v force-pushed the lazy-lucene-query branch from b265e45 to c348fcf Compare December 11, 2024 20:59

navneet1v approved these changes Dec 11, 2024

View reviewed changes

martin-gaievski approved these changes Dec 11, 2024

View reviewed changes

navneet1v merged commit 2b9a741 into opensearch-project:main Dec 11, 2024
30 of 31 checks passed

navneet1v added backport 2.x and removed backport 2.x labels Dec 11, 2024

opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 11, 2024

Adds in lazy execution for Lucene kNN queries (#2305)

ce6f32e

Signed-off-by: Kunal Kotwani <[email protected]> (cherry picked from commit 2b9a741)

opensearch-trigger-bot bot mentioned this pull request Dec 11, 2024

[Backport 2.x] Adds in lazy execution for Lucene kNN queries #2328

Merged

navneet1v pushed a commit that referenced this pull request Dec 11, 2024

Adds in lazy execution for Lucene kNN queries (#2305) (#2328)

8daedac

Signed-off-by: Kunal Kotwani <[email protected]> (cherry picked from commit 2b9a741) Co-authored-by: Kunal Kotwani <[email protected]>

owenhalpert pushed a commit to owenhalpert/k-NN that referenced this pull request Dec 19, 2024

Adds in lazy execution for Lucene kNN queries (opensearch-project#2305)

e366740

Signed-off-by: Kunal Kotwani <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds in lazy execution for Lucene kNN queries #2305

Adds in lazy execution for Lucene kNN queries #2305

kotwanikunal commented Dec 3, 2024 •

edited

Loading

navneet1v commented Dec 3, 2024

kotwanikunal commented Dec 4, 2024

navneet1v commented Dec 4, 2024

navneet1v commented Dec 5, 2024

kotwanikunal commented Dec 5, 2024

navneet1v left a comment

navneet1v commented Dec 9, 2024

martin-gaievski left a comment

martin-gaievski Dec 9, 2024

martin-gaievski left a comment

navneet1v commented Dec 11, 2024

navneet1v commented Dec 11, 2024

opensearch-trigger-bot bot commented Dec 11, 2024

Adds in lazy execution for Lucene kNN queries #2305

Adds in lazy execution for Lucene kNN queries #2305

Conversation

kotwanikunal commented Dec 3, 2024 • edited Loading

Description

Setup and Dataset

Flame Graph

Benchmarks

JFRs

Related Issues

Check List

navneet1v commented Dec 3, 2024

kotwanikunal commented Dec 4, 2024

navneet1v commented Dec 4, 2024

navneet1v commented Dec 5, 2024

kotwanikunal commented Dec 5, 2024

navneet1v left a comment

Choose a reason for hiding this comment

navneet1v commented Dec 9, 2024

martin-gaievski left a comment

Choose a reason for hiding this comment

martin-gaievski Dec 9, 2024

Choose a reason for hiding this comment

martin-gaievski left a comment

Choose a reason for hiding this comment

navneet1v commented Dec 11, 2024

navneet1v commented Dec 11, 2024

opensearch-trigger-bot bot commented Dec 11, 2024

kotwanikunal commented Dec 3, 2024 •

edited

Loading