perf(sindi): batch posting normalization for incremental add

[CharlesXu-HQ]

Change Type

• Bug fix
• New feature
• Improvement/Refactor
• Documentation
• CI/Build/Infra

Linked Issue

• Fixes: [improve](sindi): avoid repeated posting-list sorting during incremental add #2616

What Changed

• Keep each mutable SINDI posting list as a sorted prefix plus a bounded sorted dirty suffix, so incremental Add no longer re-sorts the full posting list after every batch.
• Merge the two sorted runs exactly during pruning and heap insertion. KNN, range search, filters, and value-aware term pruning retain their existing result semantics while a suffix is pending normalization.
• Normalize dirty postings at bounded thresholds, full-window/build boundaries, compaction, and serialization. The existing SINDI wire format remains version 1 and serialized bytes are unchanged.
• Add deterministic lifecycle, serialization, filtering, boundary, randomized, and three-quantization regression coverage.

Test Evidence

• make fmt equivalent with clang-format 15.0.7
• make lint equivalent on changed production sources with clang-tidy 15.0.7
• make test equivalent: complete unittests suite on Linux x86-64
• make cov equivalent, run focused tests, and collect changed-line coverage
• Other: targeted ASan and controlled Release A/B benchmarks (details below)

Test details:

clang-format 15.0.7 --dry-run --Werror on all five changed files
# passed

clang-tidy 15.0.7 on:
  src/algorithm/sindi/sindi.cpp
  src/datacell/sparse_term_datacell.cpp
# 0 project diagnostics

./build-target-config-system-openblas/tests/unittests --reporter compact
# All tests passed (85,325,939 assertions in 687 test cases)

./build-target-config-system-openblas/tests/unittests <SINDI/datacell focused filters>
# All tests passed (227,437 assertions in 47 test cases)

# ASan+UBSan: All tests passed (231,844 assertions in 48 test cases)
# gcov 13 changed production executable lines: 228 / 232 = 98.28%

Focused coverage includes all sparse value encodings (fp32, fp16, sq8), dirty-run thresholds and boundaries, 20 randomized seeds, partial/full/next-window incremental lifecycles, KNN top-k 1/10/100, range search, pure/filter paths (100%, 50%, 1% selectivity), serialization/streaming serialization, restore, wire version retention, and Add after restore.

The four uncovered added executable lines are defensive unreachable paths: the binary-partition failure guard, the invalid-quantization guard, and the no-dirty-posting early return.

Compatibility Impact

• API/ABI compatibility: no public API or serialized-format change; SINDI wire version remains 1.
• Behavior changes: incremental mutable postings can temporarily contain two individually sorted runs. Query paths merge them exactly, and persistence boundaries normalize before writing.

Performance and Concurrency Impact

• Performance impact: incremental Add throughput improves; controlled results are below. Search latency remains within normal run-to-run variation in the measured workload.
• Concurrency/thread-safety impact: Add already takes the global write lock. Serialization now takes the same exclusive lock while normalizing pending postings before writing; query locking is unchanged.

Controlled A/B setup

• Host: AMD Ryzen 7 9700X, Linux x86-64.
• Release libraries were built from the same source tree, compiler, dependencies, and flags. Baseline replaces only the three production files with upstream main (0dde8e4b9fe9a5cc643a4ee4f3a23a29fe0c1a70).
• Dataset per process: 30,000 initial + 9,000 incremental sparse vectors, vocabulary 1,024, 32 non-zero terms/document, window size 10,000.
• Isolation: CPU 15 only, one-CPU quota, 6 GiB limit, nice +10, OPENBLAS_NUM_THREADS=1, OMP_NUM_THREADS=1, network disabled.
• Order-balanced runs: baseline current current baseline baseline current current baseline baseline current for FP32 (5 samples/version); the first six entries for FP16/SQ8 (3 samples/version).
• Query phase: 100 KNN top-100 and 100 range-limit-100 searches per process. Values below are medians across independent processes.
• Host load average was 0.20 before and 0.43 after the FP32 matrix; unrelated containers were recorded before and after and left untouched.

Incremental Add throughput

Encoding	Batch	Baseline vec/s	Current vec/s	Speedup	Add time reduction
FP32	1	6,687	67,602	10.11x	90.1%
FP32	16	24,846	135,701	5.46x	81.7%
FP32	256	112,523	427,988	3.80x	73.7%
FP32	4,096	504,644	812,295	1.61x	37.9%
FP16	1	7,893	67,315	8.53x	88.3%
FP16	256	129,935	471,941	3.63x	72.5%
FP16	4,096	567,135	892,546	1.57x	36.5%
SQ8	1	7,766	72,099	9.28x	89.2%
SQ8	256	132,533	505,077	3.81x	73.8%
SQ8	4,096	600,883	977,443	1.63x	38.5%

Across these cells, Add batch p95 latency falls by 27.0% to 88.7%.

