Skip to content

Adds new unexposed and experimental IVF format #127528

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
May 7, 2025
Merged

Adds new unexposed and experimental IVF format #127528

merged 21 commits into from
May 7, 2025

Conversation

@benwtrent
Copy link
Member

@benwtrent benwtrent commented Apr 29, 2025

No description provided.

@benwtrent benwtrent requested a review from Copilot April 29, 2025 18:58
Copilot AI reviewed Apr 29, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds an experimental IVF vectors format along with its supporting reader/writer implementations, tests, modifications to the neighbor queue, and updates to the SIMD vectorization utilities.

  • Adds experimental IVF format implementations and tests.
  • Extends SIMD vectorization support with OSQ scoring and associated tests.
  • Updates module exports to include IVFVectorsFormat.

Reviewed Changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated no comments.

Show a summary per file
File Description
server/src/test/java/org/elasticsearch/index/codec/vectors/IVFVectorsFormatTests.java New tests for the experimental IVF vectors format.
server/src/main/java/org/elasticsearch/index/codec/vectors/NeighborQueue.java Introduction of a custom neighbor queue implementation.
server/src/main/java/org/elasticsearch/index/codec/vectors/IVFVectorsReader.java New reader implementation for IVF vectors indexing.
server/src/main/java/org/elasticsearch/index/codec/vectors/IVFVectorsFormat.java Implementation of the IVF vectors format with feature-flag checks.
server/src/main/java/module-info.java Updated module export to include IVFVectorsFormat.
libs/simdvec/... Enhancements to OSQ-based vector scoring and utility support across SIMD vectorization providers.
Files not reviewed (1)
  • server/src/main/resources/META-INF/services/org.apache.lucene.codecs.KnnVectorsFormat: Language not supported
Comments suppressed due to low confidence (2)

server/src/main/java/org/elasticsearch/index/codec/vectors/IVFVectorsFormat.java:103

  • The toString() method returns "IVFVectorFormat", which is inconsistent with the class name IVFVectorsFormat. Consider changing it to "IVFVectorsFormat" for consistency.
return "IVFVectorFormat";

libs/simdvec/src/main/java/org/elasticsearch/simdvec/internal/vectorization/DefaultESVectorUtilSupport.java:148

  • The fma() method is used without being defined. To improve clarity and correctness, consider using Math.fma(djk, originalResidual[i], proj) or ensure that a proper helper method is provided.
proj = fma(djk, originalResidual[i], proj);

@benwtrent benwtrent marked this pull request as ready for review April 29, 2025 19:02
@elasticsearchmachine elasticsearchmachine added the Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch label Apr 29, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Apr 29, 2025

Pinging @elastic/es-search-relevance (Team:Search Relevance)

@benwtrent benwtrent requested a review from mayya-sharipova May 6, 2025 12:00
john-wagster
john-wagster reviewed May 7, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/index/codec/vectors/NeighborQueue.java
/**
* Copied from and modified from Apache Lucene.
*/
class NeighborQueue {
Copy link
Contributor

@john-wagster john-wagster May 7, 2025

Choose a reason for hiding this comment

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

since you are pulling over NeighborQueue consider pulling over TestNeighborQueue as well?

john-wagster
john-wagster approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@john-wagster john-wagster left a comment

Choose a reason for hiding this comment

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

i've taken a few passes over the code here at this point. I feel like it's in a good enough state to merge and to start getting CI running on it. lgtm.

carlosdelest
carlosdelest approved these changes May 7, 2025
View reviewed changes
Copy link
Member

@carlosdelest carlosdelest left a comment

Choose a reason for hiding this comment

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

From a code structure and testing perspective, this LGTM.

I can only rubber stamp the codec level / algorithmic side of it as I'm not familiar with that. But I trust the testing infrastructure and how it's been extended.

mayya-sharipova
mayya-sharipova reviewed May 7, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/index/codec/vectors/DefaultIVFVectorsReader.java Outdated
return centroid;
}

