Add ES93HnswVectorsFormat #6
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to Elasticsearch's vector search capabilities by adding Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughES 9.3 introduces ES93HnswVectorsFormat, a new HNSW vector format supporting BFloat16 encoding with configurable merge parameters. A bug fix updates BFloat16 vector writer to use correct off-heap storage. Service provider registration and comprehensive test coverage validate the implementation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ES93HnswVectorsFormat
participant ES93GenericFlatVectorsFormat
participant Lucene99HnswVectorsWriter
participant Lucene99HnswVectorsReader
Client->>ES93HnswVectorsFormat: new ES93HnswVectorsFormat()
ES93HnswVectorsFormat->>ES93GenericFlatVectorsFormat: new (default)
Note over ES93HnswVectorsFormat: Initialized with<br/>default NAME and flatVectorsFormat
Client->>ES93HnswVectorsFormat: fieldsWriter(state)
ES93HnswVectorsFormat->>Lucene99HnswVectorsWriter: new (state, maxConn,<br/>beamWidth, flatWriter, mergeSettings)
Lucene99HnswVectorsWriter-->>Client: KnnVectorsWriter
Client->>ES93HnswVectorsFormat: fieldsReader(state)
ES93HnswVectorsFormat->>Lucene99HnswVectorsReader: new (state, flatReader)
Lucene99HnswVectorsReader-->>Client: KnnVectorsReader
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple areas with mixed complexity: straightforward service registration and module configuration; a simple one-line bug fix in the BFloat16 writer; a moderate new class implementation with standard HNSW format integration; and comprehensive test classes with custom assertion logic requiring careful validation of float tolerance parsing and comparison semantics. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:L
|
/refacto-visz |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new non-quantized HNSW vectors format (ES93HnswVectorsFormat) that supports bfloat16 and directIO. The changes include modifications to module-info.java, ES93BFloat16FlatVectorsWriter.java, the creation of ES93HnswVectorsFormat.java, updates to KnnVectorsFormat, and the addition of ES93HnswBFloat16VectorsFormatTests.java and ES93HnswVectorsFormatTests.java. The review focuses on correctness and maintainability.
| fieldInfo.getVectorSimilarityFunction(), | ||
| new OffHeapFloatVectorValues.DenseOffHeapVectorValues( | ||
| new OffHeapBFloat16VectorValues.DenseOffHeapVectorValues( | ||
| fieldInfo.getVectorDimension(), |
There was a problem hiding this comment.
The class OffHeapBFloat16VectorValues is used here, suggesting that the file OffHeapFloatVectorValues is no longer needed and can be removed. Please confirm if this is the case and remove the import if it's no longer used.
| } | ||
|
|
||
| @Override | ||
| protected FlatVectorsFormat flatVectorsFormat() { |
There was a problem hiding this comment.
Consider renaming this method to getFlatVectorsFormat to follow Java naming conventions for getter methods. This improves readability and consistency.
| protected FlatVectorsFormat flatVectorsFormat() { | |
| protected FlatVectorsFormat getFlatVectorsFormat() { | |
| return flatVectorsFormat; | |
| } |
|
|
||
| @Override | ||
| public void testRandom() throws Exception { | ||
| AssertionError err = expectThrows(AssertionError.class, super::testRandom); |
There was a problem hiding this comment.
The test testRandom is expected to throw an AssertionError. It would be helpful to add a comment explaining why this is expected and what kind of assertions are being made within the assertFloatsWithinBounds method. This will improve the readability and maintainability of the test.
| private static void assertFloatsWithinBounds(AssertionError error) { | ||
| Matcher m = FLOAT_ASSERTION_FAILURE.matcher(error.getMessage()); | ||
| if (m.matches() == false) { | ||
| throw error; // nothing to do with us, just rethrow | ||
| } | ||
|
|
||
| // numbers just need to be in the same vicinity | ||
| double expected = Double.parseDouble(m.group(1)); | ||
| double actual = Double.parseDouble(m.group(2)); | ||
| double allowedError = expected * 0.01; // within 1% | ||
| assertThat(error.getMessage(), actual, closeTo(expected, allowedError)); |
There was a problem hiding this comment.
The allowed error is calculated as 1% of the expected value. While this might be sufficient, consider making this configurable or using a more robust method to determine the allowed error, especially if the expected values can be very small. A relative error might not be appropriate for values close to zero.
Add ES93HnswVectorsFormatTL;DR: Introduces a new non-quantized HNSW vectors format with bfloat16 storage and direct I/O support for improved vector search performance. Refacto PR SummaryImplements ES93HnswVectorsFormat extending AbstractHnswVectorsFormat with configurable bfloat16 encoding and direct I/O capabilities. Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant App as Application
participant Format as ES93HnswVectorsFormat
participant Writer as Lucene99HnswVectorsWriter
participant FlatFormat as ES93GenericFlatVectorsFormat
participant Storage as Vector Storage
App->>Format: Initialize with bfloat16=true, directIO=true
Format->>FlatFormat: Create with encoding options
App->>Format: fieldsWriter(SegmentWriteState)
Format->>Writer: Create with flatVectorsFormat writer
Writer->>FlatFormat: Write vectors with bfloat16 encoding
FlatFormat->>Storage: Store as 16-bit floats (reduced size)
Storage-->>App: Confirm storage with optimized memory usage
Testing GuideClick to expand
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| // numbers just need to be in the same vicinity | ||
| double expected = Double.parseDouble(m.group(1)); | ||
| double actual = Double.parseDouble(m.group(2)); | ||
| double allowedError = expected * 0.01; // within 1% |
There was a problem hiding this comment.
Suggestion: Use a relative tolerance based on the absolute magnitude and a small minimum epsilon so comparisons don't fail when the expected value is zero or very small. [possible bug]
| double allowedError = expected * 0.01; // within 1% | |
| // use absolute magnitude for relative tolerance and enforce a small minimum epsilon to handle expected == 0 | |
| double allowedError = Math.max(Math.abs(expected) * 0.01, 1e-6); // within 1% or at least 1e-6 |
Why Change? ⭐
This change is syntactically valid Java and uses only java.lang.Math methods, which are always available. It preserves the intended relative-tolerance behavior for non-zero expected values (allowedError = 1% of |expected|) while avoiding an overly strict allowedError of 0 when expected == 0 by enforcing a small epsilon (1e-6). The assertion call and surrounding parsing remain unchanged. There are no new imports or external dependencies, and the new computation does not introduce new exceptions for normal numeric inputs, so this is safe to apply and thus verified.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93BFloat16FlatVectorsWriter.java (2)
229-233: Fix incorrect class name in exception messageMismatch: references ES92BFloat16FlatVectorsWriter in ES93 class. Update message to avoid confusion.
- case BYTE -> throw new UnsupportedOperationException("ES92BFloat16FlatVectorsWriter only supports float vectors"); + case BYTE -> throw new UnsupportedOperationException("ES93BFloat16FlatVectorsWriter only supports float vectors");
369-375: ramBytesUsed undercounts; FLOAT32 stored as float[] (4B), not BF16 (2B)This can under-report memory and affect IW accounting. Use Float.BYTES for FLOAT32.
- int byteSize = fieldInfo.getVectorEncoding() == VectorEncoding.FLOAT32 - ? BFloat16.BYTES - : fieldInfo.getVectorEncoding().byteSize; + int byteSize = fieldInfo.getVectorEncoding() == VectorEncoding.FLOAT32 + ? Float.BYTES + : fieldInfo.getVectorEncoding().byteSize;
🧹 Nitpick comments (2)
server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormatTests.java (1)
109-113: Verify off-heap size keys and make casting robust
- Is "vex" the intended key? If not, typo will hide accounting issues.
- Prefer Number.longValue() to avoid brittle casts.
Example:
assertEquals((long) vector.length * bytes, ((Number) offHeap.get("vec")).longValue()); assertEquals(1L, ((Number) offHeap.get("vex")).longValue());server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormat.java (1)
24-66: Consider adding class and constructor javadoc.While the code is clear, adding javadoc comments would improve maintainability, especially for:
- Class-level description explaining ES93 format features (bfloat16, directIO support)
- Constructor parameter descriptions
- Relationship to underlying Lucene99 HNSW implementation
Example:
+/** + * ES 9.3 HNSW vectors format supporting BFloat16 encoding and direct I/O. + * Wraps Lucene 9.9 HNSW implementation with ES-specific flat vectors format. + */ public class ES93HnswVectorsFormat extends AbstractHnswVectorsFormat {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/src/main/java/module-info.java(1 hunks)server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93BFloat16FlatVectorsWriter.java(1 hunks)server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormat.java(1 hunks)server/src/main/resources/META-INF/services/org.apache.lucene.codecs.KnnVectorsFormat(1 hunks)server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswBFloat16VectorsFormatTests.java(1 hunks)server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormatTests.java(1 hunks)
🔇 Additional comments (11)
server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93BFloat16FlatVectorsWriter.java (1)
250-260: Correct off-heap BF16 usage in HNSW merge pathSwitch to OffHeapBFloat16VectorValues is correct (dimension/cardinality/stride align). LGTM.
server/src/main/resources/META-INF/services/org.apache.lucene.codecs.KnnVectorsFormat (1)
13-13: Registering ES93HnswVectorsFormatProvider entry is correct and consistent with module-info. LGTM.
server/src/main/java/module-info.java (1)
468-469: Module provider wiring looks correctES93HnswVectorsFormat properly added to KnnVectorsFormat providers. LGTM.
server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormat.java (8)
12-22: LGTM! Clean import structure.The imports are minimal, well-organized, and appropriate for an HNSW vectors format implementation. The use of Lucene 9.9 components (Lucene99HnswVectorsReader/Writer) is consistent with Elasticsearch's codec strategy.
24-26: LGTM! Proper class structure.The class correctly extends
AbstractHnswVectorsFormatand defines an appropriate NAME constant following ES versioning conventions.
30-33: LGTM! Clean default constructor.The no-arg constructor properly initializes with defaults, delegating configuration to
ES93GenericFlatVectorsFormat.
40-50: LGTM! Complete constructor with merge configuration.This constructor properly passes all parameters, including merge worker settings, to the parent class and initializes the flat vectors format with bfloat16 and directIO options.
52-55: LGTM! Standard getter override.The method correctly returns the encapsulated
flatVectorsFormatinstance.
57-60: LGTM! Correct writer construction.The method properly constructs a
Lucene99HnswVectorsWriterwith appropriate parameters and delegates flat vector writing to the underlying format.
62-65: LGTM! Correct reader construction.The method properly constructs a
Lucene99HnswVectorsReaderwith the segment state and delegates flat vector reading to the underlying format.
35-38: Verify merge parameter initialization in parent class.This constructor calls
super(NAME, maxConn, beamWidth)without explicitly passing merge parameters. However, line 59'sfieldsWriter()method referencesnumMergeWorkersandmergeExec. Ensure thatAbstractHnswVectorsFormat's 3-parameter constructor properly initializes these fields to safe default values (e.g., null formergeExec, default value fornumMergeWorkers) that are acceptable toLucene99HnswVectorsWriter.Run the following script to verify the parent class constructor:
| private static final Pattern FLOAT_ASSERTION_FAILURE = Pattern.compile(".*expected:<([0-9.-]+)> but was:<([0-9.-]+)>"); | ||
|
|
||
| private static void assertFloatsWithinBounds(AssertionError error) { | ||
| Matcher m = FLOAT_ASSERTION_FAILURE.matcher(error.getMessage()); | ||
| if (m.matches() == false) { | ||
| throw error; // nothing to do with us, just rethrow | ||
| } | ||
|
|
||
| // numbers just need to be in the same vicinity | ||
| double expected = Double.parseDouble(m.group(1)); | ||
| double actual = Double.parseDouble(m.group(2)); | ||
| double allowedError = expected * 0.01; // within 1% | ||
| assertThat(error.getMessage(), actual, closeTo(expected, allowedError)); | ||
| } |
There was a problem hiding this comment.
Fix tolerance calc and regex in BF16 float assertion helper
- allowedError must be non-negative; zero expected needs absolute fallback.
- Regex should handle scientific notation.
Apply:
- private static final Pattern FLOAT_ASSERTION_FAILURE = Pattern.compile(".*expected:<([0-9.-]+)> but was:<([0-9.-]+)>");
+ private static final Pattern FLOAT_ASSERTION_FAILURE = Pattern.compile(
+ ".*expected:<([-+]?\\d*\\.?\\d+(?:[eE][-+]?\\d+)?)> but was:<([-+]?\\d*\\.?\\d+(?:[eE][-+]?\\d+)?)>.*"
+ );
@@
- double allowedError = expected * 0.01; // within 1%
- assertThat(error.getMessage(), actual, closeTo(expected, allowedError));
+ double allowedError = Math.max(1e-6, Math.abs(expected) * 0.01); // within 1% or small abs floor
+ assertThat(error.getMessage(), actual, closeTo(expected, allowedError));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static final Pattern FLOAT_ASSERTION_FAILURE = Pattern.compile(".*expected:<([0-9.-]+)> but was:<([0-9.-]+)>"); | |
| private static void assertFloatsWithinBounds(AssertionError error) { | |
| Matcher m = FLOAT_ASSERTION_FAILURE.matcher(error.getMessage()); | |
| if (m.matches() == false) { | |
| throw error; // nothing to do with us, just rethrow | |
| } | |
| // numbers just need to be in the same vicinity | |
| double expected = Double.parseDouble(m.group(1)); | |
| double actual = Double.parseDouble(m.group(2)); | |
| double allowedError = expected * 0.01; // within 1% | |
| assertThat(error.getMessage(), actual, closeTo(expected, allowedError)); | |
| } | |
| private static final Pattern FLOAT_ASSERTION_FAILURE = Pattern.compile( | |
| ".*expected:<([-+]?\\d*\\.?\\d+(?:[eE][-+]?\\d+)?)> but was:<([-+]?\\d*\\.?\\d+(?:[eE][-+]?\\d+)?)>.*" | |
| ); | |
| private static void assertFloatsWithinBounds(AssertionError error) { | |
| Matcher m = FLOAT_ASSERTION_FAILURE.matcher(error.getMessage()); | |
| if (m.matches() == false) { | |
| throw error; // nothing to do with us, just rethrow | |
| } | |
| // numbers just need to be in the same vicinity | |
| double expected = Double.parseDouble(m.group(1)); | |
| double actual = Double.parseDouble(m.group(2)); | |
| double allowedError = Math.max(1e-6, Math.abs(expected) * 0.01); // within 1% or small abs floor | |
| assertThat(error.getMessage(), actual, closeTo(expected, allowedError)); | |
| } |
🤖 Prompt for AI Agents
In
server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswBFloat16VectorsFormatTests.java
around lines 85-98, the float-assertion helper uses a regex that doesn't accept
scientific notation and computes allowedError directly from expected which can
be negative or zero; update the regex to accept decimal and scientific notation
for both groups (e.g. allow digits, optional sign, decimal point and optional
exponent with [eE][+-]?\\d+), and compute allowedError as a non-negative value
using Math.abs(expected) * 0.01 with a small absolute fallback when expected is
zero (e.g. Math.max(Math.abs(expected) * 0.01, 1e-6)); keep parsing with
Double.parseDouble and assert with closeTo using the corrected allowedError.
Pull Request Feedback 🔍
|
| org.elasticsearch.index.codec.vectors.diskbbq.ES920DiskBBQVectorsFormat | ||
| org.elasticsearch.index.codec.vectors.diskbbq.next.ESNextDiskBBQVectorsFormat | ||
| org.elasticsearch.index.codec.vectors.es93.ES93BinaryQuantizedVectorsFormat | ||
| org.elasticsearch.index.codec.vectors.es93.ES93HnswVectorsFormat |
There was a problem hiding this comment.
Suggestion: Verify that the fully-qualified name org.elasticsearch.index.codec.vectors.es93.ES93HnswVectorsFormat exactly matches the implementation class (package + class name), that the class is declared public and exposes the expected SPI-compatible constructor/signature, and confirm the class is included in the built JAR so ServiceLoader can load it at runtime. [possible issue]
|
CodeAnt AI finished reviewing your PR. |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Vector Format Implementation👍 Well Done
📁 Selected files for review (6)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| new OffHeapBFloat16VectorValues.DenseOffHeapVectorValues( |
There was a problem hiding this comment.
Missing Import Statement
Usage of OffHeapBFloat16VectorValues class without visible import statement. Runtime ClassNotFoundException will occur if import missing causing vector indexing failures. Service degradation results from failed vector operations.
import org.elasticsearch.index.codec.vectors.es93.OffHeapBFloat16VectorValues;
Commitable Suggestion
| new OffHeapBFloat16VectorValues.DenseOffHeapVectorValues( | |
| import org.elasticsearch.index.codec.vectors.es93.OffHeapBFloat16VectorValues; |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| public ES93HnswVectorsFormat( | ||
| int maxConn, | ||
| int beamWidth, | ||
| boolean bfloat16, | ||
| boolean useDirectIO, | ||
| int numMergeWorkers, | ||
| ExecutorService mergeExec | ||
| ) { | ||
| super(NAME, maxConn, beamWidth, numMergeWorkers, mergeExec); | ||
| flatVectorsFormat = new ES93GenericFlatVectorsFormat(bfloat16, useDirectIO); | ||
| } |
There was a problem hiding this comment.
Executor Validation Gap
Constructor accepts ExecutorService parameter without null validation before passing to superclass. NullPointerException during vector merging operations if null executor provided. Vector indexing failures result from unhandled null executor state.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- DbC-Preconditions
2 participants
User description
Add a non-quantized HNSW vectors format supporting bfloat16 and directIO
CodeAnt-AI Description
Add an HNSW vectors format that supports bfloat16 storage and direct I/O
What Changed
Impact
✅ HNSW indexing with optional bfloat16 encoding✅ Lower storage per vector when bfloat16 is enabled✅ New tests improve confidence and tolerate bfloat16 numeric variance💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
New Features
Tests