Repository navigation
review: hybrid search + exclude_paths + logging fix - #1
Conversation
Adds a 'hybrid' search mode that combines vector similarity search with keyword matching using Reciprocal Rank Fusion (RRF). RRF operates on rank positions rather than raw scores, making it robust to scale incompatibility between embedding distances and keyword match counts. Items appearing in both result sets receive a consensus boost. Formula: RRF_score(d) = sum(1/(k + rank_i(d))) + consensus_boost Reference: Cormack, Clarke & Buettcher (2009) The keyword search uses INSTR-based term matching on the existing code_chunks_vec table, requiring no schema changes or FTS5 setup. Also includes exclude_paths support (from cocoindex-io#146) as it shares the same full-scan query path. Usage: MCP: {"query": "auth", "mode": "hybrid"} CLI: ccc search 'auth' --mode hybrid Closes cocoindex-io#44
- Protocol: encode/decode with exclude_paths, mode, backward compat - RRF fusion: vector-only, keyword-only, consensus boost, limit, score formula - 8 tests, all passing
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements hybrid search functionality, combining vector similarity with keyword-based search using Reciprocal Rank Fusion (RRF), and introduces a new feature to exclude specific file paths via glob patterns. Key feedback includes a high-severity performance concern regarding the use of INSTR for keyword matching instead of FTS5, and a logic error where limit and offset are applied before result fusion, potentially excluding relevant matches. Additional suggestions focus on improving CLI validation for search modes, refining keyword extraction heuristics for short code terms, and replacing magic numbers and non-standard SQL syntax with clearer implementations.
| match_expr = " + ".join( | ||
| f"(CASE WHEN INSTR(LOWER(content), LOWER(?)) > 0 THEN 1 ELSE 0 END)" | ||
| for _ in keywords | ||
| ) |
There was a problem hiding this comment.
Using INSTR(LOWER(content), LOWER(?)) inside a SQL query results in a full table scan and expensive string operations for every row in the code_chunks_vec table. For larger codebases, this will significantly degrade search performance. Consider using SQLite's FTS5 (Full-Text Search) extension for efficient keyword indexing and searching.
| if mode == "hybrid": | ||
| # Extract keywords from query (simple tokenization) | ||
| keywords = [w for w in query.lower().split() if len(w) >= 3] | ||
| if keywords: | ||
| with db.readonly() as conn: | ||
| keyword_rows = _keyword_query( | ||
| conn, keywords, limit * 2, languages, paths, exclude_paths | ||
| ) | ||
| return _fuse_rrf(vector_results, keyword_rows, limit) |
There was a problem hiding this comment.
The current implementation of hybrid search applies limit and offset to the vector results before performing RRF fusion. This is incorrect because:
- Results that rank highly in keyword search but are just outside the
limitin vector search will not be correctly fused (they will only be treated as keyword-only results). - When an
offsetis provided, the fusion only considers vector results starting from that offset, completely ignoring the top vector results which might be the most relevant when combined with keywords.
Fusion should be performed on a larger pool of candidates (e.g., top 100 from both) without applying the user's offset yet. The offset and limit should then be applied to the final ranked list produced by _fuse_rrf.
| lang: list[str] = _typer.Option([], "--lang", help="Filter by language"), | ||
| path: str | None = _typer.Option(None, "--path", help="Filter by file path glob"), | ||
| exclude: list[str] = _typer.Option([], "--exclude", help="Exclude file path glob"), | ||
| mode: str = _typer.Option("semantic", "--mode", help="Search mode: semantic or hybrid"), |
There was a problem hiding this comment.
The --mode option accepts any string, but the implementation only specifically handles "hybrid", defaulting to "semantic" for anything else. It would be better to restrict the allowed values at the CLI level to provide better feedback to the user and prevent invalid inputs.
| return conn.execute( | ||
| f""" | ||
| SELECT file_path, language, content, start_line, end_line, | ||
| ({match_expr}) as match_count | ||
| FROM code_chunks_vec | ||
| {where} | ||
| HAVING match_count > 0 | ||
| ORDER BY match_count DESC | ||
| LIMIT ? | ||
| """, | ||
| params, | ||
| ).fetchall() |
There was a problem hiding this comment.
Using HAVING without a GROUP BY clause to filter by a column alias is non-standard SQL and can lead to unexpected behavior in some database engines, although SQLite allows it. It is generally safer and clearer to use a subquery or a Common Table Expression (CTE) to filter by the match_count alias.
return conn.execute(
f"""
SELECT file_path, language, content, start_line, end_line, match_count
FROM (
SELECT file_path, language, content, start_line, end_line,
({match_expr}) as match_count
FROM code_chunks_vec
{where}
)
WHERE match_count > 0
ORDER BY match_count DESC
LIMIT ?
""",
params,
).fetchall()| # Consensus boost: items in both lists get a small bonus | ||
| consensus = set(vector_map.keys()) & set(keyword_map.keys()) | ||
| for key in consensus: | ||
| scores[key] += 0.003 |
There was a problem hiding this comment.
The value 0.003 used for the consensus boost is a magic number. It would be better to define it as a named constant (e.g., _RRF_CONSENSUS_BOOST) and provide a brief explanation for why this specific value was chosen, as it affects the final ranking in a non-standard way for RRF.
|
|
||
| if mode == "hybrid": | ||
| # Extract keywords from query (simple tokenization) | ||
| keywords = [w for w in query.lower().split() if len(w) >= 3] |
There was a problem hiding this comment.
The keyword extraction logic len(w) >= 3 might be too restrictive for code search, as it will ignore important short terms like io, go, fs, or db. Consider lowering this threshold or using a more sophisticated tokenizer that understands common programming terms.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces hybrid search functionality using Reciprocal Rank Fusion (RRF) and adds an exclude_paths filter across the CLI, client, and server components. Key feedback includes a correction for a SQL syntax error with the HAVING clause in SQLite, a fix for incorrect pagination logic in the hybrid search implementation, and suggestions for better query tokenization and input validation for the search mode.
| return conn.execute( | ||
| f""" | ||
| SELECT file_path, language, content, start_line, end_line, | ||
| ({match_expr}) as match_count | ||
| FROM code_chunks_vec | ||
| {where} | ||
| HAVING match_count > 0 | ||
| ORDER BY match_count DESC | ||
| LIMIT ? | ||
| """, | ||
| params, | ||
| ).fetchall() |
There was a problem hiding this comment.
O uso de HAVING sem GROUP BY no SQLite para filtrar por um alias (match_count) não funciona como pretendido para filtragem de linhas individuais; ele transforma a consulta em uma agregação global do conjunto de resultados. Para filtrar corretamente cada linha pelo match_count, utilize uma subconsulta (como sugerido abaixo) ou repita a expressão na cláusula WHERE.
return conn.execute(
f"""
SELECT file_path, language, content, start_line, end_line, match_count
FROM (
SELECT file_path, language, content, start_line, end_line,
({match_expr}) as match_count
FROM code_chunks_vec
{where}
)
WHERE match_count > 0
ORDER BY match_count DESC
LIMIT ?
""",
params,
).fetchall()| if mode == "hybrid": | ||
| # Extract keywords from query (simple tokenization) | ||
| keywords = [w for w in query.lower().split() if len(w) >= 3] | ||
| if keywords: | ||
| with db.readonly() as conn: | ||
| keyword_rows = _keyword_query( | ||
| conn, keywords, limit * 2, languages, paths, exclude_paths | ||
| ) | ||
| return _fuse_rrf(vector_results, keyword_rows, limit) |
There was a problem hiding this comment.
A lógica de paginação para o modo hybrid está incorreta. Atualmente, o offset é aplicado aos resultados vetoriais antes da fusão RRF (linha 247), enquanto a busca por palavras-chave (_keyword_query) não considera o offset e sempre retorna os primeiros resultados. Além disso, a função _fuse_rrf retorna apenas os primeiros limit itens da fusão, ignorando o offset global. Para que o RRF funcione corretamente com paginação, a fusão deve ser realizada sobre um conjunto maior de resultados de ambas as fontes (ex: limit + offset) e o fatiamento por offset deve ser aplicado apenas ao final da lista fundida.
|
|
||
| if mode == "hybrid": | ||
| # Extract keywords from query (simple tokenization) | ||
| keywords = [w for w in query.lower().split() if len(w) >= 3] |
There was a problem hiding this comment.
A tokenização da query usando apenas .split() é muito simplista para buscas em código, pois manterá símbolos colados às palavras (ex: func() resultará no token func()). Isso pode reduzir a eficácia da busca por palavras-chave. Considere usar uma expressão regular para extrair termos alfanuméricos ou remover pontuação comum antes de dividir a string.
| mode: str = Field( | ||
| default="semantic", | ||
| description=( | ||
| "Search mode: 'semantic' (vector similarity, default)" | ||
| " or 'hybrid' (combines vector + keyword search with RRF)." | ||
| ), | ||
| ), |
There was a problem hiding this comment.
O parâmetro mode no servidor MCP não possui validação de valores permitidos. Considere utilizar um tipo Literal["semantic", "hybrid"] para restringir as opções na interface e evitar que usuários forneçam modos inexistentes ou cometam erros de digitação.
- Use CTE instead of non-standard HAVING without GROUP BY - Fetch more vector results before RRF fusion (limit*3) to avoid missing relevant matches that rank just outside the vector limit - Extract _RRF_CONSENSUS_BOOST as named constant (was magic 0.003) - Lower min keyword length to 2 (captures io, go, fs, db) - Use regex tokenizer instead of naive split for code-like terms - Validate mode parameter in query_codebase() and CLI - Add 5 new tests for keyword extraction (13 total)
1 participant
PR interno para review do Gemini Code Assist.
Combina as 3 contribuições:
@gemini-code-assist review