private void readQuantizedCentroid(int centroidOrdinal) throws IOException {
Copy link
Contributor

@mayya-sharipova mayya-sharipova May 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the function name is readQuantizedCentroid but it also reads full centroid. It looks like the second read of full centroid is not necessary in scoring. Should we break this function be only about reading quantized centroid?

mayya-sharipova
mayya-sharipova reviewed May 7, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/index/codec/vectors/DefaultIVFVectorsReader.java
@Override
NeighborQueue scorePostingLists(FieldInfo fieldInfo, KnnCollector knnCollector, CentroidQueryScorer centroidQueryScorer, int nProbe)
throws IOException {
NeighborQueue neighborQueue = new NeighborQueue(centroidQueryScorer.size(), true);
Copy link
Contributor

@mayya-sharipova mayya-sharipova May 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Are you supposed to have MIN heap of only nprobe size?
What's the role of nprobe here?

Copy link
Member Author

@benwtrent benwtrent May 7, 2025

Choose a reason for hiding this comment

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

For this, we need to be able to explore past nProbe in the case of filtered search. nProbe has no current role.

I think we will end up with a double queue system eventually, but given its all flat, we just score everything. This needs to be improved, for sure.

mayya-sharipova
mayya-sharipova reviewed May 7, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/index/codec/vectors/IVFVectorsReader.java
++centroidsVisited;
// todo do we actually need to know the score???
int centroidOrdinal = centroidQueue.pop();
// todo do we need direct access to the raw centroid???
Copy link
Contributor

@mayya-sharipova mayya-sharipova May 7, 2025

Choose a reason for hiding this comment

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

+1 for this TODO: it looks like we always do 2 reads of quantized and full centroid, why we need only a single

Copy link
Member Author

@benwtrent benwtrent May 7, 2025

Choose a reason for hiding this comment

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

We need the raw to quantize the query vector for the posting list. So, we score with the quantized and then quantize the query vector according to the raw value.

I think we might want to separate out the scoring vs. the iterating for the centroids.

mayya-sharipova
mayya-sharipova approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

@benwtrent Amazing work, Ben and the team! I've left some minor comments, but overall LGTM!

@benwtrent benwtrent merged commit ebe8ea6 into elastic:main May 7, 2025
17 checks passed
@benwtrent benwtrent deleted the ivf-format branch May 7, 2025 19:00
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request May 9, 2025
@benwtrent @ywangd
Adds new unexposed and experimental IVF format (elastic#127528)
4427d92
john-wagster added a commit that referenced this pull request May 9, 2025
@john-wagster
Revert "Adds new unexposed and experimental IVF format (#127528)"
b4deeb5 
This reverts commit ebe8ea6.
@john-wagster john-wagster mentioned this pull request May 9, 2025
Merged
john-wagster added a commit that referenced this pull request May 9, 2025
@john-wagster
Revert "Adds new unexposed and experimental IVF format (#127528)" (#1…
8a17a5e 
…28005)

This reverts commit ebe8ea6.
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request May 12, 2025
@benwtrent @jfreden
Adds new unexposed and experimental IVF format (elastic#127528)
5e08a39
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request May 12, 2025
@john-wagster @jfreden
Revert "Adds new unexposed and experimental IVF format (elastic#127528)…
7b805ea 
…" (elastic#128005)

This reverts commit ebe8ea6.
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request May 13, 2025
@benwtrent
Reapply "Adds new unexposed and experimental IVF format (elastic#127528…
5d3df96 
…)" (elastic#128005)

This reverts commit 8a17a5e.
elasticsearchmachine pushed a commit that referenced this pull request May 13, 2025
@benwtrent
Reapply "Adds new unexposed and experimental IVF format (#127528)" (#…
1324ee0 
…128005) (#128051)

This reverts commit 8a17a5e.

reapplying ivf format, but with a fix.
richard-dennehy pushed a commit to richard-dennehy/elasticsearch that referenced this pull request May 19, 2025
@benwtrent @richard-dennehy
Reapply "Adds new unexposed and experimental IVF format (elastic#127528…
a239ca8 
…)" (elastic#128005) (elastic#128051)

This reverts commit 8a17a5e.

reapplying ivf format, but with a fix.
benchaplin pushed a commit to benchaplin/elasticsearch that referenced this pull request May 20, 2025
@benwtrent @benchaplin
Reapply "Adds new unexposed and experimental IVF format (elastic#127528…
ce90f37 
…)" (elastic#128005) (elastic#128051)

This reverts commit 8a17a5e.

reapplying ivf format, but with a fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@mayya-sharipova mayya-sharipova mayya-sharipova approved these changes

@carlosdelest carlosdelest carlosdelest approved these changes

@john-wagster john-wagster john-wagster approved these changes

Assignees

No one assigned

Labels

>non-issue :Search Relevance/Vectors Vector search Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benwtrent @elasticsearchmachine @mayya-sharipova @carlosdelest @john-wagster