Search, accuracy, serialization, and memory

• Every one of the 76 measured processes produced identical A/B KNN hashes, range hashes, serialized hashes, and serialized byte counts for its encoding/batch.
• FP32: KNN c8c02d5c436e7561, range a8791bd19b32cc97, serialized ec785c19cea62734, 10,398,147 bytes.
• FP16: KNN eba60050ecc56435, range 1ffdf5c7871487ff, serialized 658e543a8b68023a, 7,905,646 bytes.
• SQ8: KNN 7224cdf82ecda77b, range ab7ebf6bbfe94ba2, serialized 5beb0893264ad66d, 6,662,690 bytes.
• Median KNN/range p50 and p99 changes stay within +/-3% across the measured cells; the largest measured regression is +2.64% (SQ8 KNN p99, batch 256).
• The baseline cached GetMemoryUsage() misses vectors added after a window is created, so it is not used for the A/B memory conclusion. Linux mallinfo2 heap differs by only +0.12% to +0.23%. The full matrix observed point-in-time RSS changes of +4.6 to +4.8 MiB; a separate order-balanced FP32/batch-256 breakdown (5 processes/version) measured median RSS +1.39 MiB, comprising only +0.02 MiB anonymous RSS and +1.37 MiB file-backed RSS. This indicates no material retained-heap increase in the measured workload.

Documentation Impact

• No docs update needed
• Updated docs:
  • README.md
  • DEVELOPMENT.md
  • CONTRIBUTING.md
  • Other:

The optimization does not add or change a user-facing parameter or workflow.

Risk and Rollback

• Risk level: medium; query/pruning code must merge two posting runs exactly until normalization.
• Rollback plan: revert this commit to restore eager full-list sorting after each Add.

Checklist

• I have linked the relevant issue
• I have added/updated tests for new behavior
• I have considered API compatibility impact
• I have updated docs if behavior/workflow changed (not applicable; no user-facing change)
• My commit messages follow project conventions (Conventional Commits, optional [skip ci] prefix)

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao

