feat(search): native hybrid search with score fusion (fuse + hybrid)#9738
Open
shaunpatterson wants to merge 54 commits into
Open
feat(search): native hybrid search with score fusion (fuse + hybrid)#9738shaunpatterson wants to merge 54 commits into
shaunpatterson wants to merge 54 commits into
Conversation
Add BM25 relevance-ranked text search to Dgraph, enabling users to query text predicates and receive results ordered by relevance score instead of boolean matching. Implementation: - New BM25 tokenizer using the fulltext pipeline (normalize, stopwords, stem) that preserves term frequencies for TF counting - BM25-specific index storage: per-term TF posting lists, doc length lists, and corpus statistics (doc count, total terms) - Query execution with full BM25 scoring: score = IDF * (k+1) * tf / (k * (1 - b + b * dl/avgDL) + tf) IDF = log1p((N - df + 0.5) / (df + 0.5)) - DQL syntax: bm25(predicate, "query" [, "k", "b"]) as root func or filter - Schema syntax: @index(bm25) - Parameter validation (k > 0, 0 <= b <= 1) - Early UID intersection for filter-mode performance - All-stopword document and query handling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three critical bugs fixed: 1. REF postings lose Value during rollup: The posting list encode/rollup cycle strips the Value field from REF postings without facets (list.go:1630). BM25 term frequencies and doc lengths were stored in Value and lost. Fix: Store TF and doclen as facets on REF postings, which are preserved. 2. Missing function validation: query/query.go has a separate isValidFuncName check from dql/parser.go. "bm25" was only added to the parser, causing "Invalid function name: bm25" at query time. 3. Unsorted UIDs break query pipeline: BM25 returned UIDs sorted by score, but the query pipeline (algo.MergeSorted, child predicate fetching) requires UID-ascending order. Fix: Sort UIDs ascending in UidMatrix, apply first/offset pagination on score-sorted results before UID sorting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the facet-based BM25 storage (~40-50 bytes/posting) with compact varint-encoded binary blobs stored as direct Badger KV entries (~4-6 bytes/posting, ~10x reduction). Add bm25_score pseudo-predicate for variable-based score ordering following the similar_to pattern. - Add posting/bm25enc package for compact binary encode/decode - Rewrite write path in posting/index.go for direct Badger KV - Add bm25Writes buffer to LocalCache with read-your-own-writes - Flush BM25 blobs in CommitToDisk with BitBM25Data UserMeta - Rewrite read path in worker/task.go with direct blob decoding - Add bm25_score pseudo-predicate in query/query.go - Add score ordering integration tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cases Cover incremental add/update/delete, IDF score stability as corpus grows, large corpus pagination, unicode, stopwords, uid filtering, score validation, and concurrent batch adds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…c tests Addresses test coverage gaps identified during code review against ArangoDB's BM25 implementation: - TestBM25ExactScoreValues: validates numerical correctness of BM25 formula using b=0 to enable hand-computed expected scores - TestBM25BM15NoLengthNormalization: verifies b=0 disables length normalization and contrasts with default b=0.75 behavior - TestBM25SingleMatchingDocument: covers df=1 edge case with high IDF Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 1 of BM25 scaling plan. Introduces bm25block package with: - BlockMeta/Dir types for block directory encoding/decoding - SplitIntoBlocks: splits monolithic entry slices into 128-entry blocks - MergeAllBlocks: compacts overlapping blocks with dedup and tombstone removal - ComputeUBPre/SuffixMaxUBPre: WAND upper-bound precomputation - New key functions: BM25TermDirKey, BM25TermBlockKey, BM25DocLenDirKey, BM25DocLenBlockKey for block-addressed Badger KV storage 17 unit tests and benchmarks for the block storage format. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phases 2-4 of BM25 scaling plan: Phase 2 - Segmented mutation path: - addBM25IndexMutations now writes to block-based storage - Each term's postings split into ~128-entry blocks with a directory - Blocks automatically split when exceeding 256 entries - Doc-length list also uses block-based storage - Block removal and directory cleanup on deletes Phase 3 - WAND top-k query path: - New bm25wand.go with listIter for block-based posting list iteration - WAND algorithm with min-heap for top-k early termination - Per-block upper bounds (UBPre) computed from maxTF at query time - Suffix-max UBPre for efficient threshold checking - Falls back to scoring all docs when no first: limit or offset is used Phase 4 - Block-Max WAND: - skipToWithBMW skips entire blocks whose UB + other terms can't beat theta - Avoids Badger reads for blocks that can't contribute to top-k - Enabled by default in handleBM25Search Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 5 - Migration support: - newListIter falls back to legacy monolithic blob when no block directory exists - lookupDocLen falls back to legacy BM25DocLenKey blob - wandSearch falls back to legacy BM25IndexKey for df computation - Legacy data transparently served through synthetic single-block directory - New writes always use block format; old data works until overwritten Unit tests for WAND components: - TestTopKHeapBasic: heap operations, threshold, eviction - TestTopKHeapTieBreaking: deterministic ordering on score ties - TestBm25ScoreFunction: formula verification, tf/dl/b edge cases - TestBm25ScoreNaN: no NaN/Inf for edge-case inputs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes critical bugs and performance issues identified by GPT-5 review: - Fix negative inBlockPos panic: guard currentDoc/currentTF/skipTo against inBlockPos < 0 (possible before first next() call) - Fix empty block pathological behavior: next()/skipTo()/skipToWithBMW() now skip empty blocks instead of leaving iterator in invalid state with MaxUint64 pivotDoc - Fix legacy loadBlock: no longer resets inBlockPos to 0 (was moving pointer backwards, could cause re-scoring or infinite loops) - Fix remainingUB panic: guard against blockIdx < 0 (before first next()) - Add docLenCache: caches doclen directory + block reads within a single query, avoiding repeated Badger reads per scored document - Optimize BMW otherUB: compute as sumUB - thisUB (O(1)) instead of iterating all other terms (O(q^2) -> O(q)) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…UB underestimate Three fixes: 1. CRITICAL: addBM25IndexMutations now checks if a UID already exists in doclen blocks before incrementing stats, preventing double-counting on SET when the document was already indexed (defensive guard for batch mutations). 2. HIGH: WAND sumUB now accumulates across ALL iterators (not just up to pivot), so BMW's otherUB calculation is correct and won't skip valid candidate blocks. 3. PERF: newListIter accepts pre-read Dir to eliminate duplicate Badger reads (directory was read once for df, then again inside newListIter). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ength Defensive hardening from GPT-5 review: if inBlockPos exceeds block length after next() reaches end of block, the sort.Search span could go negative. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add DecodeCount() to bm25enc for O(1) entry count reads without full decode, preventing OOM on legacy migration with large posting lists (e.g., common terms with millions of entries) - Use DecodeCount in WAND search legacy DF calculation path - Fix integer overflow in DecodeDir bounds check by using uint64 arithmetic (prevents panic on corrupted data with MaxUint32 count) - Pre-allocate shared score buffer in handleBM25Search with three-index slices to prevent accidental append corruption - Document bm25Writes concurrency model and limitations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces the parallel block-storage + retrieval stack (declined in review) with an implementation that rides Dgraph's standard posting-list machinery, addressing the maintainer's feedback (independently endorsed by GPT-5 and Gemini). Net ~1300 fewer lines. Storage / indexing: - BM25 term postings are standard index posting lists at IndexKey(attr, IdentBM25||term), written via the normal delta path, so they inherit MVCC, deltas, rollup, splits, backup and snapshot. Each posting is a REF posting whose value packs (term-frequency, doc-length) as two uvarints. - Fix the linchpin: List.encode() now retains REF postings that carry a value through rollup (otherwise the term frequency was silently stripped). Mirrors how faceted postings already coexist in Pack (uid) + Postings (payload). - Document length is packed into the posting value rather than a separate list, avoiding a write-hot doclen key and a per-candidate random read at query time. - Corpus stats (docCount, totalTerms) are sharded across 32 buckets keyed by uid%32 so concurrent writers rarely contend, while same-bucket updates still conflict-and-retry (mirrors the @count pattern). Term postings get the standard index conflict key (fingerprint(key)^uid), so two docs sharing a term commit concurrently without conflict -- resolving the concurrency regression that the block version only mitigated via Raft serialization. - Delete posting/bm25block and posting/bm25enc; remove LocalCache.bm25Writes, BitBM25Data, and the BM25 commit branch in mvcc.go. Query / scoring: - WAND / Block-Max WAND reworked over the standard posting-list iterator: per-term cursors are materialized from the in-memory List with per-128-posting maxTF/minDocLen bounds -- no parallel block format, no proto changes. - Surface the score via Dgraph's existing value-variable mechanism: the bm25 root function binds its per-doc score to its own variable (Uids + Vals), so `scores as var(func: bm25(...))` works with uid(scores), val(scores) and orderdesc: val(scores). Removes the bm25_score pseudo-predicate and the __bm25_scores__ ParentVars channel. - Skip the query-layer pagination pass for bm25 roots (the worker already paginates over score order), mirroring the existing `has` handling, to avoid double-applying first/offset. Tests: - Rollup TF/doc-length survival, bucketed-stats accumulation (incl. same-bucket in-transaction read-your-own-writes and deletes), value-codec round trip, and a 200-trial randomized WAND/Block-Max-WAND vs brute-force correctness check. - Convert the query integration tests to the value-variable syntax. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
updateBM25Stats maintains the bucketed (docCount, totalTerms) counters via read-modify-write, but read them with LocalCache.GetFromDelta, which skips disk and returns only the current transaction's in-memory delta. Across separately committed mutations each transaction therefore started from zero and overwrote its stats bucket instead of accumulating, collapsing the corpus document count (e.g. to the per-term df). Since avgDL = totalTerms/docCount and idf depends on N = docCount, every length- and idf-weighted BM25 score was wrong, while result ordering (a constant idf factor for a single-term query) still looked correct. Read the committed stats with LocalCache.Get instead. Term postings are unaffected: they are additive deltas that merge through the normal posting-list mechanism and never read-modify-write. Found by the BM25 integration tests (exact-score and uid-filter cases). The prior unit test only exercised a single transaction, where read-your-own-writes masked the bug; add TestBM25StatsAccumulateAcrossTxns covering the multi- transaction path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BM25 scores a single document (one value) per UID, so per-document length and corpus statistics are ill-defined for a list predicate. The bucketed stats also rely on conflict detection that a list predicate's value-dependent conflict key would not provide (a code-review concern about stats integrity on list/ @noconflict predicates). Reject the combination in checkSchema rather than silently mis-scoring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The redesign rationale (including the doc-length storage decision) lives in code comments and the PR description; the planning doc does not belong in the tree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add native multi-signal search fusion so a single DQL query can combine BM25 text relevance with vector similarity (and any other scored value variable), instead of issuing separate queries and fusing in application code. New surface: - fuse(v1, v2, ..., method:"rrf"|"linear", k:60, weights:"0.3,0.7", normalize:"max"|"none", topk:N): an N-way combinator over already-scored DQL value variables. RRF = sum 1/(k+rank); linear = weighted sum of (optionally max-normalized) scores. Outer-join/union semantics: a uid missing from a channel contributes nothing (RRF) or 0 (linear), never dropped. Computed coordinator-side over resolved variables; the existing dependency scheduler orders it after its channel blocks. - hybrid(textPred, "query", vecPred, $vec, topk:N, method:..., k:...): convenience sugar rewritten at parse time into bm25 + similar_to channel blocks plus a fuse() block (no distinct execution path). Vector scores surfaced: - similar_to now binds a higher-is-better similarity score (cosine/dot as-is; euclidean as 1/(1+d^2)) to a value variable, so vector results can be a fusion channel alongside BM25. New SearchScored / SearchWithUidScored mirror Search / SearchWithUid exactly, so scoring a plain vector query does not change which neighbors it returns; the *Options* scored variants apply only when ef/distance-threshold is supplied. Robustness (post adversarial review by GPT-5 + Gemini): - non-finite scores are dropped before fusion so they cannot break the sort comparator or poison linear sums; - fused value variable follows the bm25 value-variable contract (ascending uid set + uid->score map; ranked order via orderdesc: val(var); topk selected before the ascending sort); - undefined fuse channel vars are rejected at parse time; - the __hybrid prefix is reserved to avoid synthetic/user var collisions. Tests: fusion-core unit tests (RRF/linear/union/ties/NaN/topk/determinism), parser tests (fuse + hybrid + error paths), score-orientation tests, and end-to-end integration tests combining BM25 + vector (fuse and hybrid). Design and review notes in docs/superpowers/specs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Auto-fixed issues flagged by a majority (>=3/5) of /judge runs across Claude, GPT5, and Gemini. Minority findings documented in the deep-review report. - hybrid(): bound the generated bm25 channel to topk (first:) so a broad text query no longer scores the entire corpus before fusion (5/5). - fuse(): apply per-channel weights under RRF too, not only linear, so a user passing weights with the default method no longer has them silently ignored; default weight 1.0 keeps standard RRF (4/5). - hybrid(): check the reserved __hybrid var prefix across nested blocks, not just top-level (4/5); reject malformed (odd) option lists instead of dropping them (3/5). - fuse(): distinguish a genuinely missing channel variable (internal invariant error) from a channel that ran but matched nothing (valid empty channel) (3/5). Added unit tests for weighted RRF and the hybrid bm25 bound; updated the hybrid/fuse equivalence test to mirror the bounded bm25 channel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two issues found in deep review of native hybrid search: 1. fuse() rejected at subgraph conversion. The DQL parser accepts `fuse` (validFuncName), but query.go's isValidFuncName did not list it, so every fuse()/hybrid() query failed with "Invalid function name: fuse". The integration tests that exercise this are CI-gated and had not been run. Added "fuse"; all 25 fuse/hybrid/similar_to-score integration tests now pass. (hybrid is rewritten to fuse before this check, so only fuse needs registering.) 2. Score/UID misalignment under @filter. The generalized bm25/similar_to ranker binding zipped uidMatrix[0] with valueMatrix positionally; a later @filter on the ranker block runs updateUidMatrix (and pagination), which shrinks/reorders uidMatrix[0] in place without touching valueMatrix — misbinding scores to UIDs and feeding wrong scores into fuse() channels. Snapshot the aligned worker result into a uid->score map (sg.rankerScores) at result time, before any mutation, and bind by UID. Identical behavior on the tested no-filter paths.
- posting/bm25.go: tagged switch on err (staticcheck QF1002) - design spec: prettier proseWrap + table format, MD029 list prefix, MD040 code-fence languages Fixes the Trunk Code Quality failure on this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Consensus finding from deep review (3/5 judge runs): valToTaskValue was added but never referenced anywhere in the tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add BM25 relevance-ranked text search to Dgraph, enabling users to query text predicates and receive results ordered by relevance score instead of boolean matching. Implementation: - New BM25 tokenizer using the fulltext pipeline (normalize, stopwords, stem) that preserves term frequencies for TF counting - BM25-specific index storage: per-term TF posting lists, doc length lists, and corpus statistics (doc count, total terms) - Query execution with full BM25 scoring: score = IDF * (k+1) * tf / (k * (1 - b + b * dl/avgDL) + tf) IDF = log1p((N - df + 0.5) / (df + 0.5)) - DQL syntax: bm25(predicate, "query" [, "k", "b"]) as root func or filter - Schema syntax: @index(bm25) - Parameter validation (k > 0, 0 <= b <= 1) - Early UID intersection for filter-mode performance - All-stopword document and query handling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three critical bugs fixed: 1. REF postings lose Value during rollup: The posting list encode/rollup cycle strips the Value field from REF postings without facets (list.go:1630). BM25 term frequencies and doc lengths were stored in Value and lost. Fix: Store TF and doclen as facets on REF postings, which are preserved. 2. Missing function validation: query/query.go has a separate isValidFuncName check from dql/parser.go. "bm25" was only added to the parser, causing "Invalid function name: bm25" at query time. 3. Unsorted UIDs break query pipeline: BM25 returned UIDs sorted by score, but the query pipeline (algo.MergeSorted, child predicate fetching) requires UID-ascending order. Fix: Sort UIDs ascending in UidMatrix, apply first/offset pagination on score-sorted results before UID sorting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the facet-based BM25 storage (~40-50 bytes/posting) with compact varint-encoded binary blobs stored as direct Badger KV entries (~4-6 bytes/posting, ~10x reduction). Add bm25_score pseudo-predicate for variable-based score ordering following the similar_to pattern. - Add posting/bm25enc package for compact binary encode/decode - Rewrite write path in posting/index.go for direct Badger KV - Add bm25Writes buffer to LocalCache with read-your-own-writes - Flush BM25 blobs in CommitToDisk with BitBM25Data UserMeta - Rewrite read path in worker/task.go with direct blob decoding - Add bm25_score pseudo-predicate in query/query.go - Add score ordering integration tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cases Cover incremental add/update/delete, IDF score stability as corpus grows, large corpus pagination, unicode, stopwords, uid filtering, score validation, and concurrent batch adds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…c tests Addresses test coverage gaps identified during code review against ArangoDB's BM25 implementation: - TestBM25ExactScoreValues: validates numerical correctness of BM25 formula using b=0 to enable hand-computed expected scores - TestBM25BM15NoLengthNormalization: verifies b=0 disables length normalization and contrasts with default b=0.75 behavior - TestBM25SingleMatchingDocument: covers df=1 edge case with high IDF Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 1 of BM25 scaling plan. Introduces bm25block package with: - BlockMeta/Dir types for block directory encoding/decoding - SplitIntoBlocks: splits monolithic entry slices into 128-entry blocks - MergeAllBlocks: compacts overlapping blocks with dedup and tombstone removal - ComputeUBPre/SuffixMaxUBPre: WAND upper-bound precomputation - New key functions: BM25TermDirKey, BM25TermBlockKey, BM25DocLenDirKey, BM25DocLenBlockKey for block-addressed Badger KV storage 17 unit tests and benchmarks for the block storage format. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phases 2-4 of BM25 scaling plan: Phase 2 - Segmented mutation path: - addBM25IndexMutations now writes to block-based storage - Each term's postings split into ~128-entry blocks with a directory - Blocks automatically split when exceeding 256 entries - Doc-length list also uses block-based storage - Block removal and directory cleanup on deletes Phase 3 - WAND top-k query path: - New bm25wand.go with listIter for block-based posting list iteration - WAND algorithm with min-heap for top-k early termination - Per-block upper bounds (UBPre) computed from maxTF at query time - Suffix-max UBPre for efficient threshold checking - Falls back to scoring all docs when no first: limit or offset is used Phase 4 - Block-Max WAND: - skipToWithBMW skips entire blocks whose UB + other terms can't beat theta - Avoids Badger reads for blocks that can't contribute to top-k - Enabled by default in handleBM25Search Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 5 - Migration support: - newListIter falls back to legacy monolithic blob when no block directory exists - lookupDocLen falls back to legacy BM25DocLenKey blob - wandSearch falls back to legacy BM25IndexKey for df computation - Legacy data transparently served through synthetic single-block directory - New writes always use block format; old data works until overwritten Unit tests for WAND components: - TestTopKHeapBasic: heap operations, threshold, eviction - TestTopKHeapTieBreaking: deterministic ordering on score ties - TestBm25ScoreFunction: formula verification, tf/dl/b edge cases - TestBm25ScoreNaN: no NaN/Inf for edge-case inputs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes critical bugs and performance issues identified by GPT-5 review: - Fix negative inBlockPos panic: guard currentDoc/currentTF/skipTo against inBlockPos < 0 (possible before first next() call) - Fix empty block pathological behavior: next()/skipTo()/skipToWithBMW() now skip empty blocks instead of leaving iterator in invalid state with MaxUint64 pivotDoc - Fix legacy loadBlock: no longer resets inBlockPos to 0 (was moving pointer backwards, could cause re-scoring or infinite loops) - Fix remainingUB panic: guard against blockIdx < 0 (before first next()) - Add docLenCache: caches doclen directory + block reads within a single query, avoiding repeated Badger reads per scored document - Optimize BMW otherUB: compute as sumUB - thisUB (O(1)) instead of iterating all other terms (O(q^2) -> O(q)) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…UB underestimate Three fixes: 1. CRITICAL: addBM25IndexMutations now checks if a UID already exists in doclen blocks before incrementing stats, preventing double-counting on SET when the document was already indexed (defensive guard for batch mutations). 2. HIGH: WAND sumUB now accumulates across ALL iterators (not just up to pivot), so BMW's otherUB calculation is correct and won't skip valid candidate blocks. 3. PERF: newListIter accepts pre-read Dir to eliminate duplicate Badger reads (directory was read once for df, then again inside newListIter). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ength Defensive hardening from GPT-5 review: if inBlockPos exceeds block length after next() reaches end of block, the sort.Search span could go negative. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add DecodeCount() to bm25enc for O(1) entry count reads without full decode, preventing OOM on legacy migration with large posting lists (e.g., common terms with millions of entries) - Use DecodeCount in WAND search legacy DF calculation path - Fix integer overflow in DecodeDir bounds check by using uint64 arithmetic (prevents panic on corrupted data with MaxUint32 count) - Pre-allocate shared score buffer in handleBM25Search with three-index slices to prevent accidental append corruption - Document bm25Writes concurrency model and limitations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces the parallel block-storage + retrieval stack (declined in review) with an implementation that rides Dgraph's standard posting-list machinery, addressing the maintainer's feedback (independently endorsed by GPT-5 and Gemini). Net ~1300 fewer lines. Storage / indexing: - BM25 term postings are standard index posting lists at IndexKey(attr, IdentBM25||term), written via the normal delta path, so they inherit MVCC, deltas, rollup, splits, backup and snapshot. Each posting is a REF posting whose value packs (term-frequency, doc-length) as two uvarints. - Fix the linchpin: List.encode() now retains REF postings that carry a value through rollup (otherwise the term frequency was silently stripped). Mirrors how faceted postings already coexist in Pack (uid) + Postings (payload). - Document length is packed into the posting value rather than a separate list, avoiding a write-hot doclen key and a per-candidate random read at query time. - Corpus stats (docCount, totalTerms) are sharded across 32 buckets keyed by uid%32 so concurrent writers rarely contend, while same-bucket updates still conflict-and-retry (mirrors the @count pattern). Term postings get the standard index conflict key (fingerprint(key)^uid), so two docs sharing a term commit concurrently without conflict -- resolving the concurrency regression that the block version only mitigated via Raft serialization. - Delete posting/bm25block and posting/bm25enc; remove LocalCache.bm25Writes, BitBM25Data, and the BM25 commit branch in mvcc.go. Query / scoring: - WAND / Block-Max WAND reworked over the standard posting-list iterator: per-term cursors are materialized from the in-memory List with per-128-posting maxTF/minDocLen bounds -- no parallel block format, no proto changes. - Surface the score via Dgraph's existing value-variable mechanism: the bm25 root function binds its per-doc score to its own variable (Uids + Vals), so `scores as var(func: bm25(...))` works with uid(scores), val(scores) and orderdesc: val(scores). Removes the bm25_score pseudo-predicate and the __bm25_scores__ ParentVars channel. - Skip the query-layer pagination pass for bm25 roots (the worker already paginates over score order), mirroring the existing `has` handling, to avoid double-applying first/offset. Tests: - Rollup TF/doc-length survival, bucketed-stats accumulation (incl. same-bucket in-transaction read-your-own-writes and deletes), value-codec round trip, and a 200-trial randomized WAND/Block-Max-WAND vs brute-force correctness check. - Convert the query integration tests to the value-variable syntax. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
updateBM25Stats maintains the bucketed (docCount, totalTerms) counters via read-modify-write, but read them with LocalCache.GetFromDelta, which skips disk and returns only the current transaction's in-memory delta. Across separately committed mutations each transaction therefore started from zero and overwrote its stats bucket instead of accumulating, collapsing the corpus document count (e.g. to the per-term df). Since avgDL = totalTerms/docCount and idf depends on N = docCount, every length- and idf-weighted BM25 score was wrong, while result ordering (a constant idf factor for a single-term query) still looked correct. Read the committed stats with LocalCache.Get instead. Term postings are unaffected: they are additive deltas that merge through the normal posting-list mechanism and never read-modify-write. Found by the BM25 integration tests (exact-score and uid-filter cases). The prior unit test only exercised a single transaction, where read-your-own-writes masked the bug; add TestBM25StatsAccumulateAcrossTxns covering the multi- transaction path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BM25 scores a single document (one value) per UID, so per-document length and corpus statistics are ill-defined for a list predicate. The bucketed stats also rely on conflict detection that a list predicate's value-dependent conflict key would not provide (a code-review concern about stats integrity on list/ @noconflict predicates). Reject the combination in checkSchema rather than silently mis-scoring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The redesign rationale (including the doc-length storage decision) lives in code comments and the PR description; the planning doc does not belong in the tree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BM25 term postings live under the IdentBM25 token prefix, but the corpus
statistics buckets live under a separate reserved token prefix
("\x00_bm25stats_"). prefixesToDeleteTokensFor only emitted the
tokenizer-identifier prefix, so dropping or rebuilding a BM25 index left the
stats keys orphaned. On rebuild the fresh stats then accumulated on top of the
stale ones, double-counting docCount/totalTerms and corrupting avgDL/IDF.
Add x.BM25StatsPrefix and include it (plus its ByteSplit variant) in the bm25
deletion prefixes so the stats are removed together with the term postings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The streaming index rebuild processes a predicate's documents across ~16 goroutines, each with its own transaction/cache that is periodically reset (NewTxn) every ~10k posting lists, emitting deltas to a temp store. BM25 corpus statistics were maintained by a per-transaction read-modify-write of a single value posting per bucket, which collapses under that model: every thread (and every post-reset segment) reads an empty base and writes its own partial total, and the value postings merge last-write-wins — so the rebuilt docCount/totalTerms were severely undercounted, distorting avgDL and IDF. Even a first-time @index(bm25) over existing data produced wrong scores. Route stats through a shared, concurrency-safe accumulator (Txn.bm25Acc, set on the rebuild's per-thread txns) instead of the RMW counter, then flush all 32 buckets once as a single writer. The flush is written into the temp store as ordinary delta postings so the rebuild's second phase rolls it up into pstore at startTs alongside the term postings — no separate commit path. The live mutation path is unchanged and was already correct: on a scalar predicate fingerprintEdge returns the MaxUint64 sentinel for every untagged value, so concurrent same-bucket writers share a conflict key and one retries (this is why @index(bm25) is rejected on list predicates). Added a regression test for that invariant, a unit test for the rebuild accumulator, and integration tests for rebuild-on-existing-data, drop-then-re-add (no stale-stats double counting), and concurrent overlapping mutations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
handleBM25Search only engaged WAND top-k early termination when first was set WITHOUT an offset. Any query with an offset (or a bm25 filter) fell through to scoreAllDocs, which materializes every matching document into a map — and wandSearch already reads each query term's entire posting list into memory. A deep-paginated query over a hot term could therefore allocate and score the whole corpus to return a handful of rows. Engage WAND whenever a first limit is present, retaining first+offset documents and dropping the offset afterward (bm25TopK / bm25PaginateScored, extracted as pure, unit-tested helpers). With no first limit, every match is still scored — the caller explicitly asked for all results. wandSearch always returns score-descending results, so a single pagination path is correct for both the top-k and score-all cases. Also adds TestWandFilteredMatchesBruteForce: the existing WAND brute-force comparison only exercised a nil filter, leaving @filter(bm25(...)) pruning unverified. The new test fuzzes random filter subsets across WAND, Block-Max WAND, and the score-all path against exhaustive filtered scoring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The bulk loader ran BM25 through the generic tokenizer path, which produced a silently broken, unsearchable index: term postings were written as bare REF postings with no value (addMapEntry drops the payload of any REF posting without facets), so the packed (term frequency, document length) was lost and ReadBM25TermPostings skipped every posting; and no corpus statistics were written at all, so docCount was 0 and every bm25() query short-circuited to empty. - addMapEntry now retains REF postings that carry a Value (mirroring the len(p.Value) > 0 clause in List.encode), so BM25 term postings keep their tf/dl. - addIndexMapEntries special-cases the BM25 tokenizer: one posting per distinct term with the packed value, and per-document corpus-stat partials accumulated per mapper. - After the map phase, loader.flushBM25Stats merges every mapper's partials and writes one value posting per (predicate, bucket) into the predicate's map shard, so the reduce emits a single stats posting per bucket in the same format the live and rebuild paths use. Stats are summed across mappers (not unioned like postings), which is the correctness crux, covered by TestMergeBM25Stats. Adds an end-to-end bulk -> bm25() query integration test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two issues found in deep review of the BM25 query integration: 1. Score/UID misalignment under @filter. populateUidValVar bound the BM25 score variable by positionally zipping uidMatrix[0] with valueMatrix. That alignment only holds as the worker returns it: a later @filter on the bm25 block runs updateUidMatrix (and pagination), which shrinks/ reorders uidMatrix[0] in place without touching valueMatrix — so e.g. `s as var(func: bm25(p, "q")) @filter(...)` would bind scores to the wrong UIDs. Snapshot the (aligned) worker result into a uid->score map the moment it arrives, before any filter/pagination mutates the matrices, and bind from that map keyed by UID. Behavior is identical on the already-tested no-filter paths. 2. Removed valToTaskValue, which was unused dead code (fails the `unused` linter in the default golangci-lint set). All 52 BM25 unit + integration tests pass.
Consensus fix from deep review (3/5 judge runs; empirically verified). The top-k
min-heap ordered by score only, so on score ties eviction removed the arbitrary
min-score root — which among ties is the lowest UID (inserted first, never swapped).
A first:k WAND query could thus drop a lower-UID doc and keep a higher-UID one,
diverging from the documented (score desc, UID asc) order and the no-limit
scoreAllDocs path (e.g. k=2, uids 3,4 tied then uid5 higher -> returned {5,4}
instead of {5,3}).
Less() now breaks score ties by UID descending so the highest UID sits at the root
and is evicted first. Primary ordering stays score-ascending, so threshold() still
reports the true minimum and WAND pivot pruning is unchanged (the randomized
brute-force suite still passes). Adds a regression test for the eviction boundary
that the brute-force tests deliberately skip.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…test Follow-up to the deep-review test-gap findings on this PR. 1. @noconflict stat drift (R2/R5 finding): BM25 corpus-stats read-modify-write relies on transaction conflict detection to serialize same-bucket updates. A @noconflict predicate emits no conflict key, so concurrent updates would silently lose counts and corrupt avgDL/IDF. resolveTokenizers now rejects the bm25 + @noconflict combination at schema-parse time (covered by TestSchemaBM25NoConflict_Error, with a positive control). 2. Tie determinism: TestWandMatchesBruteForce deliberately skips UID assertions on score ties. Add TestWandTieBreakEndToEndMatchesFullScan exercising the full wandTopK/scoreAllDocs retrieval path on an eviction-triggering tie, asserting first:k returns the same docs as the exhaustive path (WAND and Block-Max WAND). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shaunpatterson
force-pushed
the
sp/hybrid-search
branch
from
July 14, 2026 01:38
edf4791 to
b0877b3
Compare
# Conflicts: # dql/parser.go # posting/bm25.go # posting/bm25_test.go # query/common_test.go # query/query.go # query/query_bm25_test.go # worker/bm25wand.go # worker/bm25wand_test.go # worker/task.go # x/keys.go
…iers Triage of opencode adversarial review findings on the hybrid() rewrite: - topk<=0 reached HNSW as expectedNeighbors<=0 and panicked (neighbors[:0] then neighbors[0]); reject non-positive topk up front. - hybrid(t, $q, v, $vec) sent the literal "$q" to bm25 because the text arg was rebuilt from .Value, dropping IsDQLVar; pass the whole Arg through. - @filter/order/cascade/children on a hybrid() block were silently discarded by the rewrite; reject them instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
opencode review dgraph-io#6: under the union's missing-uid=0 convention, max-abs normalization left signed metrics (cosine/dot) able to retrieve a document with negative similarity that then fused BELOW a document the channel never retrieved (0). Clamp normalized scores to [0,1] so "retrieved but dissimilar" ties the missing baseline instead of ranking beneath it. BM25 and euclidean (1/(1+d)) are already >=0 and unaffected; existing tests unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deep-dive test attack on the hybrid-search branch (89 blind attack questions -> 35 grounded verdicts -> tests + fixes; see HYBRID_SEARCH_TESTING_BRAINSTORM). Tests added (~30 integration + unit): - bm25: filter+first score-page, tie-storm page partition/determinism, deep offset, stats churn pins, emoji zero-token symmetry, @lang rejection pin - hybrid/fuse: score-binding oracle under filter+pagination+traversal, metric math pins, NaN/Inf query vectors, channel-validity battery, topk semantics pin, union-order drop pin, directive matrix, fuse-of-fuse RRF oracle, degenerate similar_to inputs, parser modifier/duplicate rejections, fusion-math pins (window normalization, clamp/weight order, RRF weights) - worker: tie-heavy WAND-vs-exhaustive differential (dyadic-exact), fresh- bounds differential Fixes (each gated by a test above): - bm25/similar_to root + @filter + first:N returned the N lowest-uid matches instead of the top-scored page (triple-confirmed; empirical probe) — applyPagination now pages ranker roots by (score desc, uid asc) - fuse()/hybrid() blocks silently ignored @filter/order/pagination/cascade/ children; fuse(a,a) duplicate channels — rejected at parse time - NaN/Inf reaching rankerScores or HNSW: dropped at snapshot; query vectors with non-finite components rejected; similar_to k<=0 no longer panics the alpha (guarded before HNSW) - uid-var fuse channel silently became an empty channel (ShardedMap.IsEmpty only detects nil — 30 empty shards are 'non-empty'); now rejected with a clear error - @lang + @index(bm25) schema combo rejected: lang-tagged values are stored under lang-qualified keys bm25() cannot query, making them silently unsearchable (empirically verified; fulltext-parity behavior) - groupby aggregation over coordinator-synthesized value vars: guarded valueMatrix indexing (defense; parser already rejects the reachable shape) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ats race New systest/bm25lifecycle package (dgraphtest.LocalCluster, integration2 tag): - TestHybridMultiGroupParity: text/vector predicates on different groups vs 1-group control. bm25 leg demands bit-parity (deterministic); similar_to legs assert row counts, ordering, >=n-1 uid overlap and bit-equal scores for shared uids (HNSW graphs built independently legitimately differ on boundary-rank candidates); fused legs additionally drop cross-topology score equality (fused scores inherit channel rank noise; per-uid fusion correctness is pinned by the single-cluster oracle test). - TestBM25ReplicaConvergenceUnderFailure: replica catch-up after follower outage, heavy-single-txn stats oracle, leader-failover score stability. - TestBM25BulkLiveParity: bulk vs live loaded clusters must agree with a closed-form oracle to 1e-9 (duplicate-triples variant gated, see below). - TestHNSWLifecycleParity: upsert/delete/reinsert/restart — no ghost uids, no stale-vector scores (Q22 -> SAFE empirically). - TestBM25CrashRecoveryMidRebuild: SIGKILL mid-rebuild recovers to exact scores via Raft replay (Q20 hazard refuted, recovery pinned). Bug found and FIXED (gated by HeavySingleTxnStatsOracle): - Intra-proposal parallel stats race: large proposals (>=512 edges) apply across goroutines batched to never share an (attr, entity) DATA key — but stats buckets are keyed uid%32, so different entities race the same bucket RMW and drop deltas. Txn.bm25StatsMu now serializes the stats RMW within a transaction; cross-txn safety unchanged (conflict keys). Bugs confirmed, gated behind BM25_KNOWN_FAILING=1 as acceptance tests: - Q16 rebuild-vs-live stats undercount (live write between DropPrefix and the rebuild's absolute flush RMWs from an empty bucket and shadows the flush via MVCC; uid 0x5b scored 0.0062 vs closed-form 4.137). Fix needs a Zero-mediated conflicting flush or delta-merged stats. - Q23 bulk-loader duplicate triples inflate stats (mapper counts per nquad, reduce dedupes postings; implied docCount=1050 for truth 1000). Fix needs once-per-(pred,uid) stats at reduce time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- worker/bm25wand_bench_test.go: WAND/BMW vs exhaustive matrix (corpus size x term selectivity x topK) with docs_scored/postings counters via a verified line-identical counting replica. Headlines: topK=10 multi-term ~64x wins; single high-df term ~11x; topK>=100 collapses to exhaustive; deep offset (first+offset heap) degrades ~43x; BMW==WAND on interleaved corpora. - query/fuse_bench_test.go: fusion core matrix (channels x size x overlap x method x topk). RRF 3.5-4x linear; channelRanks is ~70% of 8-channel RRF; topk does not bound work (truncate after full sort). - posting/bm25_bench_test.go: per-query stats floor ~33us/1100 allocs over 32 buckets uncached (~9x warm); codec decode 5.5ns/0 allocs. - Report updated with wave-2 and wave-3 outcomes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Q23 fix — bulk stats exact under duplicate nquads: The mapper counted corpus statistics once per nquad while the reducer deduped postings, so duplicated triples inflated docCount/totalTerms (empirically docCount=1050 for truth 1000). The mapper now emits one per-document posting (docCount=1, totalTerms=docLen) on the final stats bucket key; the reducer's same-uid dedup collapses duplicates exactly as it does for term postings, and a fold in appendToList sums the deduped contributions into the single aggregate bucket posting the live/rebuild paths store. Deletes the mapper-side pre-aggregation (bm25StatEntry, mergeBM25Stats, flushBM25Stats). Verified: the duplicate-triples parity test now passes and is ungated as the default. Fusion performance (gated by query/fuse_bench_test.go): - channelOrder copies (uid, score) pairs out of the map once so the sort comparator does slice reads instead of two map probes per comparison, and fuseRRF iterates the sorted order positionally instead of materializing a uid->rank map. 8ch/100k RRF: 581ms -> 276ms. - topk>0 now selects via a size-bounded worst-at-root heap (O(n log topk), no n-sized output allocation): 8ch/100k topk=10: -> 168ms (3.5x total). Q16 (rebuild-vs-live stats undercount) stays gated: the flush lands at r.startTs, below any concurrent live commit, so MVCC always favors the zero-based shadowing writer; every bounded workaround traced either blocks the Raft apply loop or risks heuristic corruption. Needs delta-merged stats or a Zero-serialized flush (design task; gated test is the acceptance gate). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nups OCR review findings on 594d447, triaged and applied: - [high] The bm25-on-list-predicate rejection lived only in worker.ValidateSchema, which the BULK LOADER never calls — a schema file with 'pred: [string] @index(bm25)' slipped through, and the new per-(attr, uid) stats dedup would silently collapse distinct list values' stats. Mirrored the rejection into schema resolveTokenizers (alongside the existing @lang/@noconflict checks) so every ingest path is covered; parse tests pin both rejections. - [medium] Deleted the now-dead channelRanks and retargeted its stale benchmark to channelOrder (the actual hot path: 12ms vs 51ms at 100k). - [medium] Added TestFuse_TopKHeapTieBreakEviction pinning the heap's tie-break replacement branch (score-only eviction would regress silently) plus a heap-vs-full-sort prefix equality check. - [low] topKFused fill phase appends + single heap.Init instead of per- element heap.Push interface boxing. Rejected findings (with reasons): reduce-metric skew on the defensive early-return (cosmetic; noted in code), x.Check->skip on corrupt postings (matches the file's existing convention; a panic on disk corruption in an offline bulk run is the correct UX), focused reducer-fold unit test (covered by the fold-model unit test and the ungated e2e duplicate-parity test). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Native hybrid search with score fusion (
fuse+hybrid)Brings hybrid retrieval (dense vector + sparse/keyword, fused into one ranked list)
natively into a single DQL query, instead of issuing separate queries and fusing in
application code.
Important
This branch is stacked on the BM25 ranked-text-search work, which is not yet
upstream. Of the commits here, 17 are the BM25 foundation and 2 are hybrid
search (
feat(search): native hybrid search…+ a fix). The hybrid feature reusesBM25's value-variable ranker contract and needs it as a fusion channel, so it
cannot be reviewed standalone. This PR should land after a BM25 PR (or be
split once BM25 is upstreamed). Opening it now for visibility/review of the
fusion layer.
Two pieces
fuse()— N-way fusion combinator over already-scored DQL value variables(the general primitive). Methods:
rrf(Reciprocal Rank Fusion, default,k=60)and
linear(weighted sum of optionally max-normalized scores). Supportsper-channel
weights,normalize, andtopk.hybrid()— convenience sugar for the common BM25 + vector case, rewritten atparse time into two channel blocks + a
fuse(). No distinct executor path.Design notes
channels, not an intersection: a uid missing from a channel contributes nothing
(RRF: rank = ∞; linear: score = 0), never dropped, never NaN.
fuse()is never dispatched to a worker; it's computed inpopulateVarMapover already-resolved value variables. The existing variabledependency scheduler runs channel blocks first.
similar_tonow also surfaces ahigher-is-better similarity score (cosine/dot as-is; euclidean as
1/(1+d)) sovector results can be a fusion channel.
Search*Scoredmirror the existingSearch*neighbor selection exactly — scoring a plain query does not change whichneighbors it returns; the scored path is only taken when a score is needed.
the sort comparator's strict-weak-ordering or poison a linear sum.
Tests
query/fuse_test.go,dql/fuse_parser_test.go): RRF ranks/union/ties/weights,linear normalize/weights/none, topk truncation, determinism, parser options,
hybrid expansion, error paths.
query): RRF/linear over BM25 channels, BM25+vector fusion,hybrid()equivalent to explicitfuse(), pagination, error handling, andsimilar_toscore-variable surfacing.25 fuse/hybrid/similar-score integration tests + the fusion/parser unit tests pass.
🤖 Generated with Claude Code