Add hybrid ranking to search endpoint (RRF fusion) #141

Merged

forgejo_admin merged 1 commit from 139-hybrid-ranking-search into main

2026-03-09 22:51:28 +00:00

forgejo_admin commented

2026-03-09 17:41:28 +00:00

Owner

Summary

Adds mode (keyword/semantic/hybrid) and alpha (0.0-1.0) parameters to GET /notes/search, combining tsvector keyword search and pgvector semantic search via Reciprocal Rank Fusion (RRF). Default mode=keyword preserves full backward compatibility.

Changes

src/pal_e_docs/routes/notes.py -- Refactored search endpoint to support three modes. Extracted _keyword_search, _semantic_search_notes, _embed_query, and _build_filter_clauses helpers from inline code. Mode validation runs before the PostgreSQL dialect check.
src/pal_e_docs/services/search.py -- New module implementing RRF fusion (rrf_fuse) with configurable alpha weighting and k=60 constant. Pure-Python, no DB dependency.
src/pal_e_docs/schemas.py -- Made NoteSearchResult.headline optional (str | None = None) so semantic/hybrid results can omit it.
tests/test_hybrid_search.py -- 29 tests: 13 RRF unit tests (empty lists, overlap boost, alpha extremes, clamping, tiebreaking, fallback ranks, limit), 11 endpoint validation tests (mode/alpha validation, backward compat), 2 Ollama error propagation tests, 3 schema tests.

Test Plan

pytest tests/test_hybrid_search.py -v -- 29 passed
pytest tests/test_search.py tests/test_semantic_search.py -v -- 15 passed (existing tests unbroken)
Full suite (625 tests) -- all passed
ruff check -- clean

Review Checklist

No unrelated changes
Tests pass (pytest tests/ -v -- 625 passed)
Lint clean (ruff check)
Backward compatible (default mode=keyword, headline now optional with default None)
No migrations needed
/notes/semantic-search endpoint unchanged

Plan: plan-2026-02-26-tf-modularize-postgres
Phase: phase-postgres-6e-hybrid-ranking

Closes #139

## Summary Adds `mode` (keyword/semantic/hybrid) and `alpha` (0.0-1.0) parameters to `GET /notes/search`, combining tsvector keyword search and pgvector semantic search via Reciprocal Rank Fusion (RRF). Default `mode=keyword` preserves full backward compatibility. ## Changes - **`src/pal_e_docs/routes/notes.py`** -- Refactored search endpoint to support three modes. Extracted `_keyword_search`, `_semantic_search_notes`, `_embed_query`, and `_build_filter_clauses` helpers from inline code. Mode validation runs before the PostgreSQL dialect check. - **`src/pal_e_docs/services/search.py`** -- New module implementing RRF fusion (`rrf_fuse`) with configurable alpha weighting and k=60 constant. Pure-Python, no DB dependency. - **`src/pal_e_docs/schemas.py`** -- Made `NoteSearchResult.headline` optional (`str | None = None`) so semantic/hybrid results can omit it. - **`tests/test_hybrid_search.py`** -- 29 tests: 13 RRF unit tests (empty lists, overlap boost, alpha extremes, clamping, tiebreaking, fallback ranks, limit), 11 endpoint validation tests (mode/alpha validation, backward compat), 2 Ollama error propagation tests, 3 schema tests. ## Test Plan - `pytest tests/test_hybrid_search.py -v` -- 29 passed - `pytest tests/test_search.py tests/test_semantic_search.py -v` -- 15 passed (existing tests unbroken) - Full suite (625 tests) -- all passed - `ruff check` -- clean ## Review Checklist - [x] No unrelated changes - [x] Tests pass (`pytest tests/ -v` -- 625 passed) - [x] Lint clean (`ruff check`) - [x] Backward compatible (default mode=keyword, headline now optional with default None) - [x] No migrations needed - [x] `/notes/semantic-search` endpoint unchanged ## Related - Plan: `plan-2026-02-26-tf-modularize-postgres` - Phase: `phase-postgres-6e-hybrid-ranking` Closes #139