[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/

[LHT129]

Overall this is a well-engineered PR with thorough test coverage and detailed performance benchmarks. The core idea — deferring full posting-list normalization by maintaining a sorted prefix plus a bounded sorted dirty suffix — is sound and delivers significant incremental Add throughput improvements (up to 10x for batch-size-1).

Summary of findings:

1. [suggestion] const_cast in Serialize methods — The const_cast<SINDI*>(this)->cal_memory_usage() calls break the logical const contract. Consider making the cached memory usage mutable or refactoring the normalization+recalc into a pre-serialization helper.

2. [note] Serialize lock upgrade — Changing from shared_lock to scoped_lock in serialization is a necessary trade-off to normalize dirty postings before writing. The PR description already documents this; no action needed.

3. [suggestion] Redundant ScanPostingRange call on hot path — When postings are fully normalized (the common case), the second ScanPostingRange call has term_count=0 and returns immediately. A guard on suffix_count > 0 would avoid the function call overhead.

What is done well:

• The binary-search partition in SelectPostingRuns is correct and well-tested across all three quantization types.
• ForEachSelectedPosting elegantly merges two sorted runs in heap-insertion order without materializing a merged array.
• Test coverage is comprehensive: deterministic correctness, serialization round-trips, filter paths, randomized stress tests (20 seeds), and all three quantization encodings.
• The normalization threshold clamping (MIN_DIRTY_POSTING_SIZE=32, MAX_DIRTY_POSTING_SIZE=256) provides a good balance between deferring work and bounding the dirty suffix size.
• Performance data is thorough and reproducible, with identical A/B result hashes confirming correctness.

[LHT129]

Overall this is a well-engineered optimization with thorough test coverage. The performance improvements are impressive (up to 10x for single-vector incremental Add).

Suggestions:

1. src/algorithm/sindi/sindi.cpp:1587 and :1737 — [suggestion] The Serialize and serialize_streaming_body methods are marked const but modify internal state by calling NormalizeDirtyPostings() (which mutates posting lists) and cal_memory_usage() via const_cast. This violates logical const-correctness — callers of a const method do not expect side effects that change the object's observable state. Consider making these methods non-const, or marking the affected mutable state as mutable with documentation that serialization is a normalization boundary.

2. src/datacell/sparse_term_datacell.cpp:143 — [note] The binary search in SelectPostingRuns correctly partitions two sorted runs. The fallthrough CHECK_ARGUMENT(false, ...) is an unreachable defensive guard — consistent with the codebase convention. The four uncovered lines noted in the PR description correspond to this and similar defensive paths.

[wxyucs]

/kind improvement
/version 1.1
/assign @Roxanne0321

[LHT129]

This is a well-executed optimization that defers full posting-list re-sorting during incremental Add by maintaining a sorted prefix plus a bounded sorted dirty suffix. The core algorithm (SelectPostingRuns binary search, ForEachSelectedPosting merge, NormalizePosting two-way merge) is correct and well-tested across all three quantization types with deterministic, randomized, and lifecycle coverage.

Summary of observations (no blocking issues):

1. The const_cast removal (via mutable on current_memory_usage_ and const on cal_memory_usage()) cleanly resolves the const-correctness concern in serialization paths.

2. The exclusive lock in Serialize / serialize_streaming_body is a necessary trade-off to guarantee postings are normalized before writing, and is clearly documented in the PR description.

3. The query_impl suffix scan guard (if (selected.suffix_count > 0)) noted in a prior review comment has been addressed — this avoids an unnecessary function call on the hot path for fully normalized postings.

4. The Compact() method calls FinalizeInsertBatch() defensively but does not call NormalizeDirtyPostings(). This is safe because all current callers normalize before compacting, but future callers should be aware of this invariant.

5. Test coverage is thorough: 98.28% changed-line coverage, all three quantization types, KNN/range/filter paths, serialization round-trips, randomized differential testing with 20 seeds, and wire format version retention.

The performance improvements (up to 10x for single-vector incremental adds) are well-supported by controlled A/B benchmarks with identical KNN/range/serialized hashes confirming result correctness.

[LHT129]

I have reviewed this PR and the changes look solid. The deferred normalization strategy for incremental posting batches is well-designed and the test coverage is comprehensive.

Summary of changes:

• SparseTermDataCell: Introduces two-run posting lists with deferred normalization via FinalizeInsertBatch() and NormalizeDirtyPostings(). Query and heap-insert paths correctly handle split runs through SelectPostingRuns() and ForEachSelectedPosting().
• SINDI::Add(): Replaces eager SortByValue() with batched FinalizeInsertBatch() and defers full normalization until a window reaches capacity.
• SINDI::Build(): Normalizes dirty postings before compacting.
• Serialization (Serialize / serialize_streaming_body): Upgraded to exclusive lock and normalizes dirty postings before writing to ensure canonical output.
• cal_memory_usage(): Made const with mutable memory-usage atomic.
• Tests: Updated existing test, added serialization normalization test, and added comprehensive SelectPostingRuns and two-run heap equivalence tests.

Key observations:

1. The binary search in SelectPostingRuns correctly finds the optimal partition between sorted prefix and suffix runs. The monotonicity property holds because as more entries are taken from the suffix, the boundary entries move in consistent directions.

2. The ForEachSelectedPosting template correctly merges two sorted runs in value-descending order, maintaining the same traversal semantics as the original single-run iteration.

3. The lock upgrade in serialization from shared to exclusive is necessary since normalization mutates internal state. This is an intentional trade-off: serialization now blocks concurrent reads but produces canonical output.

4. The term >= term_ids_.size() || term_sizes_[term] == 0 guard added to InsertHeapByTermLists fixes a missing null-pointer check that was already present in query_impl.

No blocking issues found. The existing review comments have been properly addressed (mutable atomic, exclusive lock rationale, suffix_count zero-guard).

[LHT129]

I have completed a thorough review of this PR. The implementation is well-structured and the test coverage is comprehensive (976 additions across 7 files with extensive unit tests covering all three quantization types, serialization round-trips, randomized differential testing, and heap equivalence).

Summary of findings:

The core approach — maintaining each mutable SINDI posting list as a sorted prefix plus a bounded sorted dirty suffix — is sound. The binary search in SelectPostingRuns correctly partitions the two sorted runs, and the merge logic in both ForEachSelectedPosting (for heap insertion) and NormalizePosting (for full normalization) is correct.

Previous review comments addressed:

• The const_cast in serialization has been eliminated by making current_memory_usage_ mutable and cal_memory_usage() const.
• The unconditional second ScanPostingRange call in query_impl is now guarded by if (selected.suffix_count > 0).
• The exclusive lock for serialization is confirmed as intentional and documented.

Remaining notes (non-blocking, already raised by LHT129):

• normalize_dirty_postings_for_serialization() is declared const but mutates posting list internals — the existing comment explains the logical const semantics.
• Scratch vector allocations in FinalizeInsertBatch are created on the stack before the early-return check — negligible overhead.

The performance data in the PR description shows impressive improvements (up to 10x for single-vector adds) with no regression in search accuracy or serialization compatibility. The controlled A/B methodology is rigorous.

No blocking issues found. The PR looks ready to merge from a code review perspective.

[LHT129]

I have completed a thorough review of this PR. The existing inline comments from the previous review round have already covered the main areas of concern, and the author has addressed several of them (e.g., making current_memory_usage_ mutable, guarding the suffix scan).

Summary of my review:

Correctness: The core algorithm — SelectPostingRuns binary search, NormalizePosting two-way merge, and ForEachSelectedPosting merge-traversal — all appear correct. The PostingEntryBefore comparator correctly implements descending-by-value with ascending-by-id tiebreaking, consistent with the existing SortPostingListByValue ordering.

Concurrency: The lock upgrade from shared_lock to scoped_lock in Serialize/serialize_streaming_body is intentional and necessary for the normalization-before-write guarantee. Search paths continue to use shared locking, and the dirty-posting data structures are only mutated under the exclusive lock.

Memory: The scratch vectors (normalization_ids_scratch_, normalization_data_scratch_) are properly released via ReleaseNormalizationScratch() at appropriate boundaries. GetMemoryUsage() accounts for the new data structures.

Test coverage: The new tests cover all three quantization types (FP32/FP16/SQ8), dirty-run lifecycle (thresholds, boundaries, explicit normalization), serialization round-trips (both ordinary and streaming), heap equivalence (KNN/range, pure/filtered), and randomized differential testing. This is thorough.

One observation (not blocking): In Compact(), FinalizeInsertBatch() is called at the start, but Build() already calls NormalizeDirtyPostings() (which internally calls FinalizeInsertBatch()) immediately before Compact(). The second call is a no-op (dirty map is already empty), so this is harmless defensive coding.

No new blocking issues found. The existing review thread covers the remaining suggestions.

[LHT129]

I have reviewed the latest commit (25e45f8) which addresses the earlier review feedback from LHT129. The changes look good:

1. const_cast removal - cal_memory_usage() is now const and current_memory_usage_ is mutable std::atomic<uint64_t>. The serialization paths no longer need const_cast. Clean solution.

2. Suffix scan guard - The second ScanPostingRange call in query_impl is now guarded by if (selected.suffix_count > 0), avoiding the unnecessary function call for the common fully-normalized case.

3. normalize_dirty_postings_for_serialization() extraction - The normalization logic is now factored into a dedicated helper, reducing duplication between Serialize and serialize_streaming_body.

4. Test coverage - Comprehensive tests cover all three quantization types (FP32/FP16/SQ8), dirty-run thresholds and boundaries, KNN/range search with pure and filtered paths, serialization round-trips, randomized differential testing, and wire format version retention.

The core algorithm (binary-search partition of two sorted runs via SelectPostingRuns, merge-on-the-fly via ForEachSelectedPosting, and bounded normalization via FinalizeInsertBatch/NormalizeDirtyPostings) is sound. The performance data (3-10x Add throughput improvement) is compelling and the A/B testing confirms result-identical output.

No blocking issues found in the latest revision.

[LHT129]

Code Review Summary

I have completed a thorough review of this PR. The overall quality is high — the dirty posting mechanism is well-designed, the test coverage is comprehensive (98.28% changed-line coverage), and the existing review comments from LHT129 have addressed the main substantive issues.

Issues already identified and addressed by the author:

• const_cast elimination: current_memory_usage_ is now mutable, and cal_memory_usage() is const. ✅
• ScanPostingRange guard: The second ScanPostingRange call is now guarded by if (selected.suffix_count > 0). ✅
• Lock upgrade documentation: The exclusive lock in Serialize/serialize_streaming_body is acknowledged as intentional. ✅

Remaining items for consideration:

[suggestion] normalize_dirty_postings_for_serialization() const-correctness
The method is declared const in both the header (sindi.h:360) and implementation (sindi.cpp:1578), but it calls window->NormalizeDirtyPostings() which is a non-const mutating method. While this works because const shared_ptr<T>& yields a non-const T* via operator->, the const qualifier is misleading to readers. The header comment already notes "The caller must hold global_mutex_" which partially addresses this. Consider either removing const from this method and its callers (Serialize, serialize_streaming_body), or adding a brief comment explaining the logical const pattern.

[note] FinalizeInsertBatch always returns true after processing
As LHT129 noted, when every pending term is missing from dirty_posting_prefixes_ (the continue branch at line 603), no posting state actually changed but the method still returns true. The caller in SINDI::Add() uses this return value to decide whether to call cal_memory_usage(). This is a minor inefficiency — cal_memory_usage() is cheap, so the impact is negligible.

[note] Redundant ReleaseNormalizationScratch() in NormalizeDirtyPostings
When FinalizeInsertBatch() already emptied dirty_posting_prefixes_, NormalizeDirtyPostings calls ReleaseNormalizationScratch() again (line 649) even though FinalizeInsertBatch already released it (line 633). The double-release is harmless (swap with already-empty vectors is a no-op), but slightly redundant.

What this PR does well:

• Clean separation of sorted prefix + unsorted dirty suffix with bounded thresholds (MIN=32, MAX=256)
• Binary partition (SelectPostingRuns) correctly handles all edge cases including fully-dirty and fully-clean terms
• ForEachSelectedPosting template enables efficient merged iteration for heap insertion
• Memory accounting covers all new data structures
• Tests cover all three quantization types, serialization round-trips, filter correctness, and randomized stress scenarios
• Backward compatible wire format (version 1 retained)

[LHT129]

I have completed a thorough review of this PR. The overall code quality is high, with well-structured changes and comprehensive test coverage.

Summary of changes reviewed:

1. sparse_term_datacell.h/cpp — Core of the optimization. The two-run posting list design (sorted prefix + bounded dirty suffix) with binary-search partition (SelectPostingRuns) and two-pointer merge (NormalizePosting, ForEachSelectedPosting) is correct and well-implemented. The FinalizeInsertBatch → NormalizeDirtyPostings lifecycle is properly integrated into SINDI::Add, SINDI::Build, and serialization paths.

2. sindi.cpp/h — The cal_memory_usage() const-correctness fix and normalize_dirty_postings_for_serialization() helper are clean. The exclusive lock upgrade in serialization is intentional and documented.

3. inner_index_interface.h — Making current_memory_usage_ mutable is appropriate since it is a cached value, not logical state.

4. Tests — Excellent coverage: lifecycle tests (partial/full window, serialization boundaries), SelectPostingRuns correctness across all three quantization types, heap equivalence for KNN/range/filter paths, and a 20-seed randomized stress test. The test for serialization round-trip with dirty postings is particularly valuable.

Existing review comments from LHT129 have been addressed:

• const_cast eliminated via mutable current_memory_usage_
• Unnecessary ScanPostingRange call now guarded by suffix_count > 0
• FinalizeInsertBatch return value now accurately reflects whether changes occurred
• Scratch vector allocation is now lazy (only after first dirty term found)

Remaining open items (already flagged by LHT129):

• Dead code after CHECK_ARGUMENT(false, ...) in SelectPostingRuns and PostingEntryBefore — minor, non-blocking

The performance data in the PR description is thorough and convincing, with 1.6x–10x Add throughput improvements across all quantization types and batch sizes. The A/B verification confirms identical KNN/range/serialization results.

No new blocking issues found. The PR is in good shape.

[LHT129]

Thanks for this well-crafted optimization. The two-run posting list design with lazy normalization is a clean approach that delivers substantial incremental Add throughput improvements (up to 10x for single-vector batches) while preserving exact query semantics.

Summary of findings from this review pass:

The code quality is high. Most issues raised in earlier review rounds have been addressed:

• const_cast removed via mutable on current_memory_usage_ and const cal_memory_usage()
• Suffix scan guarded by suffix_count > 0 on the hot query path
• FinalizeInsertBatch return value now accurately reflects whether state changed
• Scratch vectors lazily initialized only when needed

One minor note added:

• FinalizeInsertBatch() is called twice when a window becomes full in SINDI::Add() (once directly, once inside NormalizeDirtyPostings()). The second call is a fast no-op but represents minor redundancy.

Unaddressed from prior reviews (LHT129, Aug 18):

• Dead code after CHECK_ARGUMENT(false, ...) in SelectPostingRuns (line 152) and PostingEntryBefore (line 536). These return statements are unreachable but harmless.

The test coverage is excellent — spanning all three quantization types, dirty-run thresholds, boundaries, serialization round-trips, randomized differential testing, and heap equivalence verification. The A/B benchmark methodology is rigorous and the results are compelling.

[LHT129]

Code Review Summary

I have completed a thorough review of this PR (commits 25e45f80 and d92c1973). The deferred posting-list normalization strategy is well-designed, and the implementation quality is high. The test coverage is comprehensive across all three quantization types (FP32, FP16, SQ8) with deterministic, randomized, lifecycle, serialization, and heap-equivalence coverage.

What was already addressed (from LHT129's prior review)

• const_cast eliminated by making current_memory_usage_ mutable and cal_memory_usage() const
• FinalizeInsertBatch() now correctly returns posting_state_changed (only true when actual work was done)
• Scratch vectors (order, sorted_ids, sorted_data) are lazily initialized inside FinalizeInsertBatch
• Suffix scan in query_impl is guarded by suffix_count > 0
• Documentation added for normalize_dirty_postings_for_serialization explaining the logical const contract

Remaining open items (from LHT129's latest review round)

1. [suggestion] Dead code after CHECK_ARGUMENT in SelectPostingRuns (sparse_term_datacell.cpp:144): The return {}; is unreachable because CHECK_ARGUMENT(false, ...) always throws. Consider replacing with __builtin_unreachable() or removing it.

2. [note] Dead code after CHECK_ARGUMENT in PostingEntryBefore (sparse_term_datacell.cpp:146): Same pattern — return false; after the switch-default CHECK_ARGUMENT. Consider the same treatment for consistency.

3. [note] Redundant FinalizeInsertBatch call in SINDI::Add() (sindi.cpp:409-411): When a window becomes full (total_count_ == window_size_), FinalizeInsertBatch() is called first at line 409, then NormalizeDirtyPostings() at line 411 calls FinalizeInsertBatch() again internally. The second call is a fast no-op (empty pending_posting_terms_), but the redundancy is worth noting.

Additional observations

• The SelectPostingRuns binary search correctly partitions two sorted runs to find the top-k boundary. The edge cases (retained_count == 0, retained_count == posting_count, no dirty postings) are all handled correctly.
• The ForEachSelectedPosting merge loop correctly interleaves prefix and suffix entries in sorted order, matching the semantics of the old single-run scan.
• The exclusive lock change in Serialize and serialize_streaming_body is an intentional and necessary trade-off to ensure postings are fully normalized before writing. This is well-documented in the PR description.
• NormalizeDirtyPostings return value semantics are correct: returns true when it actually performed normalization work, false when there was nothing to normalize (including when FinalizeInsertBatch already handled everything).

Verdict

This is a well-executed optimization with impressive performance results (up to 10x throughput improvement for single-vector incremental Add). The remaining open items are all non-blocking [note]/[suggestion] level. No critical issues found.

[LHT129]

Overall this is a well-engineered optimization with thorough test coverage (98.28% changed-line coverage), comprehensive A/B benchmarks showing 1.57x–10.11x Add throughput improvements with no regression in search accuracy, and careful attention to serialization compatibility (wire version unchanged).

The core idea — deferring full posting-list normalization by maintaining a sorted prefix + bounded sorted dirty suffix — is sound and the merge-during-query approach preserves correctness. The lifecycle management (normalize at window-full, build boundary, compaction, and serialization) covers all necessary synchronization points.

Most of the detailed feedback has already been addressed in the existing review threads. The remaining open items are minor notes/suggestions that do not block merge.

[LHT129]

I completed a thorough review of all changed files in this PR. The existing reviews from LHT129 have already covered the key issues, and the author has addressed the important ones:

1. const_cast removal — cal_memory_usage() is now const and current_memory_usage_ is mutable. Clean solution.
2. suffix_count guard in query_impl — The hot-path ScanPostingRange call is now guarded by if (selected.suffix_count > 0). Good.
3. Exclusive lock in serialization — Confirmed as intentional design choice, documented in the PR description.

The core algorithm (SelectPostingRuns binary search + ForEachSelectedPosting merge) is correct and well-tested across all three quantization types with randomized coverage. The FinalizeInsertBatch threshold heuristic (clamp(prefix_count, 32, 256)) is reasonable.

The remaining open [note] items (dead code after CHECK_ARGUMENT, double FinalizeInsertBatch call in Add, int64_t loop variable) are minor and non-blocking.

No new issues found. The test coverage is comprehensive (deterministic lifecycle, serialization, filtering, boundary, randomized, and three-quantization regression tests).

[LHT129]

Reviewed the latest commit (d92c197). All three issues from the previous review round have been addressed:

1. const_cast removed — cal_memory_usage() is now const, current_memory_usage_ is mutable, and normalize_dirty_postings_for_serialization is const with clear documentation explaining why physical mutation is safe under the caller-held exclusive lock.

2. suffix_count > 0 guard added — query_impl now guards the suffix scan with if (selected.suffix_count > 0).

3. Lazy scratch allocation — FinalizeInsertBatch uses std::optional<Vector<>> for on-demand allocation of sort scratch buffers.

The implementation is solid:

• Two-run posting representation (sorted prefix + bounded dirty suffix) is well-designed
• Binary search partitioning in SelectPostingRuns correctly handles all edge cases
• Merge-on-read during queries (ForEachSelectedPosting, InsertHeapByTermLists) is correct
• Lock discipline is sound: exclusive lock for serialization (intentional, documented), shared lock for queries (unchanged)
• Tests cover all three quantization types, dirty posting lifecycle, and serialization round-trip
• No correctness issues found

One minor observation (not blocking): FinalizeInsertBatch returns posting_state_changed = true even when a term only had its suffix sorted without a merge (prefix_count == 0 case). This triggers a harmless cal_memory_usage() recalculation since the posting list size hasn't changed. This is on the write path, not the hot query path, so the impact is negligible.

[LHT129]

This is a well-structured optimization with comprehensive test coverage and thorough benchmarking. The core idea of deferring full posting-list normalization by maintaining a sorted prefix + bounded sorted dirty suffix is sound.

The latest commit (d92c197) addresses several important review items: const_cast removal via mutable cache, lazy scratch-vector initialization with std::optional, correct FinalizeInsertBatch return value, and suffix-scan guard. The remaining open review threads (dead code after CHECK_ARGUMENT, int64_t loop variable, double FinalizeInsertBatch call in Add) are all [note] level and do not block merging.

The test coverage is excellent: deterministic lifecycle tests, serialization round-trip tests with all three quantization types, randomized differential tests, and heap-equivalence verification for both KNN and range search with filters. The A/B benchmarks show substantial incremental Add throughput improvements (1.6x-10x) with no regression in search accuracy or serialized output.

[LHT129]

Review Summary

This PR implements batch posting normalization for incremental SINDI Add, deferring full-list re-sorting and instead maintaining a sorted prefix plus a bounded dirty suffix. The merge-on-read strategy during query/pruning is sound, and normalization boundaries are correctly placed at full-window, build, compaction, and serialization points.

What was already reviewed and addressed

Previous review rounds by @LHT129 covered several items, all of which have been resolved or acknowledged:

1. const_cast in serialization — Resolved by making current_memory_usage_ mutable and cal_memory_usage() const.
2. Exclusive lock in Serialize — Confirmed as intentional design choice for normalization-before-write.
3. Redundant ScanPostingRange call when no dirty postings — Fixed with if (selected.suffix_count > 0) guard.
4. Dead code after CHECK_ARGUMENT in PostingEntryBefore — Noted as a defensive pattern consistent with the rest of the codebase.
5. Double FinalizeInsertBatch call in SINDI::Add() — Minor redundancy; the second call is a fast no-op.
6. int64_t loop variable for container indexing — Style note; non-negative in practice.

Positive observations

• Algorithm correctness: The binary-search partition in SelectPostingRuns correctly finds the split point between two sorted runs. The merge logic in ForEachSelectedPosting and NormalizePosting is consistent and correct.
• Test coverage: Comprehensive — covers all three quantization types (FP32/FP16/SQ8), randomized stress tests with 20 seeds, serialization round-trips, heap equivalence (KNN + range, pure + filtered), and lifecycle boundaries.
• Performance: The benchmark data shows 1.6x–10x Add throughput improvement with no search regression, which is impressive.
• Wire compatibility: SINDI posting list format version remains 1; serialized bytes are unchanged.
• Memory: No material heap increase; scratch buffers are released after use via ReleaseNormalizationScratch.

No new blocking or substantive issues found. The implementation is solid.

[LHT129]

This PR has already received a thorough review from LHT129 with 18 inline comments covering correctness, performance, code style, and maintainability concerns. The author has been responsive and addressed many of the initial findings in the second commit (d92c197).

The remaining 4 unresolved comments are all [suggestion]/[note] level:

• Dead code after CHECK_ARGUMENT in SelectPostingRuns and PostingEntryBefore — minor style issue, safe as-is since CHECK_ARGUMENT always throws.
• Double FinalizeInsertBatch() call when a window becomes full in SINDI::Add() — the second call is a fast no-op, no correctness impact.
• int64_t loop variable for indexing window_term_list_ — minor type inconsistency, safe in practice since the index is derived from cur_element_count_ / window_size_.

I have no additional blocking concerns to add. The PR is well-structured with comprehensive test coverage (unit tests, randomized stress tests, serialization round-trip tests across all three quantization types), thorough performance benchmarks, and clear documentation of the design decisions.

[LHT129]

I have completed a thorough review of this PR. The changes have already been well-reviewed by LHT129, and the author has addressed the key feedback items (const_cast removal, ScanPostingRange guard, exclusive lock justification).

Summary of my review:

The core approach — maintaining a sorted prefix plus a bounded dirty suffix to defer full-list re-sorting during incremental Add — is sound and well-motivated. The binary search in SelectPostingRuns correctly partitions two sorted runs, and the merge logic in ForEachSelectedPosting and NormalizePosting is correct.

The test coverage is excellent: the PR adds deterministic lifecycle tests, serialization round-trip tests with wire format version verification, filtering boundary tests, randomized differential tests across all three quantization types (FP32/FP16/SQ8), and heap equivalence tests for both KNN and range search with pure and filtered paths.

The performance data is thorough and convincing, with 1.6x-10x Add throughput improvements depending on batch size, and search latency within normal variation.

Remaining items from prior review (all [note] level):

1. Dead code after CHECK_ARGUMENT(false, ...) in both SelectPostingRuns (return {};) and PostingEntryBefore (return false;) — these are unreachable but harmless.
2. Double FinalizeInsertBatch() call in SINDI::Add() when a window becomes full — the second call is a fast no-op.
3. int64_t loop variable for window indexing — minor type inconsistency.

None of these are blocking. The PR is in good shape for merging once the author decides on the remaining [note] items.

[LHT129]

I have completed a thorough review of this PR. The existing review comments from LHT129 have already covered the key points:

1. The const_cast issue has been resolved by making current_memory_usage_ mutable and cal_memory_usage() const.
2. The hot-path suffix_count > 0 guard has been added to query_impl.
3. Dead-code-after-CHECK_ARGUMENT patterns in SelectPostingRuns and PostingEntryBefore have been noted.
4. The redundant FinalizeInsertBatch call in Add() when a window becomes full has been noted.
5. The signed loop variable for window indexing has been noted.

The PR is well-structured with comprehensive test coverage (deterministic lifecycle, serialization, filtering, boundary, randomized, and three-quantization regression tests). The performance data is thorough and convincing. The design of maintaining a sorted prefix + bounded sorted dirty suffix is sound, and the merge-during-pruning approach correctly preserves existing query semantics.

No new blocking issues found. The remaining open [note]-level comments are non-blocking and can be addressed at the author's discretion.

[LHT129]

This is a follow-up review after the second commit (d92c197). The previous review by @LHT129 already covered the key issues thoroughly, and most actionable suggestions have been addressed in this revision.

Summary of resolved issues from the first review round:

• const_cast removed: current_memory_usage_ is now mutable, cal_memory_usage() is const — clean.
• suffix_count == 0 guard added in query_impl to skip the second ScanPostingRange — addressed.
• FinalizeInsertBatch now returns true only when at least one dirty term is processed — addressed.
• Scratch vector allocation in FinalizeInsertBatch is now deferred until the first valid dirty term — addressed.
• Serialization exclusive lock is confirmed as intentional and documented.

Remaining open notes from @LHT129 (non-blocking):

• Dead code after CHECK_ARGUMENT in SelectPostingRuns (return {};) and PostingEntryBefore (return false;).
• FinalizeInsertBatch called twice when a window becomes full in SINDI::Add() (once directly, once inside NormalizeDirtyPostings).
• int64_t window loop variable vs size_t container index.

My assessment:
The core algorithm — maintaining a sorted prefix + bounded dirty suffix, merging on-the-fly during queries, and normalizing at boundaries — is sound. The two-run merge logic in SelectPostingRuns, ForEachSelectedPosting, and NormalizePosting is correct. Test coverage is extensive (deterministic lifecycle, serialization round-trips, randomized differential tests across all three quantization types). The performance data in the PR description is thorough and shows significant incremental Add throughput improvements with no regression in search accuracy.

No new issues found beyond what has already been noted. The remaining notes are minor and do not block merge.

[LHT129]

I have completed a thorough review of the latest commit (d92c197). The previous round of review comments has been well addressed:

1. The const_cast in serialization paths has been eliminated by making cal_memory_usage() const and current_memory_usage_ mutable, with normalization factored into a dedicated normalize_dirty_postings_for_serialization() helper.
2. The hot-path ScanPostingRange call is now guarded by if (selected.suffix_count > 0), avoiding the no-op function call for fully normalized postings.
3. The exclusive lock in serialization is an intentional design choice documented in the PR description.

The remaining open notes (dead code after CHECK_ARGUMENT, redundant FinalizeInsertBatch call in the full-window path, and int64_t loop variable) are minor and non-blocking.

The core algorithm — binary-search partitioning of two sorted posting runs, merge-based normalization, and bounded dirty-suffix thresholds — is correct and well-tested with comprehensive coverage across all three quantization types, randomized seeds, and lifecycle boundaries. No new issues found.

[LHT129]

This is the third review round for this PR. The second commit (d92c197) addressed 5 out of 9 issues from the previous review rounds:

Resolved:

1. const_cast eliminated — current_memory_usage_ made mutable, cal_memory_usage() made const ✅
2. ScanPostingRange suffix guard added (if (selected.suffix_count > 0)) ✅
3. FinalizeInsertBatch now returns posting_state_changed (only true when at least one dirty term is processed) ✅
4. Scratch vectors (order, sorted_ids, sorted_data) are now lazily allocated via std::optional ✅
5. normalize_dirty_postings_for_serialization const documentation improved ✅

Remaining (all [note] level, previously flagged by LHT129):

• PostingEntryBefore dead code after CHECK_ARGUMENT (sparse_term_datacell.cpp:538)
• SelectPostingRuns dead code after CHECK_ARGUMENT (sparse_term_datacell.cpp:152-153)
• FinalizeInsertBatch called twice in SINDI::Add() — once directly, then inside NormalizeDirtyPostings() (sindi.cpp:409/411)
• int64_t loop variable for container indexing (sindi.cpp:408)

These are all minor style issues that do not affect correctness or performance. No new issues found in this review round.

Overall assessment: The implementation is solid. The core algorithm (deferred batch posting normalization with bounded dirty suffix) is correctly implemented with proper binary search partitioning, two-way merge, and threshold-triggered normalization. Test coverage is thorough across FP32/FP16/SQ8 quantization types, including randomized differential tests. Serialization correctly normalizes dirty state before writing. Memory accounting covers all new data structures.

LGTM.

[LHT129]

[note] In Compact(), the newly created compact_pending_flags vector is zero-initialized without copying the existing pending_posting_flags_ values. While FinalizeInsertBatch() is called at the start of Compact() and clears flags for terms in pending_posting_terms_, there is a subtle edge case: if any terms exist in dirty_posting_prefixes_ but were not tracked in pending_posting_terms_ (the "unprocessed insert batch" scenario), their corresponding flags would be silently dropped after compaction. The current callers are safe because SINDI::Build() calls NormalizeDirtyPostings() before Compact(), but future callers of Compact() may not be aware of this precondition. Consider either copying the existing flags into compact_pending_flags, or documenting that Compact() expects dirty postings to already be normalized.