Skip to content

fix: preserve FTS ranking order#93

Merged
Moskize91 merged 1 commit into
mainfrom
codex/fix-fts-ranking-score
Jul 4, 2026
Merged

fix: preserve FTS ranking order#93
Moskize91 merged 1 commit into
mainfrom
codex/fix-fts-ranking-score

Conversation

@Moskize91

Copy link
Copy Markdown
Contributor

Summary

  • convert SQLite FTS bm25 rank to score with the correct ordering
  • bump search ranking cache version so stale sessions with inverted scores are ignored
  • add regression coverage for text sentence and object property FTS ranking

Tests

  • pnpm run lint:fix
  • pnpm test:run test/archive/query/archive-view.test.ts test/archive/query/search-cache.test.ts -- --runInBand
  • pnpm typecheck
  • pnpm format:check

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

Summary by CodeRabbit

  • Bug Fixes
    • Search session IDs now update when ranking settings change, reducing the risk of stale cached search results.
    • Search result scoring has been refined to better reflect text relevance, improving ordering for both source and chapter matches.
  • Tests
    • Added regression tests to verify within-bucket ordering follows FTS relevance scores for matching sources and chapters.

Walkthrough

This change updates search scoring in search-index.ts so BM25 rank values are transformed into a scaled, bounded relevance score. SEARCH_RANKING_VERSION in search-cache.ts is incremented from 5 to 6, changing cached search session IDs. Two regression tests are added to verify that findArchiveObjects returns source and chapter hits ordered by descending FTS relevance.

Estimated code review effort

Medium

Suggested labels

search, tests, scoring

Suggested reviewers

Maintainers familiar with the archive search index and cache logic

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change and uses a valid conventional format without a scope.
Description check ✅ Passed The description is directly related to the ranking fix, cache bump, and regression tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch codex/fix-fts-ranking-score

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/archive/search-index/search-index.ts`:
- Around line 750-752: The FTS ranking values produced by rankToScore are being
merged directly with other search-path scores in createFindResult, so normalize
the rank-derived score into the same bounded range as the lexical scores before
sorting. Update the scoring logic near rankToScore and the result aggregation in
createFindResult so bm25-based hits do not overwhelm other matches, and keep the
comparison behavior consistent for grouped entity/triple results.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1f00590f-8c2c-4eaf-867d-d9b1720657a6

📥 Commits

Reviewing files that changed from the base of the PR and between 9e7ae5d and 945997f.

📒 Files selected for processing (3)
  • src/archive/query/search-cache.ts
  • src/archive/search-index/search-index.ts
  • test/archive/query/archive-view.test.ts

Comment thread src/archive/search-index/search-index.ts
@Moskize91 Moskize91 force-pushed the codex/fix-fts-ranking-score branch from 945997f to 2bca26d Compare July 4, 2026 02:53
@Moskize91 Moskize91 merged commit f310143 into main Jul 4, 2026
2 of 3 checks passed
@Moskize91 Moskize91 deleted the codex/fix-fts-ranking-score branch July 4, 2026 02:56

@coderabbitai coderabbitai 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.

♻️ Duplicate comments (1)
src/archive/search-index/search-index.ts (1)

113-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Correct fix for the inverted BM25 ordering, but the scale constant likely over-saturates scores.

Dropping Math.abs(rank) in favor of Math.max(0, -rank) correctly fixes the inversion (bm25 returns more-negative values for better matches, so -rank now grows with relevance) — this addresses the PR's core objective.

However, FTS5_RANK_SCORE_SCALE = 1_000_000 is very large relative to typical bm25 magnitudes (commonly single digits to low tens). For almost any nonzero rank, relevance will already be in the thousands+, so relevance / (1 + relevance) saturates to ~1.0 for both strong and weak matches alike. Ordering is preserved (the function is monotonic), but the resulting score values become nearly indistinguishable in magnitude — which defeats the purpose of a "bounded relevance score" for any downstream logic that compares/merges scores across buckets or applies thresholds (this is the same underlying concern flagged in the prior review about mixing FTS-derived scores with other lexical scores in createFindResult).

Consider a much smaller scale (or a log-based/percentile normalization) so the mapped score retains meaningful separation across the realistic bm25 range.

♻️ Example: reduce scale to preserve score spread
-const FTS5_RANK_SCORE_SCALE = 1_000_000;
+const FTS5_RANK_SCORE_SCALE = 1;

Also applies to: 751-755

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/archive/search-index/search-index.ts` at line 113, The BM25 normalization
in search-index.ts is now correctly using the inverted rank, but
FTS5_RANK_SCORE_SCALE is too large and causes score saturation. Adjust the
scaling logic in the FTS5 score mapping path (around the relevance/score
calculation and the FTS5_RANK_SCORE_SCALE constant) so typical bm25 values still
produce spread-out bounded scores, rather than near-1.0 outputs for almost all
matches; keep the rank inversion fix intact and prefer a smaller scale or a
normalization approach that preserves separation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@src/archive/search-index/search-index.ts`:
- Line 113: The BM25 normalization in search-index.ts is now correctly using the
inverted rank, but FTS5_RANK_SCORE_SCALE is too large and causes score
saturation. Adjust the scaling logic in the FTS5 score mapping path (around the
relevance/score calculation and the FTS5_RANK_SCORE_SCALE constant) so typical
bm25 values still produce spread-out bounded scores, rather than near-1.0
outputs for almost all matches; keep the rank inversion fix intact and prefer a
smaller scale or a normalization approach that preserves separation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4c80c3b3-8c9d-4327-8127-cb2be69bde63

📥 Commits

Reviewing files that changed from the base of the PR and between 945997f and 2bca26d.

📒 Files selected for processing (3)
  • src/archive/query/search-cache.ts
  • src/archive/search-index/search-index.ts
  • test/archive/query/archive-view.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/archive/query/archive-view.test.ts
  • src/archive/query/search-cache.ts

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.

1 participant