From c982a156fb1260e28ccf476ec286a7029970a000 Mon Sep 17 00:00:00 2001 From: Bernhard Merkle Date: Tue, 29 Sep 2026 00:27:04 +0200 Subject: [PATCH 1/2] Share term normalization between memory and SQLite indexes 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 #322 Co-Authored-By: Claude Sonnet 5.5 --- src/typeagent/knowpro/add_messages.py | 3 +- src/typeagent/knowpro/common.py | 14 +++ src/typeagent/knowpro/conversation_base.py | 7 +- src/typeagent/storage/memory/propindex.py | 15 ++- src/typeagent/storage/memory/semrefindex.py | 5 +- src/typeagent/storage/sqlite/propindex.py | 27 ++--- src/typeagent/storage/sqlite/semrefindex.py | 16 +-- tests/test_term_normalization.py | 120 ++++++++++++++++++++ 8 files changed, 163 insertions(+), 44 deletions(-) create mode 100644 tests/test_term_normalization.py diff --git a/src/typeagent/knowpro/add_messages.py b/src/typeagent/knowpro/add_messages.py index 295143a8..5cb12a54 100644 --- a/src/typeagent/knowpro/add_messages.py +++ b/src/typeagent/knowpro/add_messages.py @@ -13,6 +13,7 @@ from ..aitools.embeddings import IEmbeddingModel, NormalizedEmbedding from ..storage.memory import semrefindex +from .common import normalize_term from .interfaces import ( AddMessagesResult, IKnowledgeExtractor, @@ -250,7 +251,7 @@ def _collect_related_terms_for_fuzzy_index( related_terms: list[str] = [] def _add_term(term: str) -> None: - canonical = term.strip().lower() + canonical = normalize_term(term) if canonical and canonical not in seen: seen.add(canonical) related_terms.append(canonical) diff --git a/src/typeagent/knowpro/common.py b/src/typeagent/knowpro/common.py index 7634e413..0eebfc48 100644 --- a/src/typeagent/knowpro/common.py +++ b/src/typeagent/knowpro/common.py @@ -1,9 +1,23 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. +import re +import unicodedata + from .interfaces import SearchTerm +def normalize_term(term: str) -> str: + """Canonical form of an index term, shared by all storage backends. + + Strips surrounding whitespace, applies NFC normalization, collapses runs + of whitespace to a single space, and lowercases. Every term index must + apply this on both write and lookup so backends agree on term identity. + """ + term = unicodedata.normalize("NFC", term.strip()) + return re.sub(r"\s+", " ", term).lower() + + def is_search_term_wildcard(search_term: SearchTerm) -> bool: """Check if a search term is a wildcard.""" return search_term.term.text == "*" diff --git a/src/typeagent/knowpro/conversation_base.py b/src/typeagent/knowpro/conversation_base.py index 312cb9a2..217a668c 100644 --- a/src/typeagent/knowpro/conversation_base.py +++ b/src/typeagent/knowpro/conversation_base.py @@ -22,6 +22,7 @@ from ..aitools import model_adapters, utils from ..aitools.embeddings import NormalizedEmbedding from ..storage.memory import semrefindex +from .common import normalize_term from .convsettings import ConversationSettings from .interfaces import ( AddMessagesResult, @@ -494,12 +495,12 @@ async def _update_related_terms_incremental( for semref in new_semrefs: knowledge = semref.knowledge if isinstance(knowledge, ConcreteEntity): - new_terms.add(knowledge.name.lower()) + new_terms.add(normalize_term(knowledge.name)) elif isinstance(knowledge, Topic): - new_terms.add(knowledge.text.lower()) + new_terms.add(normalize_term(knowledge.text)) elif isinstance(knowledge, Action): for verb in knowledge.verbs: - new_terms.add(verb.lower()) + new_terms.add(normalize_term(verb)) if new_terms: await fuzzy_index.add_terms(list(new_terms)) diff --git a/src/typeagent/storage/memory/propindex.py b/src/typeagent/storage/memory/propindex.py index ae928278..0abffd3d 100644 --- a/src/typeagent/storage/memory/propindex.py +++ b/src/typeagent/storage/memory/propindex.py @@ -6,6 +6,7 @@ from typing import assert_never from ...knowpro.collections import TextRangesInScope +from ...knowpro.common import normalize_term from ...knowpro.interfaces import ( IConversation, IPropertyToSemanticRefIndex, @@ -240,13 +241,12 @@ async def add_property( value: str, semantic_ref_ordinal: SemanticRefOrdinal | ScoredSemanticRefOrdinal, ) -> None: - term_text = make_property_term_text(property_name, value) + term_text = self._make_term_text(property_name, value) if isinstance(semantic_ref_ordinal, int): semantic_ref_ordinal = ScoredSemanticRefOrdinal( semantic_ref_ordinal, 1.0, ) - term_text = self._prepare_term_text(term_text) if term_text in self._map: self._map[term_text].append(semantic_ref_ordinal) else: @@ -269,8 +269,7 @@ async def lookup_property( property_name: str, value: str, ) -> list[ScoredSemanticRefOrdinal] | None: - term_text = make_property_term_text(property_name, value) - return self._map.get(self._prepare_term_text(term_text)) + return self._map.get(self._make_term_text(property_name, value)) async def remove_property(self, prop_name: str, semref_id: int) -> None: """Remove all properties for a specific property name and semantic ref.""" @@ -315,7 +314,13 @@ async def remove_all_for_semref(self, semref_id: int) -> None: def _prepare_term_text(self, term_text: str) -> str: """Do any pre-processing of the term.""" - return term_text.lower() + return normalize_term(term_text) + + def _make_term_text(self, property_name: str, value: str) -> str: + """Build the normalized key; must match the SQLite property index.""" + return make_property_term_text( + normalize_term(property_name), normalize_term(value) + ) async def lookup_property_in_property_index( diff --git a/src/typeagent/storage/memory/semrefindex.py b/src/typeagent/storage/memory/semrefindex.py index edd1cba7..0c687cd1 100644 --- a/src/typeagent/storage/memory/semrefindex.py +++ b/src/typeagent/storage/memory/semrefindex.py @@ -8,6 +8,7 @@ from typechat import Failure from ...knowpro import convknowledge, secindex +from ...knowpro.common import normalize_term from ...knowpro.convsettings import ConversationSettings, SemanticRefIndexSettings from ...knowpro.interfaces import ( # Interfaces.; Other imports. IConversation, @@ -735,10 +736,10 @@ async def deserialize(self, data: TermToSemanticRefIndexData) -> None: scored_refs = [ ScoredSemanticRefOrdinal.deserialize(s) for s in scored_refs_data ] - self._map[term] = scored_refs + self._map.setdefault(term, []).extend(scored_refs) def _prepare_term(self, term: str) -> str: - return term.lower() + return normalize_term(term) # ... diff --git a/src/typeagent/storage/sqlite/propindex.py b/src/typeagent/storage/sqlite/propindex.py index c2fc274e..3377275d 100644 --- a/src/typeagent/storage/sqlite/propindex.py +++ b/src/typeagent/storage/sqlite/propindex.py @@ -6,12 +6,12 @@ from collections.abc import Sequence import sqlite3 +from ...knowpro.common import normalize_term from ...knowpro.interfaces import ( IPropertyToSemanticRefIndex, ScoredSemanticRefOrdinal, SemanticRefOrdinal, ) -from ...storage.memory import propindex class SqlitePropertyIndex(IPropertyToSemanticRefIndex): @@ -49,12 +49,8 @@ async def add_property( score = 1.0 # Normalize property name and value (to match in-memory implementation) - term_text = propindex.make_property_term_text(property_name, value) - term_text = term_text.lower() # Matches PropertyIndex._prepare_term_text - property_name, value = propindex.split_property_term_text(term_text) - # Remove "prop." prefix that was added by make_property_term_text - if property_name.startswith("prop."): - property_name = property_name[5:] + property_name = normalize_term(property_name) + value = normalize_term(value) cursor = self.db.cursor() cursor.execute( @@ -85,12 +81,9 @@ async def add_properties_batch( else: semref_id = ordinal score = 1.0 - term_text = propindex.make_property_term_text(property_name, value) - term_text = term_text.lower() - property_name, value = propindex.split_property_term_text(term_text) - if property_name.startswith("prop."): - property_name = property_name[5:] - rows.append((property_name, value, score, semref_id)) + rows.append( + (normalize_term(property_name), normalize_term(value), score, semref_id) + ) cursor = self.db.cursor() cursor.executemany( "INSERT INTO PropertyIndex (prop_name, value_str, score, semref_id) VALUES (?, ?, ?, ?)", @@ -107,12 +100,8 @@ async def lookup_property( value: str, ) -> list[ScoredSemanticRefOrdinal] | None: # Normalize property name and value (to match in-memory implementation) - term_text = propindex.make_property_term_text(property_name, value) - term_text = term_text.lower() # Matches PropertyIndex._prepare_term_text - property_name, value = propindex.split_property_term_text(term_text) - # Remove "prop." prefix that was added by make_property_term_text - if property_name.startswith("prop."): - property_name = property_name[5:] + property_name = normalize_term(property_name) + value = normalize_term(value) cursor = self.db.cursor() cursor.execute( diff --git a/src/typeagent/storage/sqlite/semrefindex.py b/src/typeagent/storage/sqlite/semrefindex.py index f462955e..a2463a7e 100644 --- a/src/typeagent/storage/sqlite/semrefindex.py +++ b/src/typeagent/storage/sqlite/semrefindex.py @@ -4,10 +4,9 @@ """SQLite-based semantic reference index implementation.""" from collections.abc import Sequence -import re import sqlite3 -import unicodedata +from ...knowpro.common import normalize_term from ...knowpro.interfaces import ( ITermToSemanticRefIndex, ScoredSemanticRefOrdinal, @@ -168,15 +167,4 @@ async def deserialize(self, data: TermToSemanticRefIndexData) -> None: ) def _prepare_term(self, term: str) -> str: - """Normalize term by converting to lowercase, stripping whitespace, and normalizing Unicode.""" - # Strip leading/trailing whitespace - term = term.strip() - - # Normalize Unicode to NFC form (canonical composition) - term = unicodedata.normalize("NFC", term) - - # Collapse multiple whitespace characters to single space - term = re.sub(r"\s+", " ", term) - - # Convert to lowercase - return term.lower() + return normalize_term(term) diff --git a/tests/test_term_normalization.py b/tests/test_term_normalization.py new file mode 100644 index 00000000..c4abadaf --- /dev/null +++ b/tests/test_term_normalization.py @@ -0,0 +1,120 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Memory and SQLite term indexes must normalize terms identically (#322).""" + +import sqlite3 + +import pytest + +from typeagent.knowpro.common import normalize_term +from typeagent.knowpro.interfaces import ( + IPropertyToSemanticRefIndex, + ITermToSemanticRefIndex, +) +from typeagent.storage.memory.propindex import PropertyIndex +from typeagent.storage.memory.semrefindex import TermToSemanticRefIndex +from typeagent.storage.sqlite.propindex import SqlitePropertyIndex +from typeagent.storage.sqlite.schema import init_db_schema +from typeagent.storage.sqlite.semrefindex import SqliteTermToSemanticRefIndex + + +@pytest.mark.parametrize( + "raw, expected", + [ + ("Space Needle ", "space needle"), + ("A ", "a"), + ("\tfoo\n bar", "foo bar"), + ("Café", "café"), # NFD -> NFC + (" ", ""), + ("", ""), + ], +) +def test_normalize_term(raw: str, expected: str) -> None: + assert normalize_term(raw) == expected + + +@pytest.fixture(params=["memory", "sqlite"]) +def term_index(request: pytest.FixtureRequest): + if request.param == "memory": + yield TermToSemanticRefIndex() + else: + db = sqlite3.connect(":memory:") + init_db_schema(db) + yield SqliteTermToSemanticRefIndex(db) + db.close() + + +@pytest.fixture(params=["memory", "sqlite"]) +def prop_index(request: pytest.FixtureRequest): + if request.param == "memory": + yield PropertyIndex() + else: + db = sqlite3.connect(":memory:") + init_db_schema(db) + yield SqlitePropertyIndex(db) + db.close() + + +@pytest.mark.asyncio +async def test_term_index_normalizes_whitespace_and_case( + term_index: ITermToSemanticRefIndex, +) -> None: + assert await term_index.add_term("Space Needle ", 1) == "space needle" + assert await term_index.get_terms() == ["space needle"] + found = await term_index.lookup_term("space needle") + assert found is not None and [r.semantic_ref_ordinal for r in found] == [1] + found = await term_index.lookup_term(" SPACE\tNEEDLE") + assert found is not None and [r.semantic_ref_ordinal for r in found] == [1] + + +@pytest.mark.asyncio +async def test_term_index_lookup_ignores_trailing_space( + term_index: ITermToSemanticRefIndex, +) -> None: + await term_index.add_term("A", 0) + found = await term_index.lookup_term("A ") + assert found is not None and [r.semantic_ref_ordinal for r in found] == [0] + + +@pytest.mark.asyncio +async def test_term_index_whitespace_only_terms_agree( + term_index: ITermToSemanticRefIndex, +) -> None: + await term_index.add_term(" ", 0) + await term_index.add_term("\t", 0) + assert await term_index.size() == 1 + assert await term_index.get_terms() == [""] + + +@pytest.mark.asyncio +async def test_term_index_deserialize_merges_colliding_terms( + term_index: ITermToSemanticRefIndex, +) -> None: + data = { + "items": [ + { + "term": "Foo", + "semanticRefOrdinals": [{"semanticRefOrdinal": 1, "score": 1.0}], + }, + { + "term": "foo ", + "semanticRefOrdinals": [{"semanticRefOrdinal": 2, "score": 1.0}], + }, + ] + } + await term_index.deserialize(data) # type: ignore[arg-type] + found = await term_index.lookup_term("foo") + assert found is not None + assert sorted(r.semantic_ref_ordinal for r in found) == [1, 2] + + +@pytest.mark.asyncio +async def test_property_index_normalizes_values( + prop_index: IPropertyToSemanticRefIndex, +) -> None: + await prop_index.add_property("name", "Space Needle ", 1) + found = await prop_index.lookup_property("name", "space needle") + assert found is not None and [r.semantic_ref_ordinal for r in found] == [1] + found = await prop_index.lookup_property("NAME", " Space\tNeedle") + assert found is not None and [r.semantic_ref_ordinal for r in found] == [1] From d3e4fa798fa331b90aa81f7ceb383aac05b92b52 Mon Sep 17 00:00:00 2001 From: Bernhard Merkle Date: Tue, 29 Sep 2026 00:35:14 +0200 Subject: [PATCH 2/2] Address review feedback on term normalization - 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 --- src/typeagent/knowpro/add_messages.py | 4 ++- src/typeagent/knowpro/conversation_base.py | 3 ++- src/typeagent/storage/memory/propindex.py | 1 + src/typeagent/storage/sqlite/propindex.py | 2 +- src/typeagent/storage/sqlite/semrefindex.py | 2 +- tests/test_term_normalization.py | 30 +++++++++++++++++++-- 6 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/typeagent/knowpro/add_messages.py b/src/typeagent/knowpro/add_messages.py index 5cb12a54..3362daff 100644 --- a/src/typeagent/knowpro/add_messages.py +++ b/src/typeagent/knowpro/add_messages.py @@ -245,7 +245,9 @@ def _collect_related_terms_for_fuzzy_index( """Collect canonical related-term texts for the fuzzy related-terms index. These terms are derived from the same knowledge that feeds semantic refs. - We lowercase and deduplicate while preserving order to match index behavior. + We normalize each term with `normalize_term` (strip, NFC, collapse + whitespace, lowercase), drop empty results, and deduplicate while + preserving order to match index behavior. """ seen: set[str] = set() related_terms: list[str] = [] diff --git a/src/typeagent/knowpro/conversation_base.py b/src/typeagent/knowpro/conversation_base.py index 217a668c..b8b1adb9 100644 --- a/src/typeagent/knowpro/conversation_base.py +++ b/src/typeagent/knowpro/conversation_base.py @@ -491,7 +491,7 @@ async def _update_related_terms_incremental( fuzzy_index = self.secondary_indexes.term_to_related_terms_index.fuzzy_index if fuzzy_index is not None and new_semrefs: - new_terms = set() + new_terms: set[str] = set() for semref in new_semrefs: knowledge = semref.knowledge if isinstance(knowledge, ConcreteEntity): @@ -501,6 +501,7 @@ async def _update_related_terms_incremental( elif isinstance(knowledge, Action): for verb in knowledge.verbs: new_terms.add(normalize_term(verb)) + new_terms.discard("") # Whitespace-only terms must not be embedded if new_terms: await fuzzy_index.add_terms(list(new_terms)) diff --git a/src/typeagent/storage/memory/propindex.py b/src/typeagent/storage/memory/propindex.py index 0abffd3d..26dba174 100644 --- a/src/typeagent/storage/memory/propindex.py +++ b/src/typeagent/storage/memory/propindex.py @@ -274,6 +274,7 @@ async def lookup_property( async def remove_property(self, prop_name: str, semref_id: int) -> None: """Remove all properties for a specific property name and semantic ref.""" # Find and remove entries matching both property name and semref_id + prop_name = normalize_term(prop_name) keys_to_remove = [] for term_text, scored_refs in self._map.items(): prop_name_from_term, _ = split_property_term_text(term_text) diff --git a/src/typeagent/storage/sqlite/propindex.py b/src/typeagent/storage/sqlite/propindex.py index 3377275d..96c3d8bb 100644 --- a/src/typeagent/storage/sqlite/propindex.py +++ b/src/typeagent/storage/sqlite/propindex.py @@ -121,7 +121,7 @@ async def remove_property(self, prop_name: str, semref_id: int) -> None: cursor = self.db.cursor() cursor.execute( "DELETE FROM PropertyIndex WHERE prop_name = ? AND semref_id = ?", - (prop_name, semref_id), + (normalize_term(prop_name), semref_id), ) async def remove_all_for_semref(self, semref_id: int) -> None: diff --git a/src/typeagent/storage/sqlite/semrefindex.py b/src/typeagent/storage/sqlite/semrefindex.py index a2463a7e..2e07191e 100644 --- a/src/typeagent/storage/sqlite/semrefindex.py +++ b/src/typeagent/storage/sqlite/semrefindex.py @@ -149,7 +149,7 @@ async def deserialize(self, data: TermToSemanticRefIndexData) -> None: # Prepare all insertion data for bulk operation insertion_data = [] for item in data["items"]: - if item and item["term"]: + if item and item.get("term") is not None: term = self._prepare_term(item["term"]) for semref_ordinal_data in item["semanticRefOrdinals"]: if isinstance(semref_ordinal_data, dict): diff --git a/tests/test_term_normalization.py b/tests/test_term_normalization.py index c4abadaf..e414070f 100644 --- a/tests/test_term_normalization.py +++ b/tests/test_term_normalization.py @@ -3,6 +3,7 @@ """Memory and SQLite term indexes must normalize terms identically (#322).""" +from collections.abc import Generator import sqlite3 import pytest @@ -35,7 +36,9 @@ def test_normalize_term(raw: str, expected: str) -> None: @pytest.fixture(params=["memory", "sqlite"]) -def term_index(request: pytest.FixtureRequest): +def term_index( + request: pytest.FixtureRequest, +) -> Generator[ITermToSemanticRefIndex, None, None]: if request.param == "memory": yield TermToSemanticRefIndex() else: @@ -46,7 +49,9 @@ def term_index(request: pytest.FixtureRequest): @pytest.fixture(params=["memory", "sqlite"]) -def prop_index(request: pytest.FixtureRequest): +def prop_index( + request: pytest.FixtureRequest, +) -> Generator[IPropertyToSemanticRefIndex, None, None]: if request.param == "memory": yield PropertyIndex() else: @@ -118,3 +123,24 @@ async def test_property_index_normalizes_values( assert found is not None and [r.semantic_ref_ordinal for r in found] == [1] found = await prop_index.lookup_property("NAME", " Space\tNeedle") assert found is not None and [r.semantic_ref_ordinal for r in found] == [1] + + +@pytest.mark.asyncio +async def test_property_index_remove_property_normalizes_name( + prop_index: IPropertyToSemanticRefIndex, +) -> None: + await prop_index.add_property(" Name ", "x", 1) + await prop_index.remove_property(" NAME", 1) + assert not await prop_index.lookup_property("name", "x") + + +@pytest.mark.asyncio +async def test_term_index_roundtrips_empty_normalized_term( + term_index: ITermToSemanticRefIndex, +) -> None: + await term_index.add_term(" ", 3) + data = await term_index.serialize() + await term_index.clear() + await term_index.deserialize(data) + found = await term_index.lookup_term("") + assert found is not None and [r.semantic_ref_ordinal for r in found] == [3]