Repository navigation
Conversation
semantic_query ranked nodes by per-keyword min-cosine over the RI node vectors alone. Those vectors are strongly collapsed (mean cosine to the centroid is 0.981 on this repo's own index), so ranking was close to noise, test code dominated the top results, and natural-language phrases found nothing. cbm_store_vector_search now fuses two channels with reciprocal rank fusion (k=60, lexical weight 5, vector weight 1): - lexical: FTS5 bm25 over nodes_fts with the same exact-name / case-insensitive / callable / type tiers as the BM25 query path - vector: one composed, IDF-weighted query vector from token_vectors, scored with cbm_cosine_i8 over node_vectors Query elements may be keywords or phrases: they are lowercased, stopwords are dropped, and words are split on camelCase/acronym boundaries for the vector lookup. Test symbols (test directories, test file names, test_* / Test* names, *_for_test hooks) are demoted below production code unless the query itself mentions tests. Ranking is fully deterministic and independent of limit, so the old certification loop is gone. Behavior change: a query in which no word is known to token_vectors and nothing matches lexically now returns no rows instead of the whole graph ranked against a random fallback vector. No index format change; xxhash is no longer needed in store.c. Refs DeusData#1417 Closes DeusData#1462 Impacted by AI (Agent mode: copilot | Prompts: 18 | Tokens: claude-sonnet-5.5: 274232k in/780k out (249223k cached) +196k reasoning | gpt-4o-mini-2024-07-18: 43k in/2k out) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Yoav Lax <yoavfax@gmail.com>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
This branch has not been deployed
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.
What does this PR do?
Closes #1462 (step 1: hybrid retrieval via RRF). Refs #1417.
semantic_queryranked nodes by per-keyword min-cosine over the RI node vectors alone. On this repo's own index those vectors are strongly collapsed (mean cosine to the centroid is 0.981). As a result, ranking was close to noise, test code filled the top results, and natural-language phrases found nothing.cbm_store_vector_searchnow fuses two channels with reciprocal rank fusion (k=60, lexical weight 5, vector weight 1), keyed on node id:bm25()overnodes_fts, using the same exact-name / case-insensitive / callable / type tiers as the BM25querypath.token_vectors, scored withcbm_cosine_i8overnode_vectors. Query elements can be keywords or phrases. They are lowercased, stopwords are dropped, and words are split on camelCase/acronym boundaries.test_*/Test*, or is a*_for_testhook.limit. The certification loop is removed.There is no index format change. xxhash is no longer included in
store.c. Docs are updated: thestore.hcontract, the README semantic search line, and thesemantic_querytype-error hint inmcp.c.Behavior change: if no query word is known to
token_vectorsand nothing matches lexically, the result is now empty instead of the whole graph ranked against a random fallback vector.Measurements
-O2build, baselinemain@ 10ab613.semantic_queryinputThe eval set is small, comes from a single repo, and was also used to tune the weights, so the numbers are optimistic.
Tests
tool_search_graph_semantic_hybrid_ranking. It covers:tool_search_graph_semantic_store_error_fails_closednow createstoken_vectorswith the query token. The vector scan only runs when at least one query word is known, so this keeps the test exercising the forced scan failure.Size
This is about 790 added / 230 removed lines, mostly the rewritten search block in
store.c. That is over the 500-line guideline. It is submitted as one PR so the design can be reviewed as a whole; I'm happy to split it (test demotion and deterministic order / RRF fusion / phrase and camelCase handling) on request.AI disclosure
This PR was written with an AI coding agent (GitHub Copilot) acting on my behalf. I reviewed it and am accountable for it; the commit carries my DCO sign-off.
Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test)I ran
build/c/test-runner mcponly, not the full ASan/UBSan suite. All semantic tests pass.search_code_scan_live_child_deadline_is_bounded_and_fails_closedfailed in 2 of 3 runs (a 100 ms subprocess timeout insearch_code). This change does not touch that path, but I did not run it on unchangedmain.make -f Makefile.cbm lint-ci)Only clang-format-18 was run, and it is clean on the changed ranges. clang-tidy and cppcheck were not run locally.