rebuild BM25 from nodes and harden index invariants - #883
Merged
Conversation
Make BM25 explicitly node-only by removing vector indexing, rebuild schema v2 from node state on startup, and fail loudly when forward, reverse, and doc-length state diverge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Greptile Summary
This PR hardens the BM25 full-text search subsystem by introducing a reverse index (
doc_id → list of (term, tf)) alongside the existing inverted index, enabling correct and efficient delete/update operations without re-scanning the inverted index. It also adds schema versioning so that on startup, if the stored index was built without the reverse index, the migration layer automatically clears and rebuilds the entire BM25 index fromnodes_db. Theadd_n,update, andupserttraversal operators are updated to maintain BM25 invariants as nodes are created or changed.Key changes:
bm25.rs: AddsReversePostingEntry,DocPresence/DocStateenums,reverse_index_db(DUP_SORT), schema version read/write, and refactoredinsert_doc/delete_doc/update_docthat enforce strict consistency invariants (returns errors on index corruption).storage_migration.rs:migrate_bm25clears and rebuilds the BM25 index in 1 024-node batches when the stored schema version doesn't matchBM25_SCHEMA_VERSION = 2.add_n.rs:bm25.insert_docis called after writing the node — but without a guard that checks whether thenodes_dbwrite succeeded, allowing BM25 errors to mask prior errors.update.rs/upsert.rs: BM25 is correctly updated before (update) or after (upsert new node) thenodes_dbwrite, with proper error propagation.Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant migrate_bm25 participant BM25Index participant nodes_db participant add_n / update / upsert Note over Caller, BM25Index: Startup Migration Path Caller->>migrate_bm25: migrate(storage) migrate_bm25->>BM25Index: schema_version(read_txn) alt version matches BM25_SCHEMA_VERSION BM25Index-->>migrate_bm25: Some(2) — skip else outdated or missing migrate_bm25->>BM25Index: clear_all(write_txn) migrate_bm25->>nodes_db: iter(read_txn) — batch by 1024 loop Each batch migrate_bm25->>BM25Index: insert_doc per node with properties (write_txn) end migrate_bm25->>BM25Index: write_schema_version(2, write_txn) end Note over Caller, BM25Index: Normal Write Path Caller->>add_n / update / upsert: write op (RwTxn) add_n / update / upsert->>nodes_db: put node add_n / update / upsert->>BM25Index: insert_doc / update_doc / delete_doc BM25Index->>BM25Index: update inverted_index_db (term→postings) BM25Index->>BM25Index: update reverse_index_db (doc_id→terms) BM25Index->>BM25Index: update doc_lengths_db BM25Index->>BM25Index: update term_frequencies_db BM25Index->>BM25Index: update metadata (total_docs, avgdl) add_n / update / upsert-->>Caller: Result<TraversalValue>Comments Outside Diff (1)
helix-db/src/helix_engine/traversal_core/ops/source/add_n.rs, line 141-148 (link)BM25 insert runs unconditionally, can mask prior errors
The BM25
insert_doccall (lines 141-148) runs even when a previous operation failed (secondary-index insertion ornodes_db.put_with_flags). Ifbm25.insert_docthen also fails, it silently overwrites the original error stored inresult, making the caller receive a BM25 error instead of the realnodes_dbor secondary-index error. Adding an early-exit guard fixes both the error masking and the wasted BM25 work:Last reviewed commit: 8d28406