Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LuceneOnFaiss Part-8] Added index.knn.memory_optimized_search index setting. #2616

Open
wants to merge 1 commit into
base: lucene-on-faiss
Choose a base branch
from

Conversation

0ctopus13prime
Copy link
Collaborator

Description

This PR adds index.knn.memory_optimized_search index settings.
Overall, this brings two major changes listed below along with the new setting definition:

  1. Add 'index name' in field attributes.
    In VectorReader, we check whether memory optimized search is enabled. If it is enabled, then use IndexInput to partial load FAISS index for searching. Partial loaded index hierarchy will be used for serving a query via Lucene's HNSW graph searcher.
    Since this setting sits on index level, we need an index name to query whether if it's enabled or not.
    To this, we inject index name into field attributes.

  2. Branching in KNNQueryBuilder
    This PR will make each knn field type decide whether memory-optimized-search is supported or not.
    Currently only Float HNSW is supported, therefore any binary HNSW graphs will return false to indicate the feature is not supported.
    If target field in search is supported for memory-optimized-search, control will fallback to Lucene query to proceed vector search. In which, Lucene's HNSW graph searcher will perform KNN search + Radius search on FAISS index.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

Sorry, something went wrong.

@0ctopus13prime
Copy link
Collaborator Author

Part-7 #2608 was merged.
Will rebase and re-raise a new revision.
In the new revision, it will have unit tests.

@0ctopus13prime 0ctopus13prime force-pushed the lucene-on-faiss-part8 branch 4 times, most recently from fbd6e65 to fcef387 Compare March 20, 2025 02:13
@0ctopus13prime
Copy link
Collaborator Author

Hi @jmazanec15 ,

In this revision, I've included a hybrid approach inspired by @shatejas 's suggestion.

Previously, I added lazy loading in search because accessing the index.knn.memory_optimized_search setting in the constructor seemed tricky. Retrieving this value requires knowing the index name, and more importantly, during recovery, the codec is loaded via Lucene's SPI, where OpenSearch's internal framework isn't available. That was the main reason for switching to lazy loading within search.

However, this scenario is rare. In most cases, OpenSearch's internal framework (e.g., IndexSettings) is available. Additionally, since we're trying to compress long[] on the fly during loading in #2609 , relying solely on lazy loading in search could negatively impact p99, as it might take several seconds for larger datasets.

With this hybrid approach, we attempt to initialize in the constructor. If IndexSettings is available, we fetch the boolean value; otherwise, we do nothing. This way, most searches will find the table already initialized, no need for lazy loading. Only searches on a just-recovered index will require it.

Let me know if you have any major concerns. I think this approach not only mitigates potential p99 issues but also avoids the Lucene SPI limitation.

@0ctopus13prime 0ctopus13prime force-pushed the lucene-on-faiss-part8 branch 2 times, most recently from c0dc961 to 822f918 Compare March 21, 2025 22:48
Copy link
Collaborator

@shatejas shatejas left a comment

Choose a reason for hiding this comment

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

Looks good overall - couple of comments

@0ctopus13prime 0ctopus13prime force-pushed the lucene-on-faiss-part8 branch from 822f918 to 6132ada Compare March 24, 2025 00:25
Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
Co-authored-by: Dooyong Kim <kdooyong@amazon.com>
@0ctopus13prime 0ctopus13prime force-pushed the lucene-on-faiss-part8 branch from 6132ada to 952723d Compare March 24, 2025 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants