Improve lookup filtering and boost larger cliques - #17
Conversation
biolink_type, only_prefixes and only_taxa were appended to a single flat "should" list, which _build_elasticsearch_query wrapped in one bool. With minimum_should_match defaulting to 1, a document only had to satisfy one clause from any category, so supplying two or more filters widened the result set instead of narrowing it. Requests using a single category were unaffected, which is why this went unnoticed. Each category now builds its own group of should-clauses and the groups are added to the query's "must", giving OR within a category and AND between categories. That matches what the OpenAPI spec and handlers/README describe, and the behaviour of the Solr reference implementation. Also fixes the values reaching the clauses: the stripped value is now the one used to build the term, so " biolink:Disease " resolves to "Disease" rather than being emitted verbatim and matching nothing. Tests cover the grouping, that filter groups are added without replacing the dis_max name query, and that exclude_prefixes populates must_not.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and tests are coherent; only a non-blocking stale README update remains.
Pull request overview
Improves Elasticsearch lookup filtering and ranking as a first step toward resolving ranking issue #132.
Changes:
- Applies OR within filter categories and AND between categories.
- Moves constraints to non-scoring filter context.
- Multiplies relevance by the clique-size factor.
File summaries
| File | Description |
|---|---|
src/nameres/handlers/lookup.py |
Updates filter grouping and scoring. |
test/test_lookup_query.py |
Tests query structure, filters, and boosting. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Blank excluded-prefix values can generate an empty prefix query that removes all results.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/nameres/handlers/lookup.py:305
- Blank excluded-prefix entries are not omitted like the positive filters are. For example,
exclude_prefixes=" | "is stripped to"|", split into two empty entries, and only one is removed above; the remaining emptyprefixquery can exclude every document that has acurie. Skip whitespace-only entries when buildingmust_not.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Addressed Copilot’s suppressed follow-up finding in 99374f4. Blank and whitespace-only exclude_prefixes segments are now ignored before building must_not clauses, with a regression test covering leading, internal, repeated, and trailing blanks. Focused tests: 10 passed; full non-integration suite: 16 passed. |
Summary
This is a first-wave Elasticsearch improvement related to TranslatorSRI/babel-validation#132. It intentionally does not close the issue.
Combine values within each positive filter category with OR and combine separate categories with AND.
Ignore blank and whitespace-only excluded-prefix values instead of emitting an empty prefix query.
Move lookup constraints into non-scoring
bool.filtercontext, leaving the name-matchingdis_maxas the only scoring query.Multiply text relevance by the Solr-equivalent clique-size factor:
text _score × log10(clique_identifier_count + 1)Elasticsearch implements this with
function_score,field_value_factorusinglog1p, andboost_mode: multiply.Scope
This is query-side only and uses the existing numeric
clique_identifier_countfield. It does not change the index mapping, require a reindex, alter the response schema, or add new matching fields.GET/POST lookup, bulk lookup, and autocomplete share this query builder, so their scores and result ordering may change. Requests combining multiple filter categories will also now apply the documented AND-between-categories behavior.
Validation
pytest -q test/test_lookup_query.py— 10 passedpytest -q -m "not integration"— 16 passed, 10 deselectedgit diff --checkTests cover OR-within/AND-between filter grouping, non-scoring filter placement, preservation of the text query, and the exact clique-count multiplier configuration.
Remaining work
This restores the clique-size ranking signal used by Solr and is expected to improve the headline Eliquis result plus several related cases, but it is not full Solr/Elasticsearch ranking parity. Elasticsearch still lacks Solr-style normalized whole-name (
*_exactish) fields and weights; cases such as Symbicort and Levemir are expected to need that follow-up mapping and reindex work.We should keep issue 132 open and benchmark the deployed results before claiming full resolution.