feat(pyramid,sindi,warp): add MARK_REMOVE support#2136
Hidden character warning
Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Require kind labelWonderful, this rule succeeded.
🟢 Require version labelWonderful, this rule succeeded.
|
There was a problem hiding this comment.
Code Review
This pull request implements the Remove method and tracks deleted elements via delete_count_ across the Pyramid, SINDI, and WARP algorithms. However, several critical issues were identified: first, delete_count_ is not restored during deserialization in Pyramid and SINDI, leading to incorrect element counts after loading; second, there are concurrency synchronization mismatches in Pyramid (mismatched locks between Add and Remove) and SINDI (using label_lookup_mutex_ instead of global_mutex_); and third, WARP's delete_count_ is not atomic, introducing a data race during concurrent reads and writes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
21e13b7 to
f85ccf7
Compare
There was a problem hiding this comment.
Pull request overview
Adds MARK_REMOVE-style deletion support to the Pyramid, SINDI, and WARP inner indexes by introducing Remove() implementations and exposing removed/active element counts via GetNumberRemoved() and updated GetNumElements() behavior.
Changes:
- Implement
Remove(ids, RemoveMode::MARK_REMOVE)for WARP, Pyramid, and SINDI usinglabel_table_->MarkRemove(ids). - Track removed counts (new
delete_count_atomics for Pyramid/SINDI) and updateGetNumElements()to return active elements. - Add
GetNumberRemoved()for Pyramid/SINDI (WARP already had it).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/algorithm/warp/warp.h | Declares Remove() for WARP. |
| src/algorithm/warp/warp.cpp | Implements WARP::Remove() (MARK_REMOVE only) and updates delete counter. |
| src/algorithm/sindi/sindi.h | Updates element counting, adds GetNumberRemoved(), and declares Remove() plus delete_count_. |
| src/algorithm/sindi/sindi.cpp | Implements SINDI::Remove() (MARK_REMOVE only) and updates delete counter. |
| src/algorithm/pyramid/pyramid.h | Declares GetNumberRemoved()/Remove() and adds delete_count_. |
| src/algorithm/pyramid/pyramid.cpp | Updates GetNumElements(), implements GetNumberRemoved()/Remove() for MARK_REMOVE. |
f85ccf7 to
a1b928e
Compare
a1b928e to
2c60931
Compare
2c60931 to
044304a
Compare
|
Thanks for the careful reviews! Addressed in 044304a: Correctness (gemini)
Perf / fast-path (copilot)
Feature flags (copilot)
Tests (copilot)
All 22050 assertions in the 3 new test cases pass locally. |
044304a to
16d81d6
Compare
|
All review comments addressed and rebased onto latest main (a1b501f). Key fixes:
|
Add Remove() method to Pyramid, SINDI, and WARP indexes to support MARK_REMOVE mode. This enables marking vectors as deleted without physical removal. Changes: - WARP: Implement Remove() using existing delete_count_ - Pyramid: Add delete_count_, implement GetNumElements/GetNumberRemoved/Remove - SINDI: Add delete_count_, implement GetNumElements/GetNumberRemoved/Remove All three indexes now: - Support Remove(ids, RemoveMode::MARK_REMOVE) - Return correct counts via GetNumElements() and GetNumberRemoved() - Throw exception for unsupported FORCE_REMOVE mode Signed-off-by: LHT129 <tianlan.lht@antgroup.com> Co-authored-by: opencode <opencode@anthropic.com>
16d81d6 to
92a5144
Compare
| int64_t | ||
| GetNumElements() const override { | ||
| return cur_element_count_; | ||
| return cur_element_count_ - delete_count_; | ||
| } |
| SINDI::Remove(const std::vector<int64_t>& ids, RemoveMode mode) { | ||
| if (mode != RemoveMode::MARK_REMOVE) { | ||
| throw VsagException(ErrorType::INVALID_ARGUMENT, "SINDI only supports MARK_REMOVE"); | ||
| } | ||
| std::scoped_lock lock(this->global_mutex_); | ||
| uint32_t delete_count = this->label_table_->MarkRemove(ids); | ||
| delete_count_ += delete_count; | ||
| return delete_count; |
Summary
Add Remove() method to Pyramid, SINDI, and WARP indexes to support MARK_REMOVE mode, enabling marking vectors as deleted without physical removal.
Changes
WARP Index
Pyramid Index
SINDI Index
Implementation Details
All three indexes:
Test Plan
Related Issue
Closes #2135
Reference