Repository navigation
Share term normalization between memory and SQLite indexes - #325
Merged
Bernhard Merkle (bmerkle) merged 5 commits intoOct 18, 2026
Merged
Bernhard Merkle (bmerkle) merged 5 commits into
Bernhard Merkle (bmerkle) merged 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Removal paths and SQLite deserialization do not consistently preserve normalized keys.
Review effort: Balanced
Findings: 4
Open (6)
What changed in this PR
Centralizes term normalization to align memory and SQLite index behavior.
Changes:
- Adds shared Unicode, whitespace, and case normalization.
- Applies normalization across term, property, and fuzzy indexes.
- Adds backend parity tests and collision-aware deserialization.
| File | Description |
|---|---|
src/typeagent/knowpro/common.py |
Defines shared normalization. |
src/typeagent/knowpro/add_messages.py |
Normalizes fuzzy-index terms. |
src/typeagent/knowpro/conversation_base.py |
Normalizes incremental related terms. |
src/typeagent/storage/memory/semrefindex.py |
Normalizes keys and merges collisions. |
src/typeagent/storage/memory/propindex.py |
Normalizes property names and values separately. |
src/typeagent/storage/sqlite/semrefindex.py |
Uses shared normalization. |
src/typeagent/storage/sqlite/propindex.py |
Uses shared property normalization. |
tests/test_term_normalization.py |
Adds cross-backend parity tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
Author
|
robgruen can you rereview please ? |
The memory term index only lowercased terms while the SQLite one also stripped, NFC-normalized and collapsed whitespace, so lookups, get_terms() and size() disagreed between backends. Add a shared normalize_term() and use it in both term indexes and both property indexes (name and value are normalized separately). Also merge, rather than overwrite, terms that collide after normalization in the memory deserialize, and use the shared function for the fuzzy related-terms lists. Fixes microsoft#322 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
- Normalize prop_name in remove_property (memory and SQLite) - Keep the empty normalized term through SQLite deserialize - Don't embed empty normalized terms in the incremental fuzzy update - Update the _collect_related_terms_for_fuzzy_index docstring - Annotate the test fixtures; add tests for removal and empty-term round trip Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Bernhard Merkle (bmerkle)
force-pushed
the
fix-term-normalization-parity
branch
from
October 11, 2026 13:39
d3e4fa7 to
f3748a2
Compare
- Normalize before the empty check in add_term/add_terms_batch so whitespace-only terms are not stored under an empty key. - Skip missing, None and empty terms on deserialize in both backends (the memory backend previously crashed on a missing term). - Dedupe semref ordinals when several raw terms normalize to the same key on deserialize, in memory and SQLite. - Remove the unused PropertyIndex._prepare_term_text. - Note in the CHANGELOG that SQLite property values from earlier versions may need re-ingesting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bernhard Merkle (bmerkle)
requested a review
from Guido van Rossum (gvanrossum)
October 11, 2026 13:55
Collaborator
Author
|
robgruen can you rereview please ? |
Collaborator
Author
|
Guido van Rossum (@gvanrossum) could you take a look at this when you have a moment? robgruen seems to be busy, so I'm asking you as the other requested reviewer. Summary:
Applying the same normalization to search terms, query matchers and the alias index is left to #346 to keep this PR small. |
Guido van Rossum (gvanrossum)
approved these changes
Oct 18, 2026
Guido van Rossum (gvanrossum)
left a comment
Guido van Rossum (gvanrossum)
left a comment
Collaborator
There was a problem hiding this comment.
Rubberstamp
Kevin Turcios (KRRT7)
added a commit
to KRRT7/typeagent-py
that referenced
this pull request
Oct 18, 2026
Lowercase before NFC in typeagent.knowpro.common.normalize_term, which both backends now share (microsoft#325), instead of in the SQLite-only _prepare_term. Move the regression tests to tests/test_term_normalization.py, cover memory and SQLite, and note the stored-term compatibility impact in the CHANGELOG.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #322.
The memory
TermToSemanticRefIndexonly lowercased terms, while the SQLite one also stripped, NFC-normalized and collapsed whitespace. Lookups,get_terms()andsize()therefore disagreed between backends.normalize_term()inknowpro/common.py(the SQLite behavior, promoted to the shared definition) and use it in the memory and SQLite term indexes and property indexes. Property name and value are normalized separately, so leading whitespace in a value is handled.deserializenow merges terms that collide after normalization instead of overwriting.add_messages,conversation_base) use the shared function too.tests/test_term_normalization.pyruns parity checks against both backends.Behavior change: memory-backed index keys are now normalized with the SQLite rules (as the issue suggested).
🤖 Generated with Claude Code