Skip to content

Unified highlighter for association search (hl.method=unified) - #1361

Draft
kevinschaper wants to merge 1 commit into
mainfrom
schema-hl-unified
Draft

Unified highlighter for association search (hl.method=unified)#1361
kevinschaper wants to merge 1 commit into
mainfrom
schema-hl-unified

Conversation

@kevinschaper

Copy link
Copy Markdown
Member

What

Sets hl.method=unified on the association free-text search (adds an optional hl_method field to SolrQuery, serialized to hl.method; None by default so other queries are unchanged).

Why

Pairs with monarch-initiative/monarch-ingest#708, which drops term vectors from the text field type (big index-size + indexing-speed win). Solr 8's default original highlighter uses term vectors and does not read postings offsets; the Unified highlighter reads the storeOffsetsWithPositions offsets that PR adds. Without this, highlighting would fall back to slow per-result re-analysis once term vectors are gone.

Must ship together with the ingest PR.

Still needed (frontend)

Stop wrapping q in *…* wildcards; rely on the analyzed _t fields already in the qf (the backend passes q through raw — solr_query_utils.py:132).

Tests

tests/unit/test_datamodels_solr.py passes (11/11); hl_method defaults to None so it's omitted from every existing query string.

https://claude.ai/code/session_012eRPzupZErBJN7495bWEVD

Set hl.method=unified on the association free-text search so highlighting reads
offsets from the postings (storeOffsetsWithPositions) instead of term vectors.
Pairs with monarch-initiative/monarch-ingest#708, which drops term vectors from
the `text` field type to shrink the index and speed indexing. Solr 8 defaults to
the `original` highlighter, which uses term vectors and does NOT read postings
offsets, so without this change highlighting would fall back to slow per-result
re-analysis once term vectors are gone.

Adds an optional hl_method field to SolrQuery (serialized to hl.method); None by
default, so all other queries are unchanged.

Companion still needed: frontend should stop wrapping q in wildcards and rely on
the analyzed _t fields already in the qf.

Claude-Session: https://claude.ai/code/session_012eRPzupZErBJN7495bWEVD
@netlify

netlify Bot commented Jul 11, 2026

Copy link
Copy Markdown

Deploy Preview for monarch-app canceled.

Name Link
🔨 Latest commit e74d162
🔍 Latest deploy log https://app.netlify.com/projects/monarch-app/deploys/6a51ce7dab993b0008cc9439

@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review

Small, well-scoped change - thanks for the clear PR description explaining the Solr internals. One correctness issue and a scope question below.

Breaks existing unit tests

Adding hl_method: Optional[str] = None as a new SolrQuery field means SolrQuery.model_dump() now always includes an hl_method key (value None when unset), since pydantic includes all fields by default in model_dump().

tests/unit/test_solr_queries.py has several tests that do:

query = build_association_query(...).model_dump()
expected = association_query_direct  # a plain dict fixture
assert compare_dicts(query, expected)

and compare_dicts (backend/src/monarch_py/utils/utils.py:43-45) is:

def compare_dicts(dict1, dict2):
    return all([k in dict2 for k in dict1]) and all([dict1[k] == dict2[k] for k in dict2])

This requires every key in the actual dict (dict1) to exist in the expected fixture (dict2). None of the fixture dicts in backend/tests/fixtures/ (association_query_direct.py, association_query_indirect.py, search_query.py, histopheno_query.py, mapping_query.py, autocomplete_query.py) include an hl_method key, so compare_dicts should now return False for each.

That means these tests should now fail (based on reading the code/fixtures - wasn't able to execute pytest to confirm at runtime, so please double check):

  • test_build_association_query (all 4 parametrized cases)
  • test_build_association_counts_query
  • test_build_histopheno_query
  • test_build_search_query
  • test_build_autocomplete_query
  • test_build_mappings_query

The PR description says "tests/unit/test_datamodels_solr.py passes (11/11)" - that file is unaffected since it only checks query_string() (which filters out None values), but test_solr_queries.py compares raw model_dump() output and looks like it wasn't run. Worth adding hl_method to the affected fixtures (or excluding it appropriately), and running the full backend unit suite (pytest backend/tests/unit) before merge.

Scope question: entity/autocomplete search also highlights via the same field type

build_search_query (solr_query_utils.py:252, used by SolrImplementation.search, the /search endpoint) also sets query.hl = highlighting on the entity core, but this PR doesn't set hl_method there. If monarch-ingest#708 drops term vectors from the shared text field type across cores (not just the association core), entity search highlighting would silently degrade to the slow original highlighter once that ingest change ships. If the entity core's schema is unaffected this is fine as-is - just flagging to confirm scope, since the failure mode here (slow highlighting, not an error) is easy to miss until production.

Minor

  • The inline comments explaining why hl.method reads offsets instead of using term vectors are genuinely useful given how non-obvious Solr's highlighter internals are - good call keeping them.
  • No new test explicitly asserts hl.method=unified appears in the query string. Adding a row to the existing table-driven test in test_datamodels_solr.py would future-proof this and would likely have caught the fixture-mismatch issue above.

Otherwise the _solrize mapping and None default (so the param is omitted from unrelated queries) look correct, and the change is appropriately minimal for a fix that must ship in lockstep with the ingest PR.

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