diff --git a/CHANGELOG.md b/CHANGELOG.md index bf8148ea..6ebf9d0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,11 @@ Code importing the renamed `knowpro.interfaces_*` names from the `typeagent.knowpro.interfaces` aggregator is unaffected. +- Term and property indexes in memory and SQLite now share one normalization + (Unicode NFC, lowercase, trimmed, internal whitespace collapsed); whitespace-only + terms are no longer indexed (#322). SQLite property values stored by earlier + versions were only lowercased, so values with extra whitespace or decomposed + Unicode may no longer match; re-ingest such databases to rebuild the indexes. ### 0.5.0 (May 26) diff --git a/src/typeagent/knowpro/add_messages.py b/src/typeagent/knowpro/add_messages.py index 9393bbf6..9e244928 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 semantic_ref_index +from .common import normalize_term from .interfaces import ( AddMessagesResult, IKnowledgeExtractor, @@ -244,13 +245,15 @@ 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] = [] 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 b0fe14d4..b2bcfecb 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 semantic_ref_index +from .common import normalize_term from .convsettings import ConversationSettings from .interfaces import ( AddMessagesResult, @@ -548,16 +549,17 @@ 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): - 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)) + 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/property_index.py b/src/typeagent/storage/memory/property_index.py index ae928278..b5db8b97 100644 --- a/src/typeagent/storage/memory/property_index.py +++ b/src/typeagent/storage/memory/property_index.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,12 +269,12 @@ 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.""" # 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) @@ -313,9 +313,11 @@ async def remove_all_for_semref(self, semref_id: int) -> None: for key in keys_to_remove: del self._map[key] - def _prepare_term_text(self, term_text: str) -> str: - """Do any pre-processing of the term.""" - return term_text.lower() + 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/semantic_ref_index.py b/src/typeagent/storage/memory/semantic_ref_index.py index 17de695d..4cf45183 100644 --- a/src/typeagent/storage/memory/semantic_ref_index.py +++ b/src/typeagent/storage/memory/semantic_ref_index.py @@ -8,6 +8,7 @@ from typechat import Failure from ...knowpro import convknowledge, secondary_index +from ...knowpro.common import normalize_term from ...knowpro.convsettings import ConversationSettings, SemanticRefIndexSettings from ...knowpro.interfaces import ( # Interfaces.; Other imports. IConversation, @@ -675,11 +676,11 @@ async def add_term( term: str, semantic_ref_ordinal: SemanticRefOrdinal | ScoredSemanticRefOrdinal, ) -> str: + term = self._prepare_term(term) if not term: return term if not isinstance(semantic_ref_ordinal, ScoredSemanticRefOrdinal): semantic_ref_ordinal = ScoredSemanticRefOrdinal(semantic_ref_ordinal, 1.0) - term = self._prepare_term(term) existing = self._map.get(term) if existing is not None: existing.append(semantic_ref_ordinal) @@ -730,15 +731,22 @@ async def deserialize(self, data: TermToSemanticRefIndexData) -> None: self._clear() for index_item_data in data["items"]: term = index_item_data.get("term") + if term is None: + continue term = self._prepare_term(term) - scored_refs_data = index_item_data["semanticRefOrdinals"] - scored_refs = [ - ScoredSemanticRefOrdinal.deserialize(s) for s in scored_refs_data - ] - self._map[term] = scored_refs + if not term: + continue + scored_refs = self._map.setdefault(term, []) + # Raw terms that normalize to the same key may share ordinals. + seen = {ref.semantic_ref_ordinal for ref in scored_refs} + for s in index_item_data["semanticRefOrdinals"]: + ref = ScoredSemanticRefOrdinal.deserialize(s) + if ref.semantic_ref_ordinal not in seen: + seen.add(ref.semantic_ref_ordinal) + scored_refs.append(ref) def _prepare_term(self, term: str) -> str: - return term.lower() + return normalize_term(term) # ... diff --git a/src/typeagent/storage/sqlite/property_index.py b/src/typeagent/storage/sqlite/property_index.py index c4a10feb..96c3d8bb 100644 --- a/src/typeagent/storage/sqlite/property_index.py +++ b/src/typeagent/storage/sqlite/property_index.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 property_index 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 = property_index.make_property_term_text(property_name, value) - term_text = term_text.lower() # Matches PropertyIndex._prepare_term_text - property_name, value = property_index.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 = property_index.make_property_term_text(property_name, value) - term_text = term_text.lower() - property_name, value = property_index.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 = property_index.make_property_term_text(property_name, value) - term_text = term_text.lower() # Matches PropertyIndex._prepare_term_text - property_name, value = property_index.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( @@ -132,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/semantic_ref_index.py b/src/typeagent/storage/sqlite/semantic_ref_index.py index 125a374a..68d90f90 100644 --- a/src/typeagent/storage/sqlite/semantic_ref_index.py +++ b/src/typeagent/storage/sqlite/semantic_ref_index.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, @@ -47,11 +46,10 @@ async def add_term( term: str, semantic_ref_ordinal: SemanticRefOrdinal | ScoredSemanticRefOrdinal, ) -> str: + term = self._prepare_term(term) if not term: return term - term = self._prepare_term(term) - semref_id, score = _split_ordinal(semantic_ref_ordinal) cursor = self.db.cursor() @@ -73,9 +71,9 @@ async def add_terms_batch( return rows = [] for term, ordinal in terms: + term = self._prepare_term(term) if not term: continue - term = self._prepare_term(term) semref_id, score = _split_ordinal(ordinal) rows.append((term, semref_id, score)) if rows: @@ -148,9 +146,13 @@ async def deserialize(self, data: TermToSemanticRefIndexData) -> None: # Prepare all insertion data for bulk operation insertion_data = [] + # Raw terms that normalize to the same key may share ordinals. + seen: set[tuple[str, int]] = set() 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"]) + if not term: + continue for semref_ordinal_data in item["semanticRefOrdinals"]: if isinstance(semref_ordinal_data, dict): semref_id = semref_ordinal_data["semanticRefOrdinal"] @@ -159,7 +161,9 @@ async def deserialize(self, data: TermToSemanticRefIndexData) -> None: # Fallback for direct integer semref_id = semref_ordinal_data score = 1.0 - insertion_data.append((term, semref_id, score)) + if (term, semref_id) not in seen: + seen.add((term, semref_id)) + insertion_data.append((term, semref_id, score)) # Bulk insert all the data if insertion_data: @@ -169,15 +173,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_property_index.py b/tests/test_property_index.py index 9a4e433c..51724b17 100644 --- a/tests/test_property_index.py +++ b/tests/test_property_index.py @@ -315,11 +315,10 @@ async def test_property_index_get_values(property_index: PropertyIndex): assert "value2" in values -def test_property_index_prepare_term_text(property_index: PropertyIndex): - """Test preprocessing term text in PropertyIndex.""" - term_text = "Prop.Name@@Value" - prepared_text = property_index._prepare_term_text(term_text) - assert prepared_text == "prop.name@@value" # Should be converted to lowercase +def test_property_index_make_term_text(property_index: PropertyIndex): + """Test building the normalized term text in PropertyIndex.""" + term_text = property_index._make_term_text(" Name", "Some Value") + assert term_text == "prop.name@@some value" @pytest.mark.asyncio diff --git a/tests/test_term_normalization.py b/tests/test_term_normalization.py new file mode 100644 index 00000000..9dda2847 --- /dev/null +++ b/tests/test_term_normalization.py @@ -0,0 +1,181 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Memory and SQLite term indexes must normalize terms identically (#322).""" + +from collections.abc import Generator +import sqlite3 + +import pytest + +from typeagent.knowpro.common import normalize_term +from typeagent.knowpro.interfaces import ( + IPropertyToSemanticRefIndex, + ITermToSemanticRefIndex, +) +from typeagent.storage.memory.property_index import PropertyIndex +from typeagent.storage.memory.semantic_ref_index import TermToSemanticRefIndex +from typeagent.storage.sqlite.property_index import SqlitePropertyIndex +from typeagent.storage.sqlite.schema import init_db_schema +from typeagent.storage.sqlite.semantic_ref_index 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, +) -> Generator[ITermToSemanticRefIndex, None, None]: + 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, +) -> Generator[IPropertyToSemanticRefIndex, None, None]: + 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_ignores_whitespace_only_terms( + term_index: ITermToSemanticRefIndex, +) -> None: + assert await term_index.add_term(" ", 0) == "" + await term_index.add_term("\t", 0) + await term_index.add_terms_batch([("\n", 1), (" Foo", 2)]) + assert await term_index.size() == 1 + assert await term_index.get_terms() == ["foo"] + + +@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] + + +@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_deserialize_skips_empty_and_missing_terms( + term_index: ITermToSemanticRefIndex, +) -> None: + data = { + "items": [ + { + "term": " ", + "semanticRefOrdinals": [{"semanticRefOrdinal": 1, "score": 1.0}], + }, + { + "term": None, + "semanticRefOrdinals": [{"semanticRefOrdinal": 2, "score": 1.0}], + }, + {"semanticRefOrdinals": [{"semanticRefOrdinal": 3, "score": 1.0}]}, + { + "term": "bar", + "semanticRefOrdinals": [{"semanticRefOrdinal": 4, "score": 1.0}], + }, + ] + } + await term_index.deserialize(data) # type: ignore[arg-type] + assert await term_index.get_terms() == ["bar"] + + +@pytest.mark.asyncio +async def test_term_index_deserialize_dedupes_merged_ordinals( + term_index: ITermToSemanticRefIndex, +) -> None: + data = { + "items": [ + { + "term": "Foo", + "semanticRefOrdinals": [{"semanticRefOrdinal": 1, "score": 1.0}], + }, + { + "term": "foo ", + "semanticRefOrdinals": [{"semanticRefOrdinal": 1, "score": 1.0}], + }, + ] + } + await term_index.deserialize(data) # type: ignore[arg-type] + found = await term_index.lookup_term("foo") + assert found is not None and [r.semantic_ref_ordinal for r in found] == [1]