LSM Vector index implementation - #2800
Conversation
Summary of ChangesHello @robfrank, 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 robust and well-documented LSM-based vector indexing system for ArcadeDB, enabling efficient storage and retrieval of vector embeddings. The implementation follows a phased approach, with the initial phases delivering core functionality, SQL/Java API integration, and a solid architectural foundation for future enhancements like HNSW acceleration. Accompanying the code are extensive documentation and a detailed analysis of the existing LSM-tree index, ensuring clarity for both users and developers. 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
|
…Mutable - Implement VectorSerializer for binary serialization of vectors and RID sets - Add disk storage infrastructure to LSMVectorIndexMutable with ComponentFile support - Initialize page buffering system for future disk writes - Add disk initialization with graceful fallback to in-memory if disk I/O fails - Update close() method to flush disk buffers - Add initializeDiskStorage() for future file I/O integration - Maintains 100% backward compatibility with existing API 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…est data Fix LSMVectorIndex factory handler parameter ordering: - docType now receives getTypeName() (was getIndexName) - property now receives getPropertyName() (was getFilePath) - name now receives getIndexName() (was getTypeName) - filePath now receives getFilePath() (was getPropertyName) Fix testKNNSearchAllDistanceMetrics test design: - Use unique property names for each metric (embedding_cosine, embedding_euclidean, etc.) - Use unique RIDs for each metric test to avoid data contamination - Use orthogonal test vectors (v1=[1,0,0], v2=[0,1,0]) for better metric distinction - Previous test data (parallel vectors) caused COSINE similarity ties Results: - All 12 LSMVectorIndexTest tests now pass (previously 3 failures) - All 20 VectorSerializerTest tests still pass (no regressions) - Factory now correctly initializes all index parameters 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
… improved KNN search filtering
a876c10 to
7f5fae8
Compare
🧪 CI InsightsHere's what we observed from your CI run for c014978. 🟢 All jobs passed!But CI Insights is watching 👀 |
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive implementation of an LSM-based vector index, LSMVectorIndex, for ArcadeDB. The changes are extensive, covering not only the core indexing logic with mutable and compacted components but also schema integration, a dedicated builder, a compactor, and numerous tests. The implementation is well-structured and follows the LSM-tree architecture principles. My review focuses on a few key areas: a critical bug in the sorting logic for the Euclidean distance metric, an incomplete implementation of the remove operation which could lead to data inconsistency with disk persistence, a missing feature for search filtering in the HNSW-accelerated path, and the use of hardcoded filtering thresholds in KNN search, which could impact usability.
| // Then sort by distance/similarity based on metric type: | ||
| // - EUCLIDEAN: lower distance is better → ascending sort | ||
| // - COSINE/DOT_PRODUCT: higher similarity is better → descending sort | ||
| if ("EUCLIDEAN".equals(similarityFunction)) { |
There was a problem hiding this comment.
There is a bug in the sorting logic for the KNN search results. The similarityFunction is an enum of type VectorSimilarityFunction, but it's being compared to a string literal "EUCLIDEAN". This comparison will always evaluate to false, causing the sorting logic for the Euclidean distance to be skipped and falling through to the descending sort, which is incorrect for a distance metric where lower values are better.
| if ("EUCLIDEAN".equals(similarityFunction)) { | |
| if (similarityFunction == VectorSimilarityFunction.EUCLIDEAN) { |
| public void remove(final float[] vector, final RID rid) { | ||
| if (vector == null || rid == null) | ||
| return; // Skip removal silently for null values | ||
|
|
||
| final VectorKey key = new VectorKey(vector); | ||
| final Set<RID> rids = vectorToRIDs.get(key); | ||
| if (rids != null && rids.remove(rid)) { | ||
| totalEntries--; | ||
| if (rids.isEmpty()) { | ||
| vectorToRIDs.remove(key); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The remove operation only updates the in-memory vectorToRIDs map. It does not account for the disk-based persistence layer. If a vector has been flushed to disk, removing it from the in-memory map will not prevent it from being reloaded from disk on a subsequent startup or during compaction. This could lead to "resurrected" data and inconsistencies. To fix this, a tombstone mechanism should be implemented for on-disk entries, so that they can be properly purged during compaction.
| final List<LSMVectorIndexMutable.VectorSearchResult> filtered = new ArrayList<>(); | ||
| for (final LSMVectorIndexMutable.VectorSearchResult result : results) { | ||
| final boolean shouldInclude = switch (similarityFunction) { | ||
| case COSINE -> result.distance > 0.5f; // More than 50% similar | ||
| case DOT_PRODUCT -> result.distance > 0.0f; // Positive correlation | ||
| case EUCLIDEAN -> true; // For EUCLIDEAN, include all (distance-based) | ||
| }; | ||
|
|
||
| if (shouldInclude) { | ||
| filtered.add(result); | ||
| if (filtered.size() >= k) | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return filtered; | ||
| } finally { |
There was a problem hiding this comment.
The KNN search results are being filtered by a hardcoded similarity threshold (e.g., > 0.5f for COSINE). This might be unexpected for users, as it can cause the method to return fewer than k results, even when more are available but do not meet this internal threshold. It would be better to return all k nearest neighbors and let the user decide how to filter them, or to make this threshold a configurable parameter of the search.
| public List<LSMVectorIndexMutable.VectorSearchResult> knnSearch( | ||
| final float[] queryVector, final int k, | ||
| final com.arcadedb.index.lsm.LSMVectorIndex.IgnoreVertexCallback ignoreCallback) { | ||
| // TODO: Implement filtering support in HNSW search | ||
| // For now, delegate to HNSW search without filtering | ||
| return knnSearch(queryVector, k); | ||
| } |
There was a problem hiding this comment.
The knnSearch method that accepts an ignoreCallback for filtering is not fully implemented. It currently delegates to the non-filtering version, and the ignoreCallback parameter is ignored. This represents a loss of functionality compared to the parent class LSMVectorIndexCompacted, which does support filtering. The TODO comment acknowledges this, but it's an important feature to implement for consistency and utility.
| final List<VectorSearchResult> filtered = new ArrayList<>(); | ||
| for (final VectorSearchResult result : results) { | ||
| boolean shouldInclude = switch (similarityFunction) { | ||
| case COSINE -> result.distance > 0.5f; // More than 50% similar | ||
| case DOT_PRODUCT -> result.distance > 0.0f; // Positive correlation | ||
| case EUCLIDEAN -> true; // For EUCLIDEAN, include all (distance-based, not similarity) | ||
| }; | ||
|
|
||
| if (shouldInclude) { | ||
| filtered.add(result); | ||
| if (filtered.size() >= k) | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return filtered; |
There was a problem hiding this comment.
Similar to the main LSMVectorIndex class, this knnSearch implementation filters results based on a hardcoded threshold (e.g., > 0.5f for COSINE). This can lead to fewer than k results being returned, which might be confusing for users. Consider removing this filtering logic to ensure the top k neighbors are always returned, or make the threshold configurable.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
|
supersed by #2816 |
1 participant
This pull request adds comprehensive documentation and analysis for the new LSMVectorIndex implementation in ArcadeDB. The changes include a detailed README for the vector index, outlining its architecture, usage, and roadmap, as well as an in-depth technical analysis of the existing LSM-tree index codebase and recommendations for vector index integration. These resources are intended to help both users and developers understand, use, and extend the LSMVectorIndex.
Documentation and Analysis Additions:
LSMVectorIndex Documentation:
LSMVECTORINDEX_README.md, providing a complete quick start, architecture overview, feature checklist, performance characteristics, configuration options, test coverage, troubleshooting, future roadmap, and developer/architect learning resources. The README also maps all related documentation and source files for easy navigation.Technical Analysis of LSM-tree Index:
LSM_ANALYSIS_SUMMARY.txtwith a comprehensive breakdown of the LSM-tree index implementation in ArcadeDB. This includes class responsibilities, storage architecture, transaction integration, concurrency, serialization, and design patterns. The analysis concludes with concrete recommendations for implementing the new LSMTreeVectorIndex, ensuring alignment with established patterns and highlighting vector-specific considerations.