forgejo_admin added 1 commit

2026-03-09 17:41:28 +00:00

Add hybrid ranking to search endpoint (mode + alpha params)

ci/woodpecker/pr/woodpecker Pipeline failed

Details

8c94ecd292

Implements Reciprocal Rank Fusion (RRF) to combine keyword (tsvector)
and semantic (pgvector) search results via the existing GET /notes/search
endpoint. Adds mode parameter (keyword/semantic/hybrid) and alpha
parameter (0.0-1.0) for weighting control. Default mode=keyword
preserves backward compatibility.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented

2026-03-09 22:30:35 +00:00

Author

Owner

PR #141 Review

BLOCKERS

None. The implementation is correct, well-structured, and backward compatible.

NITS

RRF docstring formula is inverted (src/pal_e_docs/services/search.py, line 7). The module docstring says:
```
RRF(d) = alpha * 1/(k + rank_keyword(d)) + (1 - alpha) * 1/(k + rank_semantic(d))
```
But the code (line 90) correctly implements:
```
(1.0 - alpha) * 1/(k + kr) + alpha * 1/(k + sr)
```
The code matches the Query parameter description (0.0=keyword, 1.0=semantic) and the tests confirm it. The docstring formula has alpha and (1-alpha) swapped. Low-risk since the code is correct, but confusing for anyone reading the module header.
DISTINCT ON + LIMIT truncation (_semantic_search_notes, lines 305-319). DISTINCT ON (n.slug) forces ORDER BY n.slug as the primary sort, and LIMIT :limit then clips alphabetically rather than by similarity. The Python re-sort at line 323 fixes ordering but cannot recover high-similarity notes whose slugs were alphabetically past the cutoff. For hybrid mode the internal_limit = min(limit * 3, 100) mitigates this, but a subquery approach (SELECT * FROM (DISTINCT ON subquery) ORDER BY similarity DESC LIMIT :limit) would be more correct. Note: this is a pre-existing pattern from 6d, not introduced by this PR. File a separate issue if desired.
Duplicate Ollama embedding code -- The /notes/semantic-search endpoint (lines 516-553) still has its own inline copy of the Ollama embedding + error handling logic that is now extracted into _embed_query (lines 238-277). The /notes/semantic-search endpoint was intentionally left untouched per the issue scope, so this is expected. Consider a follow-up cleanup issue to DRY up the two paths.
pg_client fixture duplicated -- tests/test_hybrid_search.py defines its own pg_client fixture (lines 229-244) that is nearly identical to the one in tests/test_semantic_search.py (lines 60-83). A shared conftest fixture would reduce duplication. Non-blocking.

SOP COMPLIANCE

