Add ES93HnswVectorsFormat #11
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 · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
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 integrates a new HNSW (Hierarchical Navigable Small World) vector format, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
codeant-ai
bot
added
the
size:L
|
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. WalkthroughThis PR introduces ES93HnswVectorsFormat, a new HNSW (hierarchical navigable small world) vector format implementation for Elasticsearch 9.3. The implementation delegates to Lucene99-based reader/writer components and is registered as a service provider. Changes include the format class, module configuration, service provider registration, and comprehensive test coverage for standard and BFloat16 encoding scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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 the ES93HnswVectorsFormat for HNSW vector indexing, along with corresponding tests. The changes are well-structured and follow the existing patterns for adding new vector formats. My review includes suggestions to improve maintainability by refactoring constructors, enhancing the robustness of test assertions, and clarifying a cryptic key used in off-heap memory reporting.
| assertFloatsWithinBounds(err); | ||
| } | ||
|
|
||
| private static final Pattern FLOAT_ASSERTION_FAILURE = Pattern.compile(".*expected:<([0-9.-]+)> but was:<([0-9.-]+)>"); |
There was a problem hiding this comment.
The regular expression used to parse floating-point numbers from the assertion error message is not robust enough. It will fail to parse numbers represented in scientific notation (e.g., 1.23E-4), which can be produced by Float.toString() for very large or small values. This can cause the test to misinterpret assertion failures. Using a more comprehensive regex for floating-point numbers will make this test more reliable.
| 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:<([-+]?[0-9]*\\.?[0-9]+(?:[eE][-+]?[0-9]+)?)> but was:<([-+]?[0-9]*\\.?[0-9]+(?:[eE][-+]?[0-9]+)?)>"); |
| public ES93HnswVectorsFormat() { | ||
| super(NAME); | ||
| flatVectorsFormat = new ES93GenericFlatVectorsFormat(); | ||
| } | ||
|
|
||
| public ES93HnswVectorsFormat(int maxConn, int beamWidth, boolean bfloat16, boolean useDirectIO) { | ||
| super(NAME, maxConn, beamWidth); | ||
| flatVectorsFormat = new ES93GenericFlatVectorsFormat(bfloat16, useDirectIO); | ||
| } | ||
|
|
||
| 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.
The constructors in this class have duplicated logic for initializing flatVectorsFormat. You can refactor them to use constructor chaining, which will reduce code duplication and improve maintainability.
public ES93HnswVectorsFormat() {
this(org.apache.lucene.codecs.lucene99.Lucene99HnswVectorsFormat.DEFAULT_MAX_CONN, org.apache.lucene.codecs.lucene99.Lucene99HnswVectorsFormat.DEFAULT_BEAM_WIDTH, false, false);
}
public ES93HnswVectorsFormat(int maxConn, int beamWidth, boolean bfloat16, boolean useDirectIO) {
this(maxConn, beamWidth, bfloat16, useDirectIO, 0, null);
}
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);
}| var offHeap = knnVectorsReader.getOffHeapByteSize(fieldInfo); | ||
| int bytes = useBFloat16() ? BFloat16.BYTES : Float.BYTES; | ||
| assertEquals(vector.length * bytes, (long) offHeap.get("vec")); | ||
| assertEquals(1L, (long) offHeap.get("vex")); |
There was a problem hiding this comment.
The key "vex" used for reporting off-heap byte size is cryptic and not self-descriptive. This harms readability and future maintainability. Please consider renaming it to something more meaningful that clarifies what this value represents, for example "vector_metadata_bytes". Although this seems to be a pattern in other similar classes, this new test provides a good opportunity to introduce a more descriptive name.
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswBFloat16VectorsFormatTests.java (2)
85-98: Regex pattern may not handle scientific notation.The pattern
[0-9.-]+won't match scientific notation (e.g.,1.23E-4or1.23e+10) whichDouble.toString()may produce for very small or large values. Consider updating the regex to handle the exponent part.🔎 Proposed fix
- 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:<([0-9.eE+-]+)> but was:<([0-9.eE+-]+)>");
31-59: Consider adding brief documentation for skipped tests.These empty overrides are clear, but a class-level or method-level comment explaining that BFloat16 format only supports float vectors (not byte vectors) would improve clarity for future maintainers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/src/main/java/module-info.javaserver/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93BFloat16FlatVectorsWriter.javaserver/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormat.javaserver/src/main/resources/META-INF/services/org.apache.lucene.codecs.KnnVectorsFormatserver/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswBFloat16VectorsFormatTests.javaserver/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormatTests.java
🧰 Additional context used
🧬 Code graph analysis (2)
server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormatTests.java (1)
server/src/main/java/org/elasticsearch/index/codec/vectors/BFloat16.java (1)
BFloat16(17-60)
server/src/main/java/module-info.java (1)
server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormat.java (1)
ES93HnswVectorsFormat(24-66)
🔇 Additional comments (9)
server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93BFloat16FlatVectorsWriter.java (1)
252-260: LGTM!The change correctly uses
OffHeapBFloat16VectorValues.DenseOffHeapVectorValuesinstead of float vector values, which aligns with the BFloat16 format. The byte size calculation usingBFloat16.BYTESis consistent with the rest of the file's handling of BFloat16 vectors.server/src/main/resources/META-INF/services/org.apache.lucene.codecs.KnnVectorsFormat (1)
12-14: LGTM!The service provider registration for
ES93HnswVectorsFormatis correctly placed alongside other ES93 vector format entries.server/src/main/java/module-info.java (1)
467-469: LGTM!The
ES93HnswVectorsFormatis correctly declared as a provider forKnnVectorsFormat, consistent with the service registration file.server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswBFloat16VectorsFormatTests.java (1)
61-83: Approach acknowledged for BFloat16 precision testing.The pattern of catching
AssertionErrorfrom the superclass and validating within tolerance bounds is a pragmatic solution for testing lossy BFloat16 compression. The re-throw on non-matching errors (line 90) ensures other failures aren't silently swallowed.server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormatTests.java (3)
43-58: LGTM!The test setup correctly initializes logging (required for ES tests) and creates the format with randomized
useDirectIOflag for better test coverage. Theformatfield is properly initialized beforesuper.setUp()which may require the codec.
93-116: LGTM!The off-heap size test correctly validates memory accounting for both float32 and BFloat16 formats by checking the
useBFloat16()flag to determine expected byte size per dimension.
80-91: [Your rewritten review comment text here]
[Exactly ONE classification tag]server/src/main/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormat.java (2)
24-66: LGTM!The
ES93HnswVectorsFormatimplementation follows established patterns for Elasticsearch HNSW vector formats. It correctly:
- Extends
AbstractHnswVectorsFormatfor shared HNSW configuration- Delegates to
ES93GenericFlatVectorsFormatfor flat vector handling with BFloat16 and DirectIO options- Uses Lucene99-based reader/writer implementations for HNSW graph operations
30-33: The default constructor correctly produces expected defaults:bfloat16=false(using float32VectorFormat) anduseDirectIO=false. No issues found.
![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: The tolerance for comparing floating point values is computed as expected * 0.01, which becomes negative when expected is negative; passing a negative delta into closeTo causes the matcher to always fail (since it effectively requires |diff| <= negative_value), so for negative expected values your "within 1%" relaxation will never succeed and these tests will still fail instead of allowing small differences due to bfloat16 rounding. [logic error]
Severity Level: Minor
| double allowedError = expected * 0.01; // within 1% | |
| double allowedError = Math.abs(expected) * 0.01; // within 1% |
Why it matters? ⭐
The current code computes allowedError as expected * 0.01. If expected is negative this produces a negative delta, which is incorrect for Hamcrest's closeTo matcher (delta must be non-negative) and will make the comparison behave wrongly or always fail for negative expected values. Using Math.abs(expected) (or otherwise ensuring a non-negative delta) fixes a real logic bug in the test relaxation introduced for bfloat16 rounding differences. This is not merely cosmetic — it affects correctness of assertions when expected < 0.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswBFloat16VectorsFormatTests.java
**Line:** 96:96
**Comment:**
*Logic Error: The tolerance for comparing floating point values is computed as `expected * 0.01`, which becomes negative when `expected` is negative; passing a negative delta into `closeTo` causes the matcher to always fail (since it effectively requires `|diff| <= negative_value`), so for negative expected values your "within 1%" relaxation will never succeed and these tests will still fail instead of allowing small differences due to bfloat16 rounding.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| assertEquals(1L, (long) offHeap.get("vex")); | ||
| assertEquals(2, offHeap.size()); |
There was a problem hiding this comment.
Suggestion: The test testSimpleOffHeapSize assumes that getOffHeapByteSize returns an entry with key vex and that the map has exactly two entries; if the underlying implementation only exposes a vec entry (as in other flat vector readers) or adds different keys in future, offHeap.get("vex") will return null and the (long) cast will throw a NullPointerException and/or the fixed size assertion will fail, making the test incorrect and brittle with respect to the actual API contract of getOffHeapByteSize. [logic error]
Severity Level: Minor
| assertEquals(1L, (long) offHeap.get("vex")); | |
| assertEquals(2, offHeap.size()); |
Why it matters? ⭐
The current test makes a brittle assumption about the exact keys and count returned by getOffHeapByteSize: casting offHeap.get("vex") to long will NPE if that key is absent and the fixed-size assertion will break with any change to the map contract. The suggested change (only asserting the essential "vec" entry) removes a fragile assertion and focuses the test on the meaningful quantity (vector bytes). This is a legitimate, non-cosmetic improvement because it prevents spurious failures tied to incidental map entries rather than the behavior under test.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** server/src/test/java/org/elasticsearch/index/codec/vectors/es93/ES93HnswVectorsFormatTests.java
**Line:** 111:112
**Comment:**
*Logic Error: The test `testSimpleOffHeapSize` assumes that `getOffHeapByteSize` returns an entry with key `vex` and that the map has exactly two entries; if the underlying implementation only exposes a `vec` entry (as in other flat vector readers) or adds different keys in future, `offHeap.get("vex")` will return null and the `(long)` cast will throw a NullPointerException and/or the fixed size assertion will fail, making the test incorrect and brittle with respect to the actual API contract of `getOffHeapByteSize`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
User description
gradle check?CodeAnt-AI Description
Add ES93 HNSW vectors format with optional bfloat16 flat storage and tests
What Changed
Impact
✅ HNSW vector search available for ES93 indices✅ Lower on-disk memory for vectors when bfloat16 is enabled✅ Clearer test coverage for bfloat16 numeric tolerance💡 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.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
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
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.