feat(sindi): implement value-aware term pruning

[Roxanne0321]

What

• expose sindi.term_prune_ratio and sindi.term_prune_threshold as peer search parameters
• support independent ratio and non-negative threshold limits, using the smaller posting scan count when both are set
• treat an omitted or explicit term_prune_threshold: 0 as disabled; divide positive thresholds across populated index windows with threshold / window_num
• require positive indexed and query sparse values; sort mutable and immutable posting lists by decoded value and always scan the high-value prefix
• reject non-positive sparse values before sorting; serialized mutable and immutable posting normalization still rejects NaN
• defer normalization of mutable partial windows until they fill; dirty partial lists scan all postings so incremental single-document Add avoids quadratic full-prefix reordering without incorrect pruning
• normalize remaining partial windows once during Build, normalize deserialized posting lists, and trim historical trailing empty windows during legacy and streaming deserialization
• apply the same pruning behavior to mutable, immutable, heap-insertion, range-search, and analyzer paths
• remove SINDI benchmark groups from hgraph-90.yml, hgraph-95.yml, and hgraph-99.yml so those configurations contain only HGraph cases
• update SINDI documentation, examples, and tests

Defaults

• term_prune_ratio: 0.0
• term_prune_threshold: 0 (disabled)

Validation

• built unittests and functests
• 8 focused unit cases covering positive-value validation, posting sorting, scan limits, mutable/immutable pruning, dirty windows, legacy normalization, and NaN rejection: 115 assertions
• SparseTermDatacell Basic Test: 230 assertions
• SINDI DMQ Rerank Large Term ID Fallback Test: 3 assertions
• SINDI DMQ Streaming Serialization Test: 22 assertions
• SparseDmqDataCell implements FlattenInterface: 18 assertions
• complete [sindi] functional filter: 8 test cases and 498,982 assertions
• clang-format 15.0.7 and git diff --check
• HGraph benchmark YAML parsing and HGraph-only index-name check
• Python example syntax check

[vsag-bot]

/label status/blocked
/waiting-on author

[vsag-bot]

Automated pull request review completed.

Review effort: high (982 changed lines across 25 files).

Submitted 5 inline comments.
Review: #2542 (review)

[mergify]

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

• label~=^kind/

🟢 Require version label

• label~=^version/

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[vsag-bot]

Automated inline review completed.

Review effort: high (600 changed lines across 28 files).
Submitted 5 inline comments.

Reviewed commit 0aa6524.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[LHT129]

Reviewed the latest commit (dc9a5c2) for the value-aware term pruning feature.

Summary: The implementation correctly replaces the flat term_prune_ratio with a nested term_prune object supporting independent ratio and threshold limits. The SortByValue() integration in Add() and build_immutable() is correctly placed before compaction/serialization. The analyzer paths (collect_coarse_candidates, collect_doc_prune_candidates) correctly pass window_num to SparseTermComputer. Test coverage is thorough across parameter parsing, sort correctness, scan count computation, and end-to-end search with pruning.

Existing review threads that still need attention:

1. CalcDistanceById (single) and CalDistanceById (batch) in sindi.cpp still construct SparseTermComputer without window_num, defaulting to window_num=1. This means term_prune.threshold is not divided across windows in these code paths, which could cause distance calculations to scan more postings than expected when a threshold is configured.

2. The SortByValue() call after every Add() batch sorts all terms in affected windows even when no term pruning is configured, which can regress streaming workloads.

No new blocking issues found beyond those already raised in existing review threads.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Roxanne0321]

Maintenance update for 0b7f2ea3:

• The latest CircleCI failure was another stale test datum: SINDI DMQ Rerank Large Term ID Fallback Test still used a zero sparse value. Changed that single test value to a positive value; no product code changed.
• Built unittests and passed the exact DMQ fallback regression (3 assertions); clang-format 15 and git diff --check pass.
• Evaluated the new suggestion to merge immutable and incremental posting-sort implementations. No refactor was made because their state transitions differ (full segment normalization versus dirty-suffix sort/merge and metadata updates), and the suggestion is non-blocking with no correctness benefit.

The inline thread was answered and resolved. A fresh CI run is starting.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Roxanne0321]

Fixed the CircleCI failure in SINDI DMQ Streaming Serialization Test with a test-only update in 467da33a. The shared SmallDmqDataset still contained a zero sparse value, so the newly enforced positive-value contract rejected it in addition to the intentionally invalid empty vector (build_res.size() became 2 instead of 1). Updated that stale fixture value to 0.125F; the empty vector remains the sole expected failure. Verified the exact test locally: 1 test case, 22 assertions, plus clang-format 15 and git diff --check.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[LHT129]

