Skip to content

fix: keep SQLite fuzzy-term index memory in sync with the database - #355

Open
Kevin Turcios (KRRT7) wants to merge 1 commit into
microsoft:mainfrom
KRRT7:fix-sqlite-fuzzy-clear
Open

Kevin Turcios (KRRT7) wants to merge 1 commit into
microsoft:mainfrom
KRRT7:fix-sqlite-fuzzy-clear

Conversation

@KRRT7

Copy link
Copy Markdown
Contributor

Summary

SqliteRelatedTermsFuzzy keeps a VectorBase, a term list (vector position → term), and an added-terms set in memory alongside the RelatedTermsFuzzy table. Three paths let the in-memory state drift from the table:

  • clear() only deleted the rows. Lookups still returned cleared terms, and re-adding a cleared term was skipped as "already added", so the table stayed empty (size 0, get_terms() == []).
  • deserialize() of empty data returned before clearing the VectorBase. Terms added afterwards were appended after the stale vectors, so their positions no longer matched the term list and lookups could not find them.
  • Opening an existing database loaded stored terms into the term list but not the added-terms set, so re-adding a stored term duplicated it: lookup_term("plum") returned plum twice and serialize() listed it twice.

The fix resets all three in-memory structures in clear() and deserialize(), and populates the added-terms set when loading.

Validation

  • New tests for each path: clear then re-add, empty deserialize then add, and reopen then re-add. Each fails without the fix.
  • make format check test

SqliteRelatedTermsFuzzy keeps a VectorBase, a term list, and an added-terms
set in memory alongside the RelatedTermsFuzzy table, but three paths let
them drift:
- clear() only deleted the rows, so lookups still returned cleared terms
  and re-adding them was skipped, leaving the table empty.
- deserialize() of empty data returned before clearing the VectorBase, so
  later terms were mapped to the wrong embeddings.
- Loading an existing database did not populate the added-terms set, so
  re-adding a stored term duplicated it in lookups and serialize().

Co-authored-by: wtfyd-agent[bot] <328989958+wtfyd-agent[bot]@users.noreply.github.com>

@bmerkle Bernhard Merkle (bmerkle) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM — verified locally.

  • All three drift paths reproduce: each new test fails against main's related_terms_index.py and passes with the fix. Full make is green (814 passed).
  • Checked the rest of the class for the same class of bug: remove_term raises NotImplementedError, and the add_terms_with_embeddings and non-empty deserialize paths already keep _vector_base / _terms_list / _added_terms in step with the table.

Non-blocking follow-up, same family: duplicates within one batch aren't filtered. add_terms(["fig", "fig", "date"]) writes 2 rows (thanks to INSERT OR REPLACE), but _terms_list/VectorBase get 3 entries, so serialize() returns ['fig', 'fig', 'date'] and lookup_term("fig") returns fig twice. Production callers pass unique terms today, so it's latent; de-duplicating new_terms in add_terms_with_embeddings (e.g. via dict.fromkeys) would close it.

This branch has not been deployed

No deployments
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.

2 participants