Branch named after issue (139-hybrid-ranking-search references issue #139)
PR body has: ## Summary, ## Changes, ## Test Plan, ## Related
Related section references the plan slug (plan-2026-02-26-tf-modularize-postgres)
Closes #139 present in PR body
Tests exist (29 new tests) and full suite reported as passing (625 tests)
No secrets, .env files, or credentials committed
No unnecessary file changes (4 files, all in scope)
No Alembic migrations (pure application logic, as specified)
/notes/semantic-search endpoint untouched (confirmed: lines 498-624 identical to main)
NoteSearchResult.headline made optional with backward-compatible default (None)
Commit messages are descriptive

Code Quality Notes

RRF implementation is correct. k=60, fallback rank = len(list)+1, alpha clamping, stable slug-alphabetical tiebreak. Standard formulation.
Backward compatibility preserved. mode defaults to keyword, alpha defaults to 0.5, headline defaults to None. Existing callers see identical behavior.
Error handling is solid. Mode validation fires before the SQLite 501 check. Ollama failures propagate as 503. Alpha bounds enforced by FastAPI ge=0.0, le=1.0 with defensive clamping in rrf_fuse.
Clean separation of concerns. services/search.py is pure Python with no DB dependency, easily unit-testable. Route logic is straightforward dispatch.
Test coverage is thorough. Unit tests cover empty lists, single-list scenarios, overlap boost, alpha extremes, clamping, tiebreaking, fallback ranks, limit, and headline preservation. Endpoint tests cover mode validation, alpha validation, backward compat, and Ollama error propagation.

VERDICT: APPROVED

## PR #141 Review ### BLOCKERS None. The implementation is correct, well-structured, and backward compatible. ### NITS 1. **RRF docstring formula is inverted** (`src/pal_e_docs/services/search.py`, line 7). The module docstring says: ``` RRF(d) = alpha * 1/(k + rank_keyword(d)) + (1 - alpha) * 1/(k + rank_semantic(d)) ``` But the code (line 90) correctly implements: ```python (1.0 - alpha) * 1/(k + kr) + alpha * 1/(k + sr) ``` The code matches the Query parameter description (`0.0=keyword, 1.0=semantic`) and the tests confirm it. The docstring formula has alpha and (1-alpha) swapped. Low-risk since the code is correct, but confusing for anyone reading the module header. 2. **`DISTINCT ON` + `LIMIT` truncation** (`_semantic_search_notes`, lines 305-319). `DISTINCT ON (n.slug)` forces `ORDER BY n.slug` as the primary sort, and `LIMIT :limit` then clips alphabetically rather than by similarity. The Python re-sort at line 323 fixes ordering but cannot recover high-similarity notes whose slugs were alphabetically past the cutoff. For hybrid mode the `internal_limit = min(limit * 3, 100)` mitigates this, but a subquery approach (`SELECT * FROM (DISTINCT ON subquery) ORDER BY similarity DESC LIMIT :limit`) would be more correct. **Note: this is a pre-existing pattern from 6d, not introduced by this PR.** File a separate issue if desired. 3. **Duplicate Ollama embedding code** -- The `/notes/semantic-search` endpoint (lines 516-553) still has its own inline copy of the Ollama embedding + error handling logic that is now extracted into `_embed_query` (lines 238-277). The `/notes/semantic-search` endpoint was intentionally left untouched per the issue scope, so this is expected. Consider a follow-up cleanup issue to DRY up the two paths. 4. **`pg_client` fixture duplicated** -- `tests/test_hybrid_search.py` defines its own `pg_client` fixture (lines 229-244) that is nearly identical to the one in `tests/test_semantic_search.py` (lines 60-83). A shared conftest fixture would reduce duplication. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue (`139-hybrid-ranking-search` references issue #139) - [x] PR body has: ## Summary, ## Changes, ## Test Plan, ## Related - [x] Related section references the plan slug (`plan-2026-02-26-tf-modularize-postgres`) - [x] `Closes #139` present in PR body - [x] Tests exist (29 new tests) and full suite reported as passing (625 tests) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (4 files, all in scope) - [x] No Alembic migrations (pure application logic, as specified) - [x] `/notes/semantic-search` endpoint untouched (confirmed: lines 498-624 identical to main) - [x] `NoteSearchResult.headline` made optional with backward-compatible default (`None`) - [x] Commit messages are descriptive ### Code Quality Notes - **RRF implementation is correct.** k=60, fallback rank = len(list)+1, alpha clamping, stable slug-alphabetical tiebreak. Standard formulation. - **Backward compatibility preserved.** `mode` defaults to `keyword`, `alpha` defaults to 0.5, `headline` defaults to `None`. Existing callers see identical behavior. - **Error handling is solid.** Mode validation fires before the SQLite 501 check. Ollama failures propagate as 503. Alpha bounds enforced by FastAPI `ge=0.0, le=1.0` with defensive clamping in `rrf_fuse`. - **Clean separation of concerns.** `services/search.py` is pure Python with no DB dependency, easily unit-testable. Route logic is straightforward dispatch. - **Test coverage is thorough.** Unit tests cover empty lists, single-list scenarios, overlap boost, alpha extremes, clamping, tiebreaking, fallback ranks, limit, and headline preservation. Endpoint tests cover mode validation, alpha validation, backward compat, and Ollama error propagation. ### VERDICT: APPROVED