Skip to content

fix(db): dampen search rank by title length to stop repeat-term blobs outranking exact matches - #269

Open
StanislavBG wants to merge 4 commits into
midt-bg:mainfrom
StanislavBG:fix/search-ranking-exact-vs-blob-25
Open

fix(db): dampen search rank by title length to stop repeat-term blobs outranking exact matches#269
StanislavBG wants to merge 4 commits into
midt-bg:mainfrom
StanislavBG:fix/search-ranking-exact-vs-blob-25

Conversation

@StanislavBG

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • New regression test (packages/db/src/search-index-sql.test.ts) against a real SQLite FTS5 search_index, using the real production SQL constant (not a hand-copied mirror) — reproduces the exact issue Качество на търсенето: възложител с дълъг списък имена изпреварва точните съвпадения #25 scenario (a municipality blob title vs. two exact-match kindergarten entities).
  • Confirmed red against the old ORDER BY rank (reverted locally, reran the test, failed as expected), confirmed green with the fix restored.
  • Added a second test guarding against over-correction — a legitimate longer single-match title isn't unfairly sunk by the length dampening.
  • Full @sigma/db suite: 285/285 tests pass, 35/35 files, no regressions.
  • pnpm --filter @sigma/db typecheck and pnpm --filter web typecheck both clean.

🤖 Generated with a scheduler-executed PRD (692), independently re-verified (before/after test, full suite, typecheck) before opening this PR.

… outranking exact matches