test comment

[LHT129]

Code review for feat(sindi): implement value-aware term pruning

[Roxanne0321]

Updated DMQ persistence handling in 5a0d7ecc per the latest scope decision. This PR no longer changes the existing on-disk version: writer/reader are restored to the base v6 contract, the v1-specific test changes are reverted, and v5 remains the intentionally rejected previous version. A source TODO records that resetting to v1 belongs in a separate future format change. Focused validation: SparseDmqDataCell implements FlattenInterface passed 18 assertions; clang-format 15 and git diff --check passed.

[LHT129]

Code Review Summary

This PR implements value-aware term pruning for SINDI sparse index. Overall code quality is good — most previous review comments have been addressed, the analyzer refactoring correctly delegates to Query(), and thread safety appears correct with proper lock usage. A few items to consider:

Key Findings

1. [note] sort_posting_list_by_value duplicates logic with SparseTermDataCell::SortByValue — The free function in sindi.cpp and the method in sparse_term_datacell.cpp share the same sorting algorithm. Consider consolidating to reduce maintenance burden.

2. [note] CalcDistanceById/CalDistanceById don't pass window_num — In sindi.cpp, SparseTermComputer is constructed without window_num, defaulting to 1. Since term_prune_ratio was removed (set to 0) and term_prune_threshold defaults to 0 (disabled), no pruning actually occurs in these paths, so this is functionally correct. However, if pruning parameters are ever enabled for these code paths, the window_num would need to be passed.

3. [note] sort_posting_list_by_value is outside the vsag namespace — The free function in sindi.cpp is defined outside the vsag namespace while all other code in the file is inside it.

What Was Addressed from Previous Reviews

• The nested term_prune JSON object design was changed to flat parameters ✅
• The FromJson default-reset issue was addressed ✅
• The uint64_t → uint32_t cast in GetTermScanCount was addressed with proper static_cast ✅

What Remains

• The CalcDistanceById/CalDistanceById window_num issue flagged by LHT129 is still present (though functionally harmless with current defaults)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[vsag-bot]

Automated inline review completed.

Review effort: high (1114 changed lines across 30 files).
Submitted 3 inline comments.

Reviewed commit 5a0d7ec.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[LHT129]

Code review for commit 96472c3 (test(sindi): simplify positive sparse fixtures).

The cleanup is well-executed. Consolidating "positive_sparse" into "sparse" with a 0.1F min value simplifies the test infrastructure without losing coverage. The NaN test simplification to only check SortByValue rejection is correct since NaN is now rejected at sort time, not insertion time.

A few observations:

1. [suggestion] The free function sort_posting_list_by_value in sindi.cpp duplicates the same sorting logic as SparseTermDataCell::SortByValue(). Consider extracting the common in-place permutation algorithm into a shared utility to reduce duplication and maintenance burden.

2. [note] The SortByValue test calls data_cell.SortByValue() twice in a row without an assertion or comment. If this is testing idempotency, a brief comment would help future readers understand the intent.

3. [note] The sindi_dmq_datacell.cpp TODO about resetting the serialization version is a good reminder — the version was bumped to 6 for the sorting change and should be reset in a dedicated format migration.

Overall the PR looks solid. The parameter validation changes (doc_prune_ratio, query_prune_ratio, term_prune_ratio all moving to [0, 1)) are consistent and well-tested. The test coverage for term pruning, incremental partial windows, threshold division across windows, and legacy deserialization sorting is comprehensive.

[LHT129]

[note] SortByValue() is called twice in a row here. If this is testing idempotency (i.e., sorting an already-sorted list produces the same result), a brief comment would help future readers understand the intent.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[LHT129]

Review Summary

This is a follow-up review on the latest commit 2cb63679. The PR implements value-aware term pruning for SINDI with posting-list sorting, threshold-based scan limits, and legacy data normalization.

Previously reported issues still present

[critical] CalcDistanceById and CalDistanceById still construct SparseTermComputer without window_num (previously reported by @LHT129 in discussion_r3656456807)

In sindi.cpp:

• Line 1981 (CalcDistanceById): std::make_shared<SparseTermComputer>(sparse_query, search_param, allocator_) — defaults to window_num=1
• Line 2067 (CalDistanceById): std::make_shared<SparseTermComputer>(mapped_query, search_param, allocator_) — defaults to window_num=1

