Skip to content

BM25 and vector search on materialized entries - #5

Merged
eddietejeda merged 2 commits into
mainfrom
feat/search
Jul 22, 2026
Merged

BM25 and vector search on materialized entries#5
eddietejeda merged 2 commits into
mainfrom
feat/search

Conversation

@eddietejeda

Copy link
Copy Markdown
Contributor

What

Search over cached entries, declared on the decorator and queried on the frame:

  • @materialize(index=BM25("notes")) / index=Vector("notes", provider=..., metric="cosine") — indexes created at persist time (server-side async builds), with a bounded retry for the RESOURCE_LOCKED window right after the data load (observed live).
  • frame.search(query, column=, limit=) — BM25 with relevance score.
  • frame.vector_search(query, column=, limit=) — semantic search; targets the indexed source column, query text embedded server-side.
  • Decoration-time ValueError for the platform rule that embedding-backed vector indexes cannot coexist with other indexes on one table — a bad combination would otherwise fail the whole persist.

Verified live

BM25: search("checkout errors outage") returned the outage row (score 2.23). Vector: vector_search("the site was down") returned the outage row nearest (cosine distance 0.61) via the workspace's OpenAI embedding provider.

Tests

73 passing; mypy and flake8 clean.

- @materialize(index=BM25("col") | Vector("col", provider=, metric=)):
  indexes are created right after the entry's data loads (async builds
  server-side), with a bounded retry for the table lock held by the load
- frame.search(query, column=) — BM25, best matches first with score
- frame.vector_search(query, column=) — semantic; pass the indexed source
  column, query text is embedded server-side (verified live)
- Decoration-time guard: an embedding-backed vector index cannot combine
  with other indexes on one entry (platform rule; a bad combo would
  otherwise destroy the persist)
- README search section; DESIGN moves index= from planned to implemented
Comment thread hotdata_materialized/store.py Outdated
Comment on lines +191 to +192
for declaration in indexes or ():
self._create_index(created.default_connection_id, declaration)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (not blocking) _create_index runs inside the _persist try-block, so an index-creation failure (a bad provider id, an unsupported metric, or a table lock that outlasts the 25s retry window) propagates to the except at line 218 and calls _cleanup_failed_build, tearing down the fully-loaded entry. The data load already succeeded, so the practical effect is that a search-index provisioning problem permanently prevents plain caching of that entry — every call re-runs the function and re-fails the index (fail-open still returns correct data, just uncached forever, and only a log line signals it). Consider decoupling index kickoff from entry publication so a failed/late index doesn't discard otherwise-valid cached data.

claude[bot]
claude Bot previously approved these changes Jul 22, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search feature looks solid — column identifiers are validated via quote_ident and query text is literal-encoded, so the search SQL is injection-safe. One non-blocking note left inline on index-failure handling.

Index kickoff moves outside the publication failure domain: a bad
provider id or stuck table lock no longer discards otherwise-valid
cached data. The entry stays served; search errors at the search site.
Comment on lines +226 to +233
try:
for declaration in indexes or ():
self._create_index(created.default_connection_id, declaration)
except Exception as exc:
raise StoreError(
f"index creation for entry {fp.short(fingerprint)} failed "
f"(entry cached; search unavailable): {exc}"
) from exc

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (not blocking) The comment says "the entry stays served," and that holds in the default background path (the exception is only logged in _persist_background, and the test covers it). But in synchronous mode (background=False), this raise StoreError propagates through materialize()_persist() back into the decorator's except MaterializedError (decorator.py:226), so the very first call logs "persist of %s failed; serving the result uncached" and returns a frame with cached=False — even though the entry was successfully published to the registry a few lines above. It self-heals (the next call hits STATUS_READY), so no data is lost, but the first-call log and cached flag are misleading for an index-only failure. Consider logging the index failure inside _persist instead of raising, so the successful publication isn't reported as a persist failure.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search feature looks solid. The prior thread on index-failure discarding cached data is addressed (mark_ready now precedes index creation), SQL is injection-safe via quote_ident/quote_literal, the embedding-coexistence rule is enforced at decoration time, and tests cover the paths well. One non-blocking nit inline on the synchronous-mode failure surfacing.

@eddietejeda
eddietejeda merged commit ecc27f8 into main Jul 22, 2026
3 checks passed
@eddietejeda
eddietejeda deleted the feat/search branch July 22, 2026 23:27
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