Repository navigation
fix(mcp): apply search_graph label filter inside the BM25 candidate window (#2386) - #2423
harshitaajoshi wants to merge 5 commits into
Conversation
|
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. |
…mics & retention, upstream watchlist - docs/ROADMAP.md: community-facing overview of both phases (T0-T7), upstream issue/PR watchlist, and the explicit not-doing list - .plan/spec.md: upstream issue sweep cross-checked against this base — T1 gains a prerequisite (BM25 label filter, issue DeusData#2386 / PR DeusData#2423, still reproducible at mcp.c:3882), T5 gains fail-loud COUNT semantics (DeusData#2012, still reproducible at store.c:3077), T2/T7 gain rebase watchlist entries and freshness-anchor constraints (DeusData#1714, DeusData#1213, DeusData#867) - discussion DeusData#2025 recorded as the hardest adoption evidence for B4/B5
…indow (DeusData#2386) Signed-off-by: harshitaajoshi <j.harshitaa06@gmail.com>
ba5e7dd to
19374f2
Compare
|
Thanks for the BM25 label-window fix and the ranking comparison. I have read the exact-filter and saturation details; this remains pending review, including the documented label-word scoring difference. |
Brings the branch up to current main (246 commits) before the zero-result follow-up. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…t zero In query (BM25) mode an explicit label naming File, Folder, Variable or Project returned 0 rows -- often with an exact "eq" total -- although those nodes are in nodes_fts and match the query: the noise exclusion meant for unlabelled queries was applied to labelled queries too. It now applies only when no label is given (one shared SQL fragment for the ranked and the count query), so such a label is served from its own label-scoped candidate window like any other label. Unlabelled queries are unchanged. A label no node carries as spelled (labels are case-sensitive) still answers 0 rows, as structural mode does, but no longer silently: the result carries a hint naming the stored spelling, or get_graph_schema when the label is unknown. An existing label with no matching row stays a plain zero. Measured on Django and meilisearch indexes (81 / 114 label queries): zero results 18 -> 0 and 21 -> 0 (8 and 18 of them reported an exact "eq" total); every other label returns byte-identical results, and unlabelled queries are byte-identical to main. Refs DeusData#2386 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you @harshitaajoshi -- this is a careful fix, and it is exactly the approach #2386 asked for. Keeping the exact While re-checking it on real indexes we found one more way a labelled query in BM25 mode came back empty. Your label-scoped window is what makes that fixable, so we pushed two small commits on top of your branch (your commit is untouched):
What we measured on Django, meilisearch and kubernetes indexes, comparing your head (merged with main) against the fix-up:
The same window effect also hits Thanks again for the reproduction test and the ranking comparison -- they made this review easy. |
|
Thanks a lot for the review and for the follow-up commits. Making the noise exclusion apply only to unlabelled queries makes sense, and the hint for a misspelled label is a nice touch. |
|
Thank you for the BM25 label-window fix and the follow-up discussion. I merged current main into the branch in |
|
Thank you again for the BM25 label-window fix. Our October updates record the additional named-label handling and baseline refresh as completed, so no repeat contributor work is requested. Sorry for the delay in the remaining maintainer follow-up—I’ll follow up soon. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again, @harshitaajoshi. We merged current main into the branch (one signed-off merge commit, 760cb9c; your commits are unchanged) so CI validates your fix against today's tree. The one red check on the previous run (the UI test job) failed while downloading a package during npm install, before any test ran, so it had nothing to do with your change. Nothing is needed from you; please pull before pushing again. |
Fixes #2386
What does this PR do?
In query (BM25) mode,
search_graphapplied thelabelfilter after the 2000 row FTS5 candidate window. When a token is common but the requested label is rare among its matches, those rows fell outside the window and the call returned 0 rows while reporting a saturated window.This PR moves the label into candidate selection, as suggested in the issue:
labelis set, the MATCH expression becomes(<tokens>) AND label : "<Label>", so the window is filled with rows of that label. The outern.label = ?check stays as the exact filter (FTS5 matching is case insensitive, so the column filter is only a superset pre-filter).total,total_relationandcandidate_window_saturateddescribe the label scoped window. A rare label no longer gets a falsegte.[A-Za-z0-9_], up to 64 chars) is spliced in, as a quoted FTS5 string. Anything else keeps the previous unscoped MATCH, so a label can never inject FTS5 syntax.Ranking stays the same
The added
label : "..."phrase is itself scored bybm25(), and its contribution depends on row length, so on its own it can reorder near ties. To avoid that, the label column weight inBM25_WEIGHTSis now a bound parameter:1.0as before,0.0when the MATCH is scoped. Every candidate carries the label in that case, so a label hit cannot distinguish them anyway.I checked this on an index of this repo's
src/(6.9k nodes) by running the ranked SQL old vs new for 20 common tokens across every label with results (75 pairs):The one intended difference: when a query token is the label word itself (for example
query=function label=Function), that token's label column hit no longer scores, since all candidates share it.Tests
tool_search_graph_bm25_label_filter_selects_candidate_window: 2001 Functions and 3 lower ranked Classes share a token.label=Classmust return all 3 withtotal:3,total_relation:eqand no saturation flag;label=Functionmust still report a saturated window; a label containing FTS5 syntax must stay in BM25 mode and return 0 rows. Fails on current main (0 Classes returned).tool_search_graph_bm25_label_scope_keeps_rank_order: a label filtered query lists its rows in the same relative order as the unfiltered query. Fails if the label weight is left at 1.0.Local run after rebasing on current main: full suite
9646 passed, 11 skipped.lint-cipasses apart from one cppcheckconstParameterCallbacknote insrc/daemon/application.c, which is already on main and not touched here.Checklist
git commit -s)make -f Makefile.cbm test)make -f Makefile.cbm lint-ci), pluslint-tidy-diffclean on the changed linesWritten with the help of Claude Code. I reviewed and ran everything above myself.