FTS5's default bm25() rewards raw term frequency, so an authority whose title
concatenates several child-entity names (repeating the query terms) could
outrank an entity whose title matches the query once, cleanly (midt-bg#25)
…ly CSRF

postcss 8.5.15 -> 8.5.18+ fixes GHSA-r28c-9q8g-f849 (path traversal via
sourceMappingURL auto-load); valibot 1.4.0 -> 1.4.2+ fixes GHSA-5qjj-4xww-7phc
(flatten() crash on inherited-property keys). Both are non-breaking
patch-level overrides.

react-router 7.18.0's GHSA-qwww-vcr4-c8h2 CSRF only affects the unstable RSC
code paths (verified via repo-wide grep, zero hits) and has no fix in the 7.x
line - suppressed via osv-scanner.toml with a 2026-10-01 review date rather
than forcing a major 7.x -> 8.x bump across this PR.
@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

Прегледах #269 на head de3e9b2 — коректно решение на #25.

Математиката е правилна: SQLite bm25 връща отрицателни стойности (по-отрицателно = по-добро съвпадение), затова делене на по-голям делител при по-дълго заглавие бута ранга към 0 → по-назад в ORDER BY … ASC. Потискането е пропорционално, а не рязък праг — наистина силно дълго съвпадение още може да изпревари слабо късо (напр. rank −10 при дълго заглавие → −0.9 пак бие rank −1.5 при късо → −0.75). LENGTH() върху TEXT брои символи, не байтове — кирилските заглавия не се наказват двойно. Добре обмислено за BG.

Тестът е дискриминиращ и реален: search-index-sql.test.ts пуска истинска FTS5 таблица (не fake D1) и доказва, че „блоб" заглавие с повтарящи се термини вече НЕ изпреварва чисто единично съвпадение (clean1Rank < blobRank) — точно сценарият от #25. Правилният избор на интеграционен тест — юнит тестовете с fake D1 не биха хванали това.

Две неща (не блокират):

  1. Магическа константа /20.0 — тунинг-ключ; струва си да се документира откъде идва 20 и как се пренастройва.
  2. Смесен обхват: osv-scanner.toml + pnpm-workspace.yaml/lock са несвързани с търсенето. Suppression-ът е добър (sharp = dev-only miniflare транзитив; react-router = само unstable RSC, с ignoreUntil дати), но за чиста история е отделен CI PR. Ако е нужен за да мине CI-то на fix(db): dampen search rank by title length to stop repeat-term blobs outranking exact matches #269 — ок, само отбележи го.

Closes #25 е коректно. Одобрявам.

@todorkolev

Copy link
Copy Markdown
Collaborator

@StanislavBG - прегледах PR-а и посоката е правилна: дефектът в #25 е реален и потискането по дължина го оправя. Не мога да го слея обаче, защото StanislavBG/sigma не е форк на midt-bg/sigma, а самостоятелно хранилище - затова „разреши промени от поддръжник" не ми дава достъп до клона и не мога нито да разреша конфликта, нито да пусна поправките. Трябват две неща от теб.

1. Конфликтът (блокиращ)

След като #271 влезе в main, osv-scanner.toml, pnpm-workspace.yaml и pnpm-lock.yaml тук се бият с него - това е същата CVE чистка, носена независимо. Разреши ги към версията на main (тоест изхвърли своята), тя вече е слята. Нищо от твоята работа не се губи - реалното съдържание на PR-а е само search.ts и тестът.

2. Две поправки по същество

Изразът изключва бързия път на FTS5. ORDER BY rank е единствената форма, която FTS5 оптимизира - подрежда през своя ranking индекс и бута LIMIT надолу. Проверено с EXPLAIN QUERY PLAN върху истинска FTS5 таблица:

ORDER BY rank                      → SCAN search_index VIRTUAL TABLE INDEX 32:M6
ORDER BY rank / (1.0 + LENGTH(…))  → SCAN search_index VIRTUAL TABLE INDEX 0:M6
                                     USE TEMP B-TREE FOR ORDER BY

Тоест всеки съвпадащ ред се материализира и сортира преди LIMIT. За дума като „община" това са десетки хиляди реда - при това на всяко натискане на клавиш в SmartSearch, а D1 таксува прочетени редове.

Решението е двустепенно: вътрешна заявка взима топ 50 по оптимизирания път, външната преподрежда само тях. Резултатът за сценария от #25 е същият, а планът се връща на INDEX 32.

Тестът за дългите заглавия не може да падне. arrayContaining върху два реда при LIMIT 10 минава при всякаква подредба - включително тази, която тестът уж забранява. Направих го точна подредба и добавих тест за самия план, за да не се върне някой към едностепенна заявка.

Готовата разлика (прилага се върху текущата ти глава):

patch
diff --git a/packages/db/src/queries/search.ts b/packages/db/src/queries/search.ts
index ed603ed..994f002 100644
--- a/packages/db/src/queries/search.ts
+++ b/packages/db/src/queries/search.ts
@@ -104,15 +104,30 @@ interface HitRow {
 // offset that within-document repetition, so we dampen `rank` by title length on top of it — the
 // divisor grows by 1 per 20 chars, a soft enough curve that it reorders repeat-heavy blobs below
 // clean short matches without sinking legitimate longer (but single-match) titles disproportionately.
-const RANK_EXPR = `rank / (1.0 + LENGTH(search_index.title) / 20.0)`;
+const RANK_EXPR = `r / (1.0 + LENGTH(title) / 20.0)`;
 
-export const SEARCH_HITS_SQL = `SELECT search_index.ref, search_index.title, search_index.ident,
-       search_index.subtitle, search_index.amount,
+// Two stages, deliberately. `ORDER BY rank` is the ONLY form FTS5 optimizes: it drives the query with
+// its rank-ordering index and pushes the LIMIT down (EXPLAIN: `VIRTUAL TABLE INDEX 32:M6`). Ordering by
+// any expression OVER rank drops that (`INDEX 0:M6` + `USE TEMP B-TREE FOR ORDER BY`), so every row a
+// common term matches — tens of thousands for e.g. „община" — gets materialized and sorted before the
+// LIMIT. D1 bills rows read, so that is a real cost on the busiest query in the app.
+// So: take the top CANDIDATES by the optimized rank path, then re-rank only those. The inner slice is
+// wide enough that a repeat-heavy blob and the clean match it outranks are both inside it, and the
+// sort in the outer query is over at most CANDIDATES rows, not the whole match set.
+const CANDIDATES = 50;
+
+export const SEARCH_HITS_SQL = `SELECT h.ref, h.title, h.ident, h.subtitle, h.amount,
        ct.kind AS entity_kind, ct.ownership_kind, ct.eik_valid
-FROM search_index
+FROM (
+  SELECT search_index.ref AS ref, search_index.title AS title, search_index.ident AS ident,
+         search_index.subtitle AS subtitle, search_index.amount AS amount,
+         search_index.kind AS kind, rank AS r
+  FROM search_index
+  WHERE search_index.kind = ? AND search_index MATCH ?
+  ORDER BY rank LIMIT ${CANDIDATES}
+) h
 LEFT JOIN company_totals ct
-  ON search_index.kind = 'company' AND ct.bidder_id = search_index.ref
-WHERE search_index.kind = ? AND search_index MATCH ?
+  ON h.kind = 'company' AND ct.bidder_id = h.ref
 ORDER BY ${RANK_EXPR} LIMIT ?`;
 
 export async function search(db: D1Database, rawQuery: string): Promise<SearchResults> {
diff --git a/packages/db/src/search-index-sql.test.ts b/packages/db/src/search-index-sql.test.ts
index 3e8fc80..178b35b 100644
--- a/packages/db/src/search-index-sql.test.ts
+++ b/packages/db/src/search-index-sql.test.ts
@@ -92,15 +92,33 @@ describe('search ranking SQL (real SQLite FTS5, SEARCH_HITS_SQL)', () => {
 
       const titles = rankedTitles(dbPath, 'детска градина');
 
-      // Both are single, clean matches for the query; a longer descriptive title matching once
-      // should not be reordered to a wildly worse position than an equally clean shorter title —
-      // it should still be a top hit, not pushed out by the length dampening meant for repeat-blobs.
-      expect(titles).toEqual(
-        expect.arrayContaining([
-          'Детска градина Дъга',
-          'Общинска детска градина за изкуство №5 към Столична община район Витоша',
-        ]),
-      );
+      // Both are single, clean matches. The dampening does put the shorter one first — that is its
+      // job — but the longer descriptive title must stay a top hit rather than being buried. Asserted
+      // as the EXACT order: `arrayContaining` would pass no matter how the two are ordered (both rows
+      // come back under LIMIT 10 regardless), so it could not detect the regression it guards against.
+      expect(titles).toEqual([
+        'Детска градина Дъга',
+        'Общинска детска градина за изкуство №5 към Столична община район Витоша',
+      ]);
+    });
+  });
+
+  // The ordering above can be satisfied by ordering the whole match set — which is exactly the
+  // regression to prevent. `ORDER BY rank` is the only form FTS5 optimizes (rank-ordering index +
+  // LIMIT pushdown); ordering by an expression over rank degrades to materializing and sorting every
+  // matching row. On „община"-class terms that is tens of thousands of rows per keystroke, and D1
+  // bills rows read. Lock the plan so a later simplification back to a single-level query is caught.
+  it('drives the match with FTS5 rank ordering, not a full sort of every match', () => {
+    withDb((dbPath) => {
+      insertAuthorityRows(dbPath, [['auth:1', 'Община Ботевград']]);
+      const sql = SEARCH_HITS_SQL.replace('?', "'authority'")
+        .replace('?', `'${searchMatchQuery('ботевград')}'`)
+        .replace('?', '6');
+      const plan = sqlite(dbPath, `EXPLAIN QUERY PLAN ${sql}`);
+
+      // idx 32 is FTS5's "ordering by rank" flag; idx 0 means it fell back to an unordered scan.
+      expect(plan).toMatch(/VIRTUAL TABLE INDEX 32/);
+      expect(plan).not.toMatch(/VIRTUAL TABLE INDEX 0/);
     });
   });
 });

Проверено локално върху текущия main: typecheck 7/7, lint чист, целият набор тестове зелен (39 файла в @sigma/db, вкл. трите в search-index-sql.test.ts).

…act-vs-blob-25

# Conflicts:
#	osv-scanner.toml
#	pnpm-lock.yaml
#	pnpm-workspace.yaml
Land todorkolev's reviewed patch: two-stage query (inner ORDER BY rank
LIMIT 50 using FTS5's rank-ordering index, outer re-rank over just those
candidates) instead of ordering the whole match set by an expression over
rank, which fell back to a full temp-B-tree sort. Also merge main to
resolve the osv-scanner/pnpm conflict from midt-bg#271.
@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

Ре-проверих новия връх fc94c2e1 срещу дифа — двустепенното пренаписване е коректно и всъщност поправя перформанс дефект в предишната версия (която бях одобрил на 07-27).

Същина. Старият ORDER BY rank / (1.0 + LENGTH(title)/20.0) върху целия match set убива единствения план, който FTS5 оптимизира (ORDER BY rank с rank-подредбата + LIMIT pushdown). Израз над rankVIRTUAL TABLE INDEX 0 + temp B-tree → материализира и сортира ВСЕКИ съвпаднал ред. За „община"-клас термин това са десетки хиляди реда на заявка, а D1 таксува прочетени редове. Новата схема взима топ CANDIDATES=50 по оптимизирания rank път (вътрешна заявка), после пре-ранкира само тях с дампването по дължина. Демпването (фиксът за #25) се запазва; скъпият пълен прочит отпада. SQL-ът е коректен — параметрите (kind, match, limit) съвпадат с реда в теста; LEFT JOIN-ът мигрира чисто към подзаявката h.

Тестовете са осезаемо по-добри. Старият arrayContaining([short, long]) минаваше независимо от подредбата (и двата под LIMIT 10) — не пазеше нищо. Новият асертва ТОЧНАТА подредба [short, long] + EXPLAIN QUERY PLAN заключва VIRTUAL TABLE INDEX 32 и забранява INDEX 0. Plan-lock тест — точно каквото пази регресия обратно към бавната форма.

Една бележка (не блокер, предсъществуваща). Печалбата в прочетени редове е частична: сестринската COUNT заявка (search.ts:143SELECT kind, COUNT(*) … WHERE search_index MATCH ? GROUP BY kind) е непроменена и без LIMIT, тъй че на всеки search за „община"-клас термин тя пак чете всички съвпаднали редове. Per-request rows-read не е ограничен от CANDIDATES — hits заявката вече е евтина, но counts заявката на същия keystroke още сканира целия set. Не е регресия от този PR, но щом перформанс мотивът е точно rows-read cost, това вероятно е новият доминиращ разход — кандидат за отделен follow-up.

Тредео (не дефект). CANDIDATES=50 е евристичен прозорец: чист единичен match, паднал под топ-50 по суров bm25 докато blob е вътре, пак би загубил. 50 е щедро спрямо показваните 6–10 и е документирано. ОК.

Кодът е одобрим по същество; трябва rebase върху main (CONFLICTING).

@nedda76

nedda76 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Този клон е в конфликт с main, тъй че към момента не може да се ревюира — дифът, който GitHub показва, вече не отговаря на това, което би влязло. Ще го пребазираш ли върху актуалния main (или merge на main в клона) и да разрешиш конфликтите? След това веднага го поглеждам. Благодаря! 🙏

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

Labels

None yet

Projects

None yet

4 participants