feat: support host filtering and streaming across SINDI modes - #2701
Conversation
|
/label status/waiting-for-review |
Merge Protections🟢 All 3 merge protections satisfied — ready to merge. Show 3 satisfied protections🟢 Require kind label
🟢 Require version label
🟢 Require linked issue for feature/bug PRs
|
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds host-aware routing for immutable DMQ-based SINDI by introducing per-element uint32_t dataset metadata (host_id), persisting it through (legacy + streaming) serialization, and using it to restrict search to host-relevant immutable windows (or direct-DMQ score small hosts).
Changes:
- Extend
Datasetwith nameduint32_tmetadata, including deep-copy/append semantics and ownership-safe destruction. - Add
host_filter_thresholdparameter and implement immutable host-aware search routing (direct scoring for small hosts; window-overlap routing for larger hosts). - Add versioned host metadata to legacy + streaming SINDI serialization and document the new behavior (EN/ZH), with new unit tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage/serialization_tags.h | Adds a new streaming block tag for SINDI host metadata. |
| include/vsag/dataset.h | Public API: adds named uint32_t metadata setters/getters to Dataset. |
| src/dataset_impl.h / src/dataset_impl.cpp | Implements storage, deep copy, append, and owner-safe cleanup for uint32_t metadata. |
| src/dataset_impl_test.cpp | Adds unit tests for uint32_t metadata deep copy, append, and missing-metadata errors. |
| src/algorithm/sindi/sindi_parameter.{h,cpp} | Adds host_filter_threshold param with JSON parsing/validation and compatibility rules. |
| src/algorithm/sindi/sindi_parameter_test.cpp | Adds parameter default/compat/boundary tests for host filter threshold. |
| src/algorithm/sindi/sindi.h / src/algorithm/sindi/sindi.cpp | Implements host grouping at build, host-range routing at query, and host metadata (legacy + streaming) serialization. |
| src/algorithm/sindi/sindi_dmq_test.cpp | Adds host-filter routing + serialization tests. |
| docs/docs/{en,zh}/src/indexes/sindi.md | Documents host filtering API/behavior and streaming support for immutable runtime. |
| docs/docs/{en,zh}/src/advanced/new_serialization.md | Documents the new sindi_host_metadata streaming block and immutable support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
90dd534 to
653fb1c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/algorithm/sindi/sindi.cpp:1995
deserialize_host_metadataonly checkshost_offsets_.back() <= cur_element_count_, which would allow a corrupted/partial host-offset table that fails to cover all inserted documents. Since host metadata implies every successfully inserted doc is assigned to exactly one host range, the final offset should match the element count exactly; otherwise host-scoped queries can silently miss documents.
CHECK_ARGUMENT(host_offsets_.front() == 0, "serialized SINDI host offsets must start at zero");
CHECK_ARGUMENT(std::is_sorted(host_offsets_.begin(), host_offsets_.end()),
"serialized SINDI host offsets must be ordered");
CHECK_ARGUMENT(host_offsets_.back() <= static_cast<uint64_t>(cur_element_count_.load()),
"serialized SINDI host offsets exceed the element count");
LHT129
left a comment
There was a problem hiding this comment.
Thank you for this well-structured PR. The host-aware filtering feature for immutable SINDI is implemented cleanly with comprehensive test coverage and documentation. All previous Copilot review comments have been properly addressed.
Summary of changes reviewed:
- Host-aware filtering: Documents are grouped by
host_iduint32 metadata, with coordinate-compressed host ID mapping for O(log H) query routing. Two search paths: direct DMQ scoring for small hosts (≤ threshold) and window-based search withInnerIdRangeFilterfor large hosts. - Dataset uint32 metadata: Clean implementation with ownership-safe deep copy, append validation, and proper destructor cleanup.
- Serialization: Both legacy and streaming paths support host metadata with format versioning and validation.
- Tests: 3 focused test cases covering direct DMQ path, window path, filter+tombstone, invalid host_ids, serialization round-trip (legacy + streaming), compatibility, and cross-window host spanning.
- Documentation: English and Chinese docs updated for both the index feature and streaming serialization.
Observations (non-blocking):
-
host_filter_thresholdis always serialized inToJson()(line 222 ofsindi_parameter.cpp), whiledmq_shared_codebook_thresholdis only serialized whenrerank_type == dmq8. The unconditional serialization is harmless (default value on deserialization ensures backward compatibility), but the inconsistency with the existing pattern is worth noting. -
The
host_ids_.front() != 0check indeserialize_host_metadata(line 1981 ofsindi.cpp) is logically redundant with the subsequent strict-ordering check, though it provides a clearer error message for this specific case.
No critical issues found. The implementation is correct, well-tested, and follows the existing codebase patterns.
653fb1c to
5c66d00
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/dataset_impl.cpp:241
- This destructor path allocates (and may rehash) an
std::unordered_set. If that allocation throws, the destructor will callstd::terminate(destructors are implicitlynoexceptin most cases). Consider avoiding heap allocations in the destructor by using a small inlined container + linear dedup (metadata key count is typically small), or by tracking owneduint32_t*allocations in a member structure during mutation so destruction becomes a simple non-allocating iteration.
const auto* vector_counts = DatasetImpl::GetVectorCounts();
allocator_->Deallocate(void_ptr(vector_counts));
std::unordered_set<const uint32_t*> released_uint32_arrays;
if (vector_counts != nullptr) {
released_uint32_arrays.insert(vector_counts);
}
for (const auto& [key, value] : this->data_) {
if (IsUInt32MetadataKey(key)) {
const auto* values = std::get<const uint32_t*>(value);
if (values != nullptr && released_uint32_arrays.insert(values).second) {
allocator_->Deallocate(void_ptr(values));
}
}
}
LHT129
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds host-aware filtering to immutable SINDI with a clean design:
Architecture
- Coordinate-compressed host IDs with binary search lookup at query time — memory scales with unique hosts, not max host ID
- Two-path routing: direct forward-store scoring for small hosts (≤threshold), window-based search for large hosts
InnerIdRangeFiltercomposes host range with user filter cleanly- Shared
route_host_searchused by bothKnnSearchandSearchWithRequest
Correctness
- Proper handling of missing host metadata, invalid host IDs, empty hosts, and tombstone deletions
- Streaming and legacy serialization both covered with format versioning
- Host metadata compatibility checked during deserialization (not parameter comparison), avoiding false load failures for non-host indexes
Tests
- 61 assertions across 3 focused test cases covering direct routing, window routing, filter composition, tombstones, serialization roundtrips, and edge cases
All prior review feedback has been addressed in commit 653fb1ca. No substantive issues found.
5c66d00 to
04e526b
Compare
|
Maintenance update for
Local verification: incremental |
69519e6 to
f386a16
Compare
Release host directory storage on reset, improve malformed metadata diagnostics, remove the fixed unique-host cap, skip empty windows between disjoint host ranges, and cover repeated mutable additions and range-aware routing. Keep host search behavior scoped to posting-window filtering, preserve wrapped filter valid-ID bounds, clear stale host state after legacy loads, avoid allocations while releasing Dataset metadata, and keep host-filter tests independent of DMQ-specific coverage. Signed-off-by: Roxanne0321 <liruoxvan020321@qq.com> Assisted-by: Codex:gpt-5
LHT129
left a comment
There was a problem hiding this comment.
[suggestion] In SindiHostFilter::Classify (sindi_host_filter.cpp), query_host_id[0] is accessed without verifying that the query dataset has at least one element. If a caller passes a query with NumElements(0) and a non-null host_id metadata pointer (pointing to an empty or undersized buffer), this is undefined behavior. Consider adding query->GetNumElements() == 0 to the early-return guard.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR adds host-aware filtering and streaming serialization to both SINDI and SINDI_V2 indexes. The implementation is well-structured with clear separation of concerns: SindiHostFilter handles the host metadata lifecycle, SindiHostBuildPlan manages the build-time reordering, and InnerIdHostFilter provides runtime inner-ID range filtering.
Architecture & Design
The host filter design is sound:
- Sort-based grouping in
PrepareBuildgroups documents by host_id before insertion, enabling contiguous inner-ID ranges per host - Merge-based
CommitBuildcorrectly handles incremental additions with contiguous range merging - Three-route classification (UNFILTERED/EMPTY/WINDOW) cleanly separates the no-filter, no-results, and filtered search paths
- Window-level optimization via
ApplyWindowRoute/NextMatchingWindow/RequiresFullTermScanavoids scanning irrelevant windows and correctly disables term pruning at window boundaries
The SINDI_V2 streaming serialization is a substantial new feature (300+ lines) that follows the same pattern as the existing SINDI streaming code. The block-based format with version checking and critical/non-critical tagging is well-designed for forward compatibility.
Test Coverage
The test suite is thorough:
- Host filter routing tests for both SINDI and SINDI_V2 (immutable mode)
- Mutable/immutable/reorder mode combinations for SINDI_V2
- Window boundary term prune preservation tests
- Legacy deserialization compatibility (host metadata cleared)
- Full streaming round-trip tests with block manipulation (erase block, version bump)
- Dataset UInt32Metadata tests (aliasing, deep copy, append, slice, null)
Issues Raised
I have left 5 inline comments on the PR:
RecordSuccessordering assumption (line 85)CommitBuildmerge invariant documentation (line 167)RequiresFullTermScanthree-case clarity (line 275)RecordSuccessbounds check when enabled but empty (line 92)NextMatchingWindowmid-window range edge case (line 290)
All are suggestions for improving clarity or defensive checks — none are correctness issues.
Overall Assessment
The implementation is correct and well-tested. The host filter integration is cleanly done across both SINDI and SINDI_V2 with minimal code duplication. The streaming serialization for SINDI_V2 is comprehensive and follows established patterns. Ready to merge after addressing the minor suggestions.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
I reviewed the full diff (22 files, +2445/-46) for the host-aware filtering and streaming serialization across SINDI modes. The overall architecture is well-designed: coordinate-compressed host IDs, clean separation of SindiHostFilter/SindiHostBuildPlan/SindiHostSearchRoute, and thorough validation in serialization. All issues from previous review rounds (coordinate compression, host_filter_threshold removal, InnerIdRangeFilter → InnerIdHostFilter, search_host_direct removal, uint32_t truncation) have been properly addressed.
Remaining observations
[note] ABI-breaking Dataset interface change
include/vsag/dataset.h: Adding pure virtual methods UInt32Metadata and GetUInt32Metadata breaks ABI compatibility. Any downstream Dataset implementation will fail to compile until it adds stubs for these methods. This was flagged previously and remains present. Consider whether this can be done with a non-virtual helper or free function instead, or document the break clearly in release notes.
[suggestion] CommitBuild merge logic uses double-negative booleans
src/algorithm/sindi_host_filter.cpp lines 786-787: has_existing = not take_added and has_added = not take_existing. When both host_ids are equal, both take_existing and take_added are false, making both has_existing and has_added true — which is correct but the reasoning is indirect. Consider using an explicit equality comparison for the same-host-id case to make the intent clearer.
[note] O(n²) aliasing detection in DatasetImpl destructor
src/dataset_impl.cpp lines 2801-2822: The release_uint32_metadata lambda detects aliased pointers with a nested loop over data_. In practice the number of uint32 metadata keys is very small (typically 1-2), so this is not a real performance concern, but it was flagged in a previous review and remains unchanged.
What looks good
- Serialization format consistency:
WriteVector(uint64_t size + raw data) inSerializematches theReadObj+reader.Readpattern inDeserialize. Validation is thorough: strict ordering, disjoint ranges, full element coverage, and bounds checking. - Host metadata lifecycle:
PrepareBuildcorrectly rejects host metadata after host-unaware documents and vice versa. LegacyDeserializeclears host metadata in both SINDI and SINDIV2. - Streaming block handling: Both SINDI and SINDIV2 properly handle missing/duplicate/unsupported blocks with appropriate error messages. The
expects_host_metadataflag from basic_info correctly gates the host metadata block requirement. - Term prune control:
SetTermPruneEnabledcorrectly disables pruning for windows that only partially overlap with host ranges, preventing incorrect candidate elimination at window boundaries. - Test coverage: 13 host-filter test cases (356 assertions), SINDIV2 streaming roundtrip with mutable/immutable modes, legacy deserialize clears host metadata, streaming validation (missing blocks, version checks), and per-window term prune tests.
- Documentation: Both English and Chinese docs updated with clear examples and behavioral descriptions.
|
Tick the box to add this pull request to the merge queue (same as
|
Summary
This PR adds host-aware filtering to SINDI and SINDI_V2 across mutable and immutable modes, including mutable Add and the new streaming serialization lifecycle.
Host filtering
uint32host metadata during host-aware Build and Add.host_id = 0for documents without a host ID; it is indexed and queried like any other host bucket.uint32_t.host_filter_thresholdand the low-cardinality direct or brute-force path without compatibility handling for unpublished threshold-based host indexes.Streaming serialization
SerializeStreaming,DeserializeStreaming, andIndex::Loadfor mutable and immutable SINDI and SINDI_V2.Compatibility impact
Datasetinterface withUInt32MetadataandGetUInt32Metadata. DownstreamDatasetimplementations must add these methods, so this is a source and ABI compatibility change.DatasetImpl, mocks, and tests for the new interface.host_filter_thresholdconfiguration or threshold-based host serialization.Performance impact
uint32host IDs.Documentation
Updates the English and Chinese SINDI, SINDI_V2, and new-serialization pages. The documentation describes missing-host ID 0, mutable multi-range Add, posting-only host filtering, supported streaming modes, and legacy serialization behavior.
Validation
unitteststarget.git diff --checkpassed.Closes: #2700