Fix/hybrid search id space - #22
Open
dartsoer-netizen wants to merge 7 commits into
Open
Conversation
Hybrid search has been silently degraded to pure vector search in every
non-trivial index. The two retrieval arms keyed their results on different
id spaces:
- vector arm: `row_offset + i`, a position within THIS query's result
batches, so always 0..n
- BM25 arm: `count_rows() + i`, a table row number assigned at index
time
Reciprocal Rank Fusion therefore combined two disjoint key sets. Every
consequence we observed follows from that one defect:
- each vector hit scored exactly 1/(60+rank), i.e. 0.016393, 0.016129,
... for every query regardless of match quality, which makes min_score
meaningless against hybrid scores
- keyword_score was always null, because BM25 wrote into original_scores
under an id no vector entry shared
- keyword-only hits were dropped: their row number was not a valid index
into the returned batches, so the materialisation scan never found them
- querying an exact symbol name returned unrelated chunks, since only the
semantic arm was ever contributing
The row-number scheme was also unstable on its own terms: incremental
re-indexing deletes and re-adds rows, so the numbering drifted from what
BM25 had stored.
Both arms now key on the chunk's stable `file_path:start_line`, which the
vector table already stored in its `id` column. Derivation moved into one
place, LanceVectorDB::chunk_id, since having it written out twice is how
the two drifted apart.
Also:
- BM25Search stores `chunk_id` as a STORED text field instead of a u64
`id`. Tantivy field handles are positional, so an index directory
written by an older build would be read with the wrong types; new()
now detects a pre-chunk_id schema and rebuilds the directory. It is a
derived artifact, so that costs only a re-index.
- keyword-only hits are fetched from the table by id after fusion. This
is what makes exact-symbol search actually work: such a chunk often
is not in the vector top-N at all, so it has to be materialised
separately rather than looked up in the vector batches.
- the BM25 read guard is scoped so it is released before the new await;
holding it across one makes the future non-Send, which the
VectorDatabase trait requires. An explicit drop() is not sufficient.
Why the existing tests passed: test_search_hybrid stores a single batch
into a fresh table, where count_rows() is 0 and the row numbers 0..n
coincide with the batch positions 0..n. The bug only appears from the
second insert onward, or whenever distance order differs from insertion
order. The new test_search_hybrid_across_multiple_batches reproduces both
conditions -- it inserts the keyword target first but gives it the vector
furthest from the query -- and fails on the parent commit with
"keyword_score must be populated for a BM25 match".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tracing_subscriber::fmt::init() defaults to stdout. In stdio MCP mode stdout carries the JSON-RPC stream, so every log line the server emitted was injected into the protocol and the client saw malformed frames. This is a plain bug for any stdio MCP server, independent of this project's configuration, which is why it is worth carrying upstream rather than keeping as a local patch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tools returned empty for every input. Distinct causes, one shared piece. Symbol resolution snapped to the enclosing node. Both find_references and get_call_graph located "the symbol at this position" by taking the FIRST definition whose line range contained the line. On a call site that is always the enclosing function, so asking about SendNotification2(...) inside ProcessDeviceNotifyCache resolved to ProcessDeviceNotifyCache and then answered a question nobody asked. Resolution now reads the identifier token actually under the cursor and prefers a definition of that name, falling back to the INNERMOST containing range rather than the first. find_definition uses the same helper, so it stops reporting an enclosing class for a member. References were only ever searched in the defining file. extract_references was called with the target's own FileInfo and nothing else, so any reference from another translation unit was invisible -- which is the normal case for anything public. Both tools now scan the defining file plus the files the index says mention the symbol. The shortlist comes from keyword search: a reference must contain the literal identifier, so BM25 names exactly the right files and tree-sitter runs only on those instead of the whole corpus. This depends on the hybrid-search fix in the previous commit; it would have been useless while the keyword arm was dead. get_call_graph additionally attributed callers to definitions from the wrong file, since it matched call sites found anywhere against the defining file's definition list. Callers are now attributed to the innermost enclosing function of the file the call was actually found in, and deduplicated per (file, function). search_git_history returned nothing because search_filtered post-filters. It searched the whole table for limit*3 candidates and then kept only rows with language "git-commit". A few hundred commits share that table with tens of thousands of code chunks, so no commit ever reached the candidate set and the filter emptied the list every time. The candidate pool is now widened when a filter is actually present. Predicate pushdown into the LanceDB query is the real fix and is noted as such in the code. Also fixes a hazard introduced by the previous commit: every commit in a repository is stored with file_path "git://<repo>" and start_line 0, so a fusion key of path+line collapsed an entire history onto one id and the BM25 index would have held a single document for it. chunk_id now includes file_hash, which is the commit hash for commits and is constant within a file for code, where start_line still separates chunks. Tests: 25 in vector_db pass, including a new test_commit_chunks_are_not_collapsed_by_shared_path. Full library suite is 402 passed / 13 failed, and those 13 fail identically on the parent commit -- they are pre-existing Windows issues in fs_lock, path normalisation and concurrent indexing, untouched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
Accepted |
Callees were never unimplemented -- get_call_graph builds them in full. The
name was parsed out of Reference::target_symbol_id with the wrong layout: that
field holds a Definition id, `def:<file>:<name>:<line>`, not a SymbolId id,
`<file>:<name>:<line>:<col>`. The old forward split(':') took parts[1], which
is the file path, so every lookup missed and the list came back empty.
Adds Definition::name_from_storage_id, which strips the `def:` prefix and
splits from the right so a Windows drive-letter colon cannot shift the fields.
Cross-TU callees needed a second fix: extract_references only emits a reference
whose identifier is a key in the index it is handed, and that index was built
from the target file alone, so a callee defined in another translation unit was
invisible at extraction time rather than filtered later. The index is now
widened first from call-site identifiers in the target span via
files_mentioning, bounded at 40 names x 5 files, with control-flow keywords
filtered so `if (` is not reported as a call. The emitted node now carries the
definition's own file rather than the requested one.
list_symbols(file_path) is new: every definition in a file as name / kind /
line span / signature, with no chunk content. It is the enumeration primitive
the server lacked -- query_codebase and search_by_filters are relevance-ranked
with a limit, and find_definition/find_references need a position you already
have. Unit1.cpp returns 297 definitions in ~115 ms.
Verified on PPSKiosk Unit1.cpp:16359 (WndProc): returns its five documented
targets plus five cross-file ones in DeviceManager.cpp/XFSMonitoring.cpp.
Empty callers there is correct -- nothing names a VCL override. Full suite
405 passed / 13 failed, the same 13 failing on a stashed baseline.
Known limits: list_symbols does not enumerate header members (Unit1.h yields 4
symbols), and the C++ parser desynchronises on some Borland constructs, losing
definitions across a range.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two independent defects, both surfaced by using the server against a large non-ASCII C++ tree. get_statistics reported total_files == total_points == total_vectors, because the stored row count was substituted for all three. DatabaseStats now carries a distinct file count and a LanguageBreakdown of (language, file_count, chunk_count), computed in the Lance backend from the stored file_path column, and the MCP layer maps it through to LanguageStats. The Qdrant backend does not scan payloads, so it leaves total_files and database_size_bytes at 0 rather than repeating the substitution that made the old numbers misleading. Checked against a 1341-file tree: 1341 files / 15525 chunks, with the per-language rows summing exactly to both totals. Byte caps applied to commit messages and diff text could land inside a multi-byte character, where &s[..n] and String::truncate panic. The three sites that cap by byte length -- the git chunker, the git walker, and the client-side git indexer -- now floor the offset through git::floor_char_boundary. Trees with Cyrillic commit messages hit that cut sooner or later. Tests cover the boundary helper directly and the statistics path in the Lance backend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Definition nodes whose name could not be extracted were dropped with no diagnostic, so a short or empty symbol list was indistinguishable from a complete one. Track them as SkippedDefinition (line, node kind, reason, snippet), surface them in ListSymbolsResponse.skipped via a new extract_definitions_reporting trait method, and log a warning per file. Also fix two early returns in the C/C++ name lookup (find_name_node and find_innermost_identifier) that returned None before trying the generic identifier fallback, which was one source of silently vanishing definitions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`cargo fmt` hard-errored ("failed to resolve mod `stack_graphs`") because
relations/mod.rs declares `pub mod stack_graphs;` behind the `stack-graphs`
feature, but no source file for it existed -- rustfmt tries to resolve every
module regardless of cfg. Added src/relations/stack_graphs/mod.rs as an
honest placeholder: StackGraphsProvider::new() always errors, which
HybridRelationsProvider already handles by falling back to RepoMapProvider,
so enabling the feature changes nothing observable yet.
Fixed the real bugs behind 13 tests that only failed on Windows:
- fs_lock.rs: fs2 signals lock contention differently per platform (Unix:
EWOULDBLOCK -> io::ErrorKind::WouldBlock; Windows: ERROR_LOCK_VIOLATION,
which std does not map to WouldBlock). The old `.kind() == WouldBlock`
check missed every contended lock on Windows and turned "someone else is
indexing" into a hard error instead of Ok(None). Now compares raw OS error
codes against fs2's own `lock_contended_error()`. This was the root cause
of 9 of the 13 failures (fs_lock's own tests plus every index-lock test
built on top of it). Also fixed a misplaced brace that had put
test_concurrent_lock_fails_async outside `mod tests`, so it compiled
unconditionally instead of only under cfg(test).
- file_walker/mod.rs: matches_patterns() matched include/exclude patterns
against the absolute path instead of the path relative to the walk root,
so any ancestor directory name containing the pattern as a substring
(e.g. "Users" containing "rs") produced false positives unrelated to the
file itself. Root cause of test_do_index_with_include_patterns failing
deterministically on this machine.
- tests.rs: three tests encoded Unix-only path assumptions --
`starts_with('/')` after canonicalize() (Windows yields `\?\C:\...`; now
checks `Path::is_absolute()`), and two comparing `file_path` against
literal "src/" without accounting for Windows' native '\' separator.
All 423 lib tests pass; default and `--features stack-graphs` builds and
`cargo fmt` both succeed.
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
2 participants
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.
No description provided.