Skip to content

Remove duplicated vector storage and rely on inline vectors for RAVV#145

Closed
tjake wants to merge 11 commits into
opensearch-project:mainfrom
tjake:ravv-from-inline-vectors
Closed

Remove duplicated vector storage and rely on inline vectors for RAVV#145
tjake wants to merge 11 commits into
opensearch-project:mainfrom
tjake:ravv-from-inline-vectors

Conversation

@tjake

@tjake tjake commented Aug 28, 2025

Copy link
Copy Markdown
Collaborator

This pull request refactors the JVector codec implementation to remove dependencies on Lucene's flat vector format and related classes, streamlining the codebase and improving performance and memory management. The changes replace the use of FlatVectorsWriter and FlatVectorsReader with internal data structures for handling float vectors, and update reader and writer logic accordingly.

Most importantly this removes the second copy of vectors which was causing a doubling of disk usage per index.

Removal of Lucene Flat Vectors Dependencies

  • Eliminated all usage of FlatVectorsWriter, FlatVectorsReader, and related classes from JVectorWriter, JVectorReader, and supporting classes, replacing them with direct management of float vectors using internal lists and utility methods. [1] [2] [3] [4] [5]

Refactoring Writer Logic

  • Updated the FieldWriter class to store vectors in a List<VectorFloat<?>> instead of delegating to FlatFieldVectorsWriter, and adjusted vector addition and memory usage calculations to use the new structure. [1] [2] [3]
  • Modified merge and flush operations to remove flat vector handling, focusing solely on the graph index and internal vector list. [1] [2] [3]

Refactoring Reader Logic

  • Updated JVectorReader to remove references to flat vector reading and merging logic, simplifying float vector retrieval and integrity checks. [1] [2] [3] [4]

Improvements to Vector Access and Memory Management

  • Changed random access vector classes to use the new internal vector lists, improved thread safety, and clarified ownership semantics for shared values. [1] [2] [3] [4]
  • Optimized vector retrieval in JVectorFloatVectorValues to reuse a single vector object, reducing allocations. [1] [2]

Miscellaneous Updates

  • Restored the @Override annotation for readLong() in JVectorRandomAccessReader to match the updated jVector version.### Description
    [Describe what this change achieves]

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.

Signed-off-by: Jake Luciani <jake@datastax.com>
@tjake

tjake commented Sep 12, 2025

Copy link
Copy Markdown
Collaborator Author

Before PR:

-rw-r--r--. 1 jake jake 1585966 Sep 12 10:48 _2_JVectorFormat_0_test_field.data-jvector
-rw-r--r--. 1 jake jake  640088 Sep 12 10:48 _2_JVectorFormat_0.vec
-rw-r--r--. 1 jake jake  660073 Sep 12 10:48 _2_Lucene90_0.dvd

After First Commit: .vec is removed saving 1 full copy of vectors (10k 16 vectors):

-rw-r--r--. 1 jake jake 1585966 Sep 11 17:48 _2_JVectorFormat_0_test_field.data-jvector
-rw-r--r--. 1 jake jake  660073 Sep 11 17:47 _2_Lucene90_0.dvd

After Second Commit: .dvd binary vectors are removed saving 1 full copy of vectors (10k 16d vectors)

-rw-r--r--. 1 jake jake 1585966 Sep 11 17:36 _2_JVectorFormat_0_test_field.data-jvector
-rw-r--r--. 1 jake jake   20073 Sep 11 17:36 _2_Lucene90_0.dvd

Signed-off-by: Jake Luciani <jake@datastax.com>
@tjake
tjake force-pushed the ravv-from-inline-vectors branch from 3147fd7 to 61bf31a Compare September 12, 2025 15:01
Jake Luciani added 6 commits September 12, 2025 11:01
Signed-off-by: Jake Luciani <jake@datastax.com>
Signed-off-by: Jake Luciani <jake@datastax.com>
Signed-off-by: Jake Luciani <jake@datastax.com>
Signed-off-by: Jake Luciani <jake@datastax.com>
Signed-off-by: Jake Luciani <jake@datastax.com>
@Override
public void addBinaryField(FieldInfo field, DocValuesProducer valuesProducer) throws IOException {
delegatee.addBinaryField(field, valuesProducer);
if (!(field.hasVectorValues() && extractKNNEngine(field) == KNNEngine.JVECTOR)) delegatee.addBinaryField(field, valuesProducer);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should we just change the conditions for isKNNBinaryFieldRequired() method?
We can place the condition exception for jVector engine there, and also avoid binary field creation altogether for FP vectors.

assert mergeState != null;
assert mergeState.mergeFieldInfos != null;

for (DocValuesProducer docValuesProducer : mergeState.docValuesProducers) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we just remove doc values all together for KNN fields?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think perhaps what we can do is actually to leverage the "DerivedSourceFormat", glancing at the plugin it seems like it's already there.
https://github.com/opensearch-project/opensearch-jvector/blob/main/src/main/java/org/opensearch/knn/index/codec/KNN9120Codec/DerivedSourceStoredFieldsFormat.java
Do you want to try and test with the derived source for the index and see if it achieves the same result?

@sam-herman

sam-herman commented Sep 15, 2025

Copy link
Copy Markdown
Collaborator

Hi @tjake I think in order to remove the doc values we will need to introduce derviedSource such as:
opensearch-project/k-NN#2377
For unblocking, should we remove the lucene flat vector format in this change and work on derivedSource in a different one? Or you prefer to continue to derived source in this change?

@sam-herman

Copy link
Copy Markdown
Collaborator

Changes from this PR were augmented to #167 and merged

@sam-herman sam-herman closed this Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants