Skip to content

fix(core): weight the title above the body in the sqlite bm25 score - #1867

Open
xubenqiang wants to merge 2 commits into
basicmachines-co:mainfrom
xubenqiang:fix/title_weighted_bm25
Open

xubenqiang wants to merge 2 commits into
basicmachines-co:mainfrom
xubenqiang:fix/title_weighted_bm25

Conversation

@xubenqiang

Copy link
Copy Markdown

The text leg of search_notes scored with bm25(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_WEIGHTS gives one weight per search_index column in FTS5 declaration order, with the title at 5 and the rest at 1, matching the shape the issue measured.
  • BM25_SCORE_EXPRESSION builds bm25(search_index, ...) from that tuple, and compile_fts_filter uses it.
  • Three tests pin the weight table to the live column order, so an added FTS column fails the suite instead of silently shifting the weights onto the wrong column.

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).
  • Red check with the source change stashed: the new tests fail with module ... has no attribute 'SEARCH_INDEX_BM25_WEIGHTS'.
  • ruff check and ruff format --check on both files -> clean.
  • Not run: bm reindex + bm tool search-notes against 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

Copilot AI balanced review requested due to automatic review settings October 25, 2026 17:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-25T17:28:36.221914Z 1754fab PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

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>
@xubenqiang
xubenqiang force-pushed the fix/title_weighted_bm25 branch from 1754fab to 84afd56 Compare October 25, 2026 17:25
@xubenqiang xubenqiang changed the title fix(search): weight the title above the body in the SQLite bm25 score fix(core): weight the title above the body in the sqlite bm25 score Oct 25, 2026
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@phernandez

Copy link
Copy Markdown
Member

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:

  1. Use the weighted score in the metadata-filtered mixed-script path too. As the existing review thread notes, preserve_match_score still builds its derived table with plain bm25(search_index). I verified this against real SQLite FTS5: for API 文档, the same note ranks 1st without a metadata filter and 21st with metadata_filters={"status": "active"}, even though every note has that status. Please use BM25_SCORE_EXPRESSION when building fts_score as well.
  2. Add a behavioral ranking regression using real SQLite. The current tests check the weight constants and expression wiring, but do not prove the ranking improvement. A reproducible fixture is one note titled API Documentation with body "endpoint response request route " * 50, 20 notes titled Meeting N whose bodies say Step 1 update the api documentation and link it from the plan, and 100 unrelated journal notes. With the body populated in both content_stems and content_snippet, the exact-title note ranks 21st with plain BM25 and 1st with title weight 5. Please assert the result order through the real search path, and cover the filtered mixed-script case above too. No mocks are needed.

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>
@xubenqiang

Copy link
Copy Markdown
Author

Both points addressed in 5dba6c0.

1. The filtered derived table now scores with BM25_SCORE_EXPRESSION. That was indeed the gap: the preserve_match_score branch built its table with plain bm25(search_index). There is now a test asserting the derived table carries the weighted expression, plus an invariant test for the symptom you described: a metadata_filters that every note satisfies must not reorder the same query, so search("API 文档") and search("API 文档", metadata_filters={"status": "active"}) have to come back in the same order.

2. There is a behavioural ranking test through the real search path, with the fixture shape from your note: one note titled API Documentation whose body is "endpoint response request route " * 50, 20 Meeting N notes carrying the words in their body, and 100 unrelated journal notes, bodies in both content_stems and content_snippet, no mocks. It asserts the exact-title note comes first.

One honest limitation on the filtered ordering: I tried to assert the exact-title note is first for API 文档 with the filter as well, and could not make that fixture decisive. In a script query the script_ngrams column dominates the score and the title weight only multiplies the title column, so whether the titled note wins depends on body lengths in a way I could not pin down without copying your exact corpus. That is why the filtered path is pinned by the derived-table expression and the no-reorder invariant instead of an ordering claim I cannot keep stable. If you can share the fixture you used, I will add the ordering assertion on top of it.

pytest tests/repository/test_sqlite_bm25_weights.py -q -o addopts="" -> 6 passed; pytest tests/repository/test_search_repository.py -> 56 passed; ruff check and ruff format --check clean. DCO is green; the CLA check is still waiting on the account owner, which I cannot sign from here.

@xubenqiang

Copy link
Copy Markdown
Author

On the two Codex findings (reviewed commit 1754fab, before 5dba6c0):

1. "Reuse title weights in metadata-filtered script search" — fixed in 5dba6c0. The preserve_match_score branch now builds its derived table with BM25_SCORE_EXPRESSION instead of plain bm25(search_index), and a test asserts the compiled SQL carries it.

2. "Preserve title weighting for unsegmented-script queries" — still true, and out of scope here. build_script_ngrams concatenates title, content_stems and content_snippet into one column, so a pure CJK/Kana/Thai query matches script_ngrams only and the per-column title weight cannot apply. Fixing it needs a schema-level change (a separate title n-gram column, or a score that knows which source column a gram came from), which is a different piece of work from #1851. I would rather land the word-query weighting now and treat the pure-script case as its own issue.

The behavioural fixture from @phernandez's review uses API 文档 (a word plus a script term, so it exercises the mixed path) and the exact-title note now comes first there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants