Added capability to retrieve float data type vectors using doc_values

[navneet1v]

Description

Added capability to retrieve float data type vectors using doc_values. More details on small scale benchmarks can be found here: #3315

Next Steps:

1. Added the support for byte and binary data types.

When this is useful?

1. If a user wants to get the vector field in the response, but don't want to pay for reading of whole source, then they can user this way to get the vectors.
2. They can also combine with source.excludes since during source excludes for vector field we don't read vectors and inject them back in _source.

Related Issues

Partially resolves: #3315

Example

POST /my_index/_search
  {
    "query": {
      "knn": {
        "my_vector": {
          "vector": [1.0, 2.0, 3.0, 4.0],
          "k": 5
        }
      }
    },
    "docvalue_fields": ["my_vector"],
    "_source": false
  }

This returns the vectors directly from doc values without reading _source, which is faster — especially for large vectors. The response looks like:

  {
    "hits": {
      "hits": [
        {
          "_id": "1",
          "_score": 0.95,
          "fields": {
            "my_vector": [[1.0, 2.0, 3.0, 4.0]]
          }
        }
      ]
    }
  }

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 37b9942)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue In getLeafValueFetcher, if vectorValues.advance(docId) throws an IOException, the exception is not caught. The method signature does not declare throws IOException, so any IOException from advance() will be wrapped in an unchecked exception at runtime. This can occur when reading corrupted segment data or encountering I/O errors during vector retrieval. if (vectorValues.advance(docId) == docId) { count = 1; return true; } count = 0; return false; } Possible Issue In getLeafValueFetcher, the count variable is set to 1 when advanceExact succeeds, but nextValue() can be called multiple times if docValueCount() returns 1. Each call to nextValue() invokes vectorValues.conditionalCloneVector(), which may advance the iterator or return stale data depending on implementation. If a caller invokes nextValue() twice for the same document, the second call may return incorrect data or throw an exception. public Object nextValue() throws IOException { return vectorValues.conditionalCloneVector(); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 37b9942

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate state before returning vector The nextValue() method does not verify that advanceExact() was called successfully before accessing the vector. If called without a prior successful advance, it may return stale or incorrect vector data from the iterator's previous position. src/main/java/org/opensearch/knn/index/KNNVectorDVLeafFieldData.java [153-156] @Override public Object nextValue() throws IOException { + if (count == 0) { + throw new IllegalStateException("nextValue() called without successful advanceExact()"); + } return vectorValues.conditionalCloneVector(); } Suggestion importance[1-10]: 5 __ Why: Adding a state check in nextValue() would improve robustness by catching misuse of the API (calling nextValue() without a successful advanceExact()). However, this is a defensive programming practice rather than fixing an actual bug, as the DocValueFetcher.Leaf contract expects callers to follow the proper sequence. The score reflects moderate value for API safety.	Low

---

Previous suggestions

Suggestions up to commit 3c65d2a

Category	Suggestion	Impact
General	Add test index cleanup The test methods do not clean up the TEST_INDEX after execution, which can cause test pollution and failures when tests run in sequence. Add deleteKNNIndex(TEST_INDEX) in a tearDown method or at the end of each test to ensure proper cleanup. src/test/java/org/opensearch/knn/integ/DocValueFieldsIT.java [52-71] @SneakyThrows public void testDocValueFields_faissHnsw_returnsVectorWithoutSource() { - createHnswIndex(KNNEngine.FAISS); - indexTestDocuments(); - ... + try { + createHnswIndex(KNNEngine.FAISS); + indexTestDocuments(); + ... + } finally { + deleteKNNIndex(TEST_INDEX); + } } Suggestion importance[1-10]: 7 __ Why: This is a valid concern for integration tests. The test methods reuse TEST_INDEX without cleanup, which could cause test pollution. However, many tests in the file already use unique index names (e.g., TEST_INDEX + "_high_dim"), and some tests do call deleteKNNIndex. Adding cleanup would improve test isolation and reliability, though the impact is moderate since test frameworks often provide cleanup hooks.	Medium
	Validate state before returning value The nextValue() method is called without verifying that advanceExact returned true or that docValueCount() is greater than zero. If called inappropriately, this could return stale or incorrect vector data. Consider adding a guard to throw an exception if called when no value is available. src/main/java/org/opensearch/knn/index/KNNVectorDVLeafFieldData.java [153-156] @Override public Object nextValue() throws IOException { + if (count == 0) { + throw new IllegalStateException("nextValue() called when no value is available"); + } return vectorValues.getVector(); } Suggestion importance[1-10]: 5 __ Why: Adding a guard to validate that count > 0 before calling nextValue() would improve robustness and catch misuse of the API. However, the DocValueFetcher.Leaf contract expects callers to respect docValueCount() and only call nextValue() the appropriate number of times. This is a defensive programming improvement rather than fixing a critical bug in the current implementation.	Low
Possible issue	Prevent iterator state corruption The KNNVectorValues iterator is created once during getLeafValueFetcher and shared across all advanceExact calls. If the same Leaf instance is reused for multiple documents, the iterator state may become corrupted. Consider creating the iterator lazily inside advanceExact or document that each Leaf instance must only be used once. src/main/java/org/opensearch/knn/index/KNNVectorDVLeafFieldData.java [128-133] -final KNNVectorValues<?> vectorValues; -try { - vectorValues = KNNVectorValuesFactory.getVectorValues(fieldInfo, Lucene.segmentReader(reader)); -} catch (IOException e) { - throw new IllegalStateException("Cannot load vector values for field: " + fieldName, e); -} +return new DocValueFetcher.Leaf() { + private int count; + private KNNVectorValues<?> vectorValues; + @Override + public boolean advanceExact(int docId) throws IOException { + if (vectorValues == null) { + vectorValues = KNNVectorValuesFactory.getVectorValues(fieldInfo, Lucene.segmentReader(reader)); + } + if (vectorValues.advance(docId) == docId) { + count = 1; + return true; + } + count = 0; + return false; + } + ... +}; + Suggestion importance[1-10]: 3 __ Why: The concern about iterator state corruption is valid in theory, but the PR's design explicitly creates a fresh KNNVectorValues iterator per getLeafValueFetcher call (line 130), and the javadoc states "Each call creates its own iterator" (lines 101-103). The Leaf instance is not intended to be reused across multiple fetcher calls. The suggested lazy initialization adds complexity without clear benefit given the current usage pattern.	Low

Suggestions up to commit 7c9f1c8

Category	Suggestion	Impact
General	Validate state before returning value The nextValue() method is called after advanceExact() returns true, but there's no validation that count > 0 before calling getVector(). If advanceExact() returned false and nextValue() is mistakenly called, this could return stale or incorrect data. Add a guard to prevent misuse. src/main/java/org/opensearch/knn/index/KNNVectorDVLeafFieldData.java [154-156] @Override public Object nextValue() throws IOException { + if (count == 0) { + throw new IllegalStateException("nextValue() called without successful advanceExact()"); + } + count--; return vectorValues.getVector(); } Suggestion importance[1-10]: 5 __ Why: Adding a guard to prevent calling nextValue() without a successful advanceExact() improves robustness and prevents potential misuse. The suggestion to decrement count ensures that nextValue() can only be called once per document, which aligns with the DocValueFetcher.Leaf contract. This is a reasonable defensive programming practice.	Low
	Ensure separate segment creation The test assumes that indexing with refresh=true after a prior refreshIndex will create separate segments, but this is not guaranteed. The documents may still end up in the same segment. Use flush() between documents or explicitly control segment creation to ensure the test validates the intended scenario. src/test/java/org/opensearch/knn/integ/DocValueFieldsIT.java [867-873] -public void testDocValueFields_docsWithoutVectorField_returnsEmptyFields() throws IOException { - ... - // Index a doc WITH the vector field - addKnnDoc(indexName, "1", VECTOR_FIELD, Floats.asList(VECTOR_1).toArray()); - refreshIndex(indexName); +addKnnDoc(indexName, "1", VECTOR_FIELD, Floats.asList(VECTOR_1).toArray()); +refreshIndex(indexName); +forceMergeKnnIndex(indexName, 1); - // Index a doc WITHOUT the vector field (in a separate segment) - Request request = new Request("POST", "/" + indexName + "/_doc/2?refresh=true"); - request.setJsonEntity("{\"title\": \"no vector here\"}"); - client().performRequest(request); +Request request = new Request("POST", "/" + indexName + "/_doc/2?refresh=true"); +request.setJsonEntity("{\"title\": \"no vector here\"}"); +client().performRequest(request); Suggestion importance[1-10]: 4 __ Why: While the suggestion correctly identifies that segment creation is not guaranteed, the test's intent is to verify behavior when a document lacks the vector field, not specifically to test multi-segment scenarios. The current approach with refresh=true is sufficient for the test's purpose. However, adding forceMergeKnnIndex could make the test more deterministic if segment isolation is critical.	Low
Possible issue	Prevent iterator state corruption The vectorValues iterator is created once during getLeafValueFetcher invocation but is shared across all advanceExact calls. If the same Leaf instance is reused for multiple documents, the iterator state may become corrupted. Consider creating the iterator lazily inside advanceExact or document that each Leaf instance must only be used once. src/main/java/org/opensearch/knn/index/KNNVectorDVLeafFieldData.java [128-133] -final KNNVectorValues<?> vectorValues; -try { - vectorValues = KNNVectorValuesFactory.getVectorValues(fieldInfo, Lucene.segmentReader(reader)); -} catch (IOException e) { - throw new IllegalStateException("Cannot load vector values for field: " + fieldName, e); -} +return new DocValueFetcher.Leaf() { + private int count; + private KNNVectorValues<?> vectorValues; + @Override + public boolean advanceExact(int docId) throws IOException { + if (vectorValues == null) { + vectorValues = KNNVectorValuesFactory.getVectorValues(fieldInfo, Lucene.segmentReader(reader)); + } + if (vectorValues.advance(docId) == docId) { + count = 1; + return true; + } + count = 0; + return false; + } + ... +}; + Suggestion importance[1-10]: 2 __ Why: The concern about iterator state corruption is unfounded. The PR documentation explicitly states that each call to getLeafValueFetcher creates its own KNNVectorValues iterator, ensuring isolation. The Leaf instance is designed to be used once per search request, and the iterator is correctly initialized before use. The suggested lazy initialization adds unnecessary complexity without addressing a real issue.	Low

Suggestions up to commit 3f72c3d

Category	Suggestion	Impact
Possible issue	Prevent shared iterator state corruption The vectorValues iterator state is shared across all DocValueFetcher.Leaf instances returned by this method. If getLeafValueFetcher() is called multiple times (which is possible in certain search scenarios), subsequent calls will receive a Leaf that captures an already-advanced iterator, leading to incorrect results or skipped documents. Create a fresh iterator for each Leaf instance to ensure isolation. src/main/java/org/opensearch/knn/index/KNNVectorDVLeafFieldData.java [124-155] @Override public DocValueFetcher.Leaf getLeafValueFetcher(final DocValueFormat format) { if (vectorDataType == VectorDataType.BYTE || vectorDataType == VectorDataType.BINARY) { throw new UnsupportedOperationException( "docvalue_fields is not supported for [" + vectorDataType + "] vector field '" + fieldName + "'" ); } - return new DocValueFetcher.Leaf() { - private int count; + try { + final KNNVectorValues<?> leafVectorValues = KNNVectorValuesFactory.getVectorValues(fieldInfo, Lucene.segmentReader(reader)); + return new DocValueFetcher.Leaf() { + private int count; - @Override - public boolean advanceExact(int docId) throws IOException { - if (vectorValues.advance(docId) == docId) { - count = 1; - return true; + @Override + public boolean advanceExact(int docId) throws IOException { + if (leafVectorValues.advance(docId) == docId) { + count = 1; + return true; + } + count = 0; + return false; } - count = 0; - return false; - } - ... - }; + + @Override + public int docValueCount() { + return count; + } + + @Override + public Object nextValue() throws IOException { + return leafVectorValues.getVector(); + } + }; + } catch (IOException e) { + throw new IllegalStateException("Cannot create vector values iterator for field: " + fieldName, e); + } } Suggestion importance[1-10]: 9 __ Why: This identifies a critical bug. If getLeafValueFetcher() is called multiple times on the same instance, all returned Leaf instances share the same vectorValues iterator. Advancing one Leaf would corrupt the state for others, causing incorrect results or skipped documents. Creating a fresh iterator per Leaf ensures isolation and correctness.	High
General	Defer vector values initialization The constructor eagerly initializes vectorValues for all instances, even when they may never be used (e.g., when only getScriptValues() is called). This can cause unnecessary I/O overhead and resource consumption. Consider lazy initialization of vectorValues only when getLeafValueFetcher() is invoked, or add a factory method to create instances with different initialization strategies based on the expected usage pattern. src/main/java/org/opensearch/knn/index/KNNVectorDVLeafFieldData.java [46-59] +private KNNVectorValues<?> vectorValues; + public KNNVectorDVLeafFieldData(LeafReader reader, String fieldName, VectorDataType vectorDataType) { this.reader = reader; this.fieldName = fieldName; this.vectorDataType = vectorDataType; this.fieldInfo = reader.getFieldInfos().fieldInfo(fieldName); if (this.fieldInfo == null) { throw new IllegalStateException("Field info not found for field: " + fieldName); } - try { - this.vectorValues = KNNVectorValuesFactory.getVectorValues(fieldInfo, Lucene.segmentReader(reader)); - } catch (IOException e) { - throw new IllegalStateException("Cannot load vector values for field: " + fieldName, e); - } } +private synchronized KNNVectorValues<?> getVectorValues() throws IOException { + if (this.vectorValues == null) { + this.vectorValues = KNNVectorValuesFactory.getVectorValues(fieldInfo, Lucene.segmentReader(reader)); + } + return this.vectorValues; +} + Suggestion importance[1-10]: 4 __ Why: The suggestion to defer initialization is valid for reducing overhead when vectorValues is never used. However, the PR's javadoc explicitly states that eager initialization is intentional to avoid per-document overhead on the hot path. The suggestion contradicts this design decision and adds synchronization overhead. The improvement is marginal given the single-threaded, short-lived nature of these instances.	Low

[github-actions]

Persistent review updated to latest commit 7c9f1c8

[github-actions]

Persistent review updated to latest commit 3c65d2a

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.46%. Comparing base (c31661c) to head (37b9942).

Files with missing lines	Patch %	Lines
...opensearch/knn/index/KNNVectorDVLeafFieldData.java	85.71%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3321      +/-   ##
============================================
- Coverage     83.53%   83.46%   -0.08%     
  Complexity     4282     4282              
============================================
  Files           450      450              
  Lines         15531    15552      +21     
  Branches       2013     2015       +2     
============================================
+ Hits          12974    12980       +6     
- Misses         1766     1779      +13     
- Partials        791      793       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 37b9942

[navneet1v]

@shatejas improved the tests please check

[github-actions]

Persistent review updated to latest commit 37b9942