Skip to content

Conversation

@akhilesh-k
Copy link

Description

This PR updates the decision logic in filtered vector search to leverage the new expectedVisitedNodes(int k, int size) heuristic introduced in #14836.

The existing flow for filtered vector search is discussed in this issue comment: #14845 (comment)

The previous heuristic filterCost <= perLeafK for triggering exact search could cause unnecessary approximate searches which may stop early due to filterCost <= expectedVisitedNodes(perLeafK, size) and then require a fallback exact search.

By using expectedVisitedNodes, which estimates the number of visited nodes based on both k and graph size, we can more accurately predict query cost and bypass unnecessary approximate searches.

@github-actions
Copy link
Contributor

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

@akhilesh-k
Copy link
Author

Hi @jpountz would it be possible to review this PR please?

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thank you, the logic looks good to me.

My main concern is that AbstractKnnVectorQuery, which should be agnostic of the vector search index, now imports some HNSW-specific logic. We should be able to fix this when @shubhamvishu completes the work of pushing down the decision logic for exact vs. approximate to the codec (see discussion on #14963). I'd suggest to wait until this other work is complete before merging this PR.

}

BitSet acceptDocs = createBitSet(scorer.iterator(), liveDocs, reader.maxDoc());
int documentSize = reader.maxDoc();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you call it maxDoc instead?

if (cost <= expectedVisitedNodesEstimate) {
// If there are <= expectedVisitedNodes possible matches, short-circuit and perform exact
// search, since
// HNSW must always visit at least expectedVisitedNodes documents
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's fix this comment to say that HNSW likely needs to visit this many documents, rather than guaranteeing that it needs to visit this many vectors?

Copy link
Author

Choose a reason for hiding this comment

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

Yes makes sense, have updated the PR to address this.

@akhilesh-k
Copy link
Author

Thank you for your review @jpountz. I will wait for the PR #14963 to be merged and will fix up conflicts later.

@github-actions
Copy link
Contributor

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

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.

2 participants