Repository navigation
fix(core): weight the title above the body in the sqlite bm25 score - #1867
xubenqiang wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The text leg scored with bm25(search_index) and no column weights, so a note whose title is the query ranked below notes that merely mention the words in their body. Pass one weight per search_index column with the title at 5 and the rest at 1, and pin the weight table to the table's column order with tests. Refs basicmachines-co#1851 Signed-off-by: 许本强 <820396428@qq.com>
1754fab to
84afd56
Compare
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1754fab4ee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| match_conditions: list[str] = [] | ||
| from_clause = "search_index" | ||
| score_expression = "bm25(search_index)" | ||
| score_expression = BM25_SCORE_EXPRESSION |
There was a problem hiding this comment.
Reuse title weights in metadata-filtered script search
When metadata_filters is combined with a mixed word/script query, compile_fts_filter enters preserve_match_score, builds its derived table with the still-hard-coded bm25(search_index), and then replaces this expression with search_index.fts_score. The new weighted expression is therefore never used for that supported path, so a title word and a body word still carry equal weight; build fts_score with BM25_SCORE_EXPRESSION too.
Useful? React with 👍 / 👎.
| # project_id, from_id, to_id, relation_type, entity_id, category, metadata, | ||
| # created_at, updated_at). The title is weighted above the body so a note whose | ||
| # title is the query does not rank below notes that merely mention the words. | ||
| SEARCH_INDEX_BM25_WEIGHTS: tuple[float, ...] = (1.0, 5.0) + (1.0,) * 15 |
There was a problem hiding this comment.
Preserve title weighting for unsegmented-script queries
For a pure CJK, Kana, Thai, or other supported unsegmented-script query, analyze_script_query makes compile_fts_filter match only script_ngrams, while build_script_ngrams has already concatenated title, content_stems, and content_snippet into that one column. BM25 consequently uses the new 1.0 weight for script_ngrams, never the 5.0 title weight, so title-only and body-only occurrences remain indistinguishable; preserve the source column or include a separately weighted title match for this path.
Useful? React with 👍 / 👎.
|
Thanks for tackling this, @xubenqiang! Giving title matches more weight looks like the right direction. I ran the new weight tests and the existing search repository suite: all 59 tests pass. I also reproduced an English-query case where the title weight moves the exact-title note from 21st to 1st. Before approval, I would like to see two things addressed:
The contributor CLA is also still pending; please complete the CLA Assistant check before merge. Thanks again for the contribution! |
The metadata-filtered script path builds a derived table and ranked it with plain bm25(search_index), so a title weight was lost exactly when a filter was present. Score that table with BM25_SCORE_EXPRESSION too. Tests: a behavioural ranking test following the issue's fixture, an assertion that the derived table carries the weighted expression, and an invariant that a filter matching every note does not reorder the same query. Refs basicmachines-co#1851 Signed-off-by: 许本强 <820396428@qq.com>
|
Both points addressed in 5dba6c0. 1. The filtered derived table now scores with 2. There is a behavioural ranking test through the real search path, with the fixture shape from your note: one note titled One honest limitation on the filtered ordering: I tried to assert the exact-title note is first for
|
|
On the two Codex findings (reviewed commit 1. "Reuse title weights in metadata-filtered script search" — fixed in 5dba6c0. The 2. "Preserve title weighting for unsegmented-script queries" — still true, and out of scope here. The behavioural fixture from @phernandez's review uses |
The text leg of
search_notesscored withbm25(search_index)and no column weights, so the title counted like any body column and a note whose title was exactly the query ranked below notes that merely mention the words in their body. Hybrid inherits the text rank, so it was wrong there too.What changed
SEARCH_INDEX_BM25_WEIGHTSgives one weight persearch_indexcolumn in FTS5 declaration order, with the title at 5 and the rest at 1, matching the shape the issue measured.BM25_SCORE_EXPRESSIONbuildsbm25(search_index, ...)from that tuple, andcompile_fts_filteruses it.Verification
python -m pytest tests/repository/test_sqlite_bm25_weights.py -q -o addopts=""-> 3 passed.python -m pytest tests/repository/test_search_repository.py -q -o addopts=""-> 56 passed (no ordering regression in the existing SQLite search suite).module ... has no attribute 'SEARCH_INDEX_BM25_WEIGHTS'.ruff checkandruff format --checkon both files -> clean.bm reindex+bm tool search-notesagainst a real project, which is what the issue measured. An attempt to reproduce the ranking in a unit test with fixture rows did not reproduce it (the fixture rows did not exercise the term-frequency case the issue describes), so the tests assert the weight table and its use rather than the end-to-end order.Refs #1851