Both KnnSearch (line 626) and RangeSearch (line 1145) correctly pass the actual window count. When term_retain_threshold is configured, these two code paths will not divide the threshold across windows, potentially scanning more postings than expected.

Resolved issues

• Posting-list sorting now uses SortPostingListByValue with in-place permutation, avoiding extra allocation for sorted copies.
• SortByValue in Add() now correctly handles the case where last_affected_window remains -1 when all input vectors are rejected.
• deserialize_immutable_window normalizes legacy posting lists on load via SortPostingListByValue.
• trim_deserialized_trailing_windows correctly handles both mutable and immutable paths.
• sindi_analyzer.cpp correctly passes window_num to SparseTermComputer.

Overall

The implementation is solid. The only remaining concern is the window_num omission in CalcDistanceById/CalDistanceById, which was already flagged in a prior review.

[LHT129]

Review Summary (follow-up on 2cb6367)

[critical] CalcDistanceById and CalDistanceById still construct SparseTermComputer without window_num (previously reported by @LHT129)

In sindi.cpp line 1981 and line 2067, SparseTermComputer is constructed with the default window_num=1, while KnnSearch (line 626) and RangeSearch (line 1145) correctly pass the actual window count. When term_retain_threshold is configured, these two code paths will not divide the threshold across windows.

Other previously reported issues have been resolved. No new issues found in this commit.

[LHT129]

Review Summary (follow-up on commit 2cb6367)

[critical] CalcDistanceById and CalDistanceById still construct SparseTermComputer without window_num (previously reported by @LHT129)

In sindi.cpp line 1981 and line 2067, SparseTermComputer is constructed with the default window_num=1, while KnnSearch (line 626) and RangeSearch (line 1145) correctly pass the actual window count. When term_retain_threshold is configured, these two code paths will not divide the threshold across windows.

Other previously reported issues have been resolved. No new issues found in this commit.

[vsag-bot]

Automated inline review completed.

Review effort: high (982 changed lines across 25 files).
Submitted 5 inline comments.

Reviewed commit 2cb6367.

[vsag-bot]

[P1] Avoid sorting complete windows after every Add

This invokes SortByValue() after every Add, and that method scans every term and runs a full std::sort over every non-empty posting list. Repeated single-document Adds sharing a term therefore redo prefixes of lengths 1 through N even when pruning is disabled; a 2,000-document debug probe took 1,426 ms versus 9 ms for one batch, and the allowed 60,000-document window magnifies this quadratic behavior. Track dirty suffixes or otherwise avoid rebuilding complete posting lists on each incremental Add.

[vsag-bot]

[P2] Choose the retained end from the query sign

Posting lists are descending, and this always scans from data(), although signed sparse values remain accepted. With base values 3 and -4, query value -1, and threshold 1, the unpruned search returns the -4 document while this path scans 3 and returns no result. Either reject signed indexed/query values before mutation or scan the low-value suffix for negative queries; immutable and heap-insertion paths need the same rule.

[vsag-bot]

[P1] Validate NaN before committing Add state

Add reaches this check only during its post-insertion SortByValue() call, after postings, labels, element count, and optional rerank data have been committed. A one-term FP32 NaN Add returns an error but leaves GetNumElements() equal to 1, and subsequent normalization continues encountering the poisoned posting. Validate original values before SQ8 training/remapping/insertion, or roll back every committed structure when sorting fails.

[vsag-bot]

[P1] Validate deserialized posting metadata before dereferencing

Deserialize() reads term_sizes_ without verifying that its length and non-empty entries match term_ids_, term_datas_, and their byte counts, then enters this loop. A payload with capacity 1, an empty term 0, and serialized sizes {0,1} reaches term 1 and dereferences term_ids_[1]; the focused probe exited with SIGSEGV. Reject inconsistent mutable payloads before calling SortByValue() instead of trusting these vectors to be aligned.

[vsag-bot]

[P2] Do not re-sort every current payload during load

Every payload written by this revision is already sorted because Add/Build normalizes postings, yet every mutable load unconditionally runs full sorting here; immutable loading has the same unconditional per-term normalization. Thus ordinary round trips, including configurations with pruning disabled, change from linear restoration to sum(m log m) comparisons and scratch work. Persist ordering metadata or perform a linear orderedness check so only legacy unsorted lists are normalized.