Skip to content

fix: make SQLite term normalization idempotent - #350

Open
Kevin Turcios (KRRT7) wants to merge 3 commits into
microsoft:mainfrom
KRRT7:fix-sqlite-term-normalization
Open

Kevin Turcios (KRRT7) wants to merge 3 commits into
microsoft:mainfrom
KRRT7:fix-sqlite-term-normalization

Conversation

@KRRT7

@KRRT7 Kevin Turcios (KRRT7) commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Term normalization applied NFC before lowercasing. Lowercasing can produce a sequence that NFC then composes differently, so the normalization was not idempotent:
    • "T̈" → "ẗ", but normalizing that again gives "ẗ" (U+1E97).
    • A term returned by get_terms() could not be looked up, and deserialize, which re-normalizes stored terms, silently lost them.
    • Affected inputs are an uppercase letter followed by a combining mark with no precomposed uppercase form (e.g. T̈, J̌, Greek polytonic such as Α͂).
  • Lowercase first, then apply NFC, in the shared typeagent.knowpro.common.normalize_term (Share term normalization between memory and SQLite indexes #325). This fixes the memory and SQLite term indexes and the property indexes together. An exhaustive check over every code point, plus letter/digit + combining-mark pairs, found non-idempotent inputs with the old order and none with the new one.
  • Compatibility: indexes that stored one of these rare terms keep it in the old form, so lookups for it miss until re-ingested (noted in the 0.6.0 CHANGELOG). Other terms, including precomposed accents like café, normalize the same under both orders.

Validation

  • New tests in tests/test_term_normalization.py: normalize_term is idempotent on the affected inputs, and for both the memory and SQLite term indexes the stored term looks up the same as the original input, lowercase input finds it, and lookups survive a serialize/deserialize round-trip. All 9 new cases fail without the fix.
  • make format check test

_prepare_term applied NFC before lowercasing. Lowercasing can produce a
sequence that NFC composes differently (e.g. "T̈" -> "ẗ" ->
"ẗ"), so normalizing a stored term again gave a different key: terms from
get_terms() could not be looked up, and deserialize, which re-normalizes
stored terms, silently lost them. Lowercase first, then apply NFC.

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.

Thanks — the analysis is right and the fix is correct. I confirmed it exhaustively on Python 3.15 / Unicode 17 (every code point, plus every letter/digit + Mn combining-mark pair): the NFC→lower order has 433 non-idempotent inputs, lower→NFC has 0, and no single code point normalizes differently between the two orders, so ordinary text such as café is unaffected.

However, the PR now conflicts with main: #325 moved normalization into the shared typeagent.knowpro.common.normalize_term, and SqliteTermToSemanticRefIndex._prepare_term just delegates to it. Could you rebase and:

  1. Apply the reorder in normalize_term (common.py) instead of _prepare_term, and update its docstring ("strips, lowercases, applies NFC, collapses whitespace"). That fixes both backends, since the memory TermToSemanticRefIndex.deserialize also re-normalizes stored terms, and it covers the property indexes too.
  2. Move the regression test to tests/test_term_normalization.py, add a direct normalize_term(normalize_term(t)) == normalize_term(t) case, and parametrize the lookup/serialize round-trip over the existing term_index fixture so memory and SQLite are both covered.

I checked that version locally: on main with the reorder in normalize_term, your test cases plus a direct idempotency check fail before the change (6) and pass after it, and full make is green (839 passed).

Minor: the compatibility note (existing SQLite DBs keep such terms in the old form) seems worth a line in the 0.6.0 (unreleased) CHANGELOG section.

…lization

# Conflicts:
#	src/typeagent/storage/sqlite/semantic_ref_index.py
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.
@KRRT7

Copy link
Copy Markdown
Contributor Author

Thanks, done as suggested:

  1. Merged main and dropped the _prepare_term change; the reorder is now in normalize_term (common.py), with the docstring updated (ad57e20).
  2. Moved the regression tests to tests/test_term_normalization.py: a direct normalize_term(normalize_term(t)) == normalize_term(t) case, plus the lookup/serialize round-trip parametrized over the term_index fixture (memory and SQLite). All 9 new cases fail on main and pass with the change.
  3. Added the compatibility note to the 0.6.0 (unreleased) CHANGELOG section.

make format check test is green (842 passed). PR description updated to match.

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