Skip to content

Align Unicode numbers in Android lab marker keys - #2552

Open
kavemang wants to merge 3 commits into
ryanbr:mainfrom
kavemang:fix/lab-marker-unicode-numbers-2351
Open

kavemang wants to merge 3 commits into
ryanbr:mainfrom
kavemang:fix/lab-marker-unicode-numbers-2351

Conversation

@kavemang

@kavemang kavemang commented Sep 28, 2026 •

Copy link
Copy Markdown

What this PR does

Android custom lab-marker keys retain Unicode letter/other numbers and supplementary characters, matching Swift for NFC-composable names. A marker such as eGFR mL/min/1.73m² now uses the same canonical key on both platforms.

Reimport also repairs affected history while the original marker name is available. Both generic CSV and WHOOP biomarker exports derive the exact historical Android UTF-16 key alongside the current key. For each device, the importer moves legacy readings only when the current target has no readings on any day/source. Empty keys, conflicting targets, ambiguous aliases in the file, and an old key explicitly imported as another current marker are left alone.

The move changes only markerKey; the subsequent normal upsert applies the imported readings. Reading IDs, dates, source, values, units, notes and reference text in moved history survive. Repair reimports preserve natural-key IDs on Android as Swift already does. History moves, incoming readings and the old/new daily projections share one transaction, so failure rolls everything back and stale old-key projections disappear. No schema migration or guessed reverse mapping is involved.

NFC-composable names agree across platforms. Swift grapheme clusters and Kotlin code points can still differ for uncomposed combining marks; that existing limitation remains documented.

Verification

All 18 expected CI checks passed at 842d4fcad1179f25c04414323f427f00d9b5080f: native macOS/iOS app builds, macOS StrandTests, Android APK/full unit suite, Swift packages/tools, parity governance, Tools Python, i18n and source hygiene. The final exact-head check API reports the same 18-check roster with zero non-success results, and the PR head was re-read to confirm it matches. Current main was integrated with a normal merge; the sole generated-map conflict was resolved through successful guarded regeneration.

  • Optimized production Swift output for 11 historical-key cases is pinned verbatim in Swift/Kotlin tests, including Unicode number categories, surrogate pairs, combining marks, percent tokens, empty keys and the historical single-pass underscore collapse.
  • Thirteen real-SQLite Swift store tests passed, including preserved history/fields, both projections, another device, an existing target on another day/source, invalid/empty keys, repeat import and rollback after a failed incoming write.
  • Thirty-five focused Swift generic/vendor parser tests passed, including name-derived migration metadata and ambiguous-alias refusal. Local Linux validation used a temporary FoundationXML import workaround for an unrelated upstream XlsxSheet.swift build gap; the file was restored and the workaround is not committed.
  • Thirty-eight JVM tests passed using actual pure parser sources and production DAO transaction bodies with recorded primitive writes. This focused harness does not compile the Android app or exercise Room's generated implementation.
  • Normal parity regeneration passed with 289 known findings. Store/DAO writers and the new legacy helpers have reciprocal twin claims. The Android repository forwarding facade has an explicit out-of-scope twin disposition for Swift's direct actor call in LabBookView.importMarkersCsv.

Related issues

Refs #2351

@ryanbr

ryanbr commented Sep 29, 2026

Copy link
Copy Markdown
Owner

@kavemang The category alignment is right, and I verified it against the Swift source rather than the description: ch.isLetter || ch.isNumber covers L*, Nd, Nl and No, and isLetterOrDigit || LETTER_NUMBER || OTHER_NUMBER covers the same four. Moving from UTF-16 Char to code points is a second real fix on top, since the old loop would have split a supplementary character into two units and mapped both to _.

One blocking thing, and it is not visible from your branch.

Rebased onto current main, the repository-acceptance suite fails:

twin-map-authority-drift|unpaired_functions cannot enter the compact baseline
authority functions: 4509 -> 4510

The nested fun isWordChar is a declaration the ledger counts, and it enters unpaired while Tools/parity_twin_map.json still says 4509. Merged as-is this would turn main red.

Your 17/17 is not wrong, it is just older than the gate. Your base predates #2582, which an hour ago moved that suite onto the unfiltered leg precisely because a change can move the declaration graph without parity-governance running. #2557 did exactly this earlier today and main carried it red until #2581. This is the same shape, caught before the merge instead of after.

The fix I would suggest is not the obvious one. Refreshing the derived snapshots and disposing the new function as one-sided debt would work, but it records debt that does not exist. Swift already has its own nested isWordChar, and it carries no twin claim of its own; the claim on that file sits on customKey. So after this change there are two genuinely-twin helpers, both unpaired.

Reciprocal claims on the two nested functions should pair them instead, leaving unpaired_functions where it is and needing no disposition at all. Then python3 Tools/parity_ledger.py --refresh-derived. If claim resolution turns out not to reach a nested declaration, refresh plus a disposition is the fallback, but it is worth trying the pairing first: they are twins in the ordinary sense, not a one-sided helper.

Two notes that are not blocking.

The doc still says "Byte-identical to Swift LabMarkerCsvImport.customKey". It is much closer now, and for anything NFC composes it holds. It is not exact: Swift iterates grapheme clusters and this iterates code points, so a letter plus a combining mark with no precomposed form is one Swift Character and two code points here, giving q̇ against q_. Narrow, and probably unreachable for real marker names, but the sentence asserts more than the code does. Worth softening to say where the two agree.

Your paragraph on existing data is the right call and I want to say so explicitly: naming that stored rows keep their old keys, that the original name is not stored so no automatic migration is possible, and that a reimport can therefore leave older readings under the former key, is exactly what a reviewer needs in order to weigh this. It does mean an affected marker's history can split across two keys, which is a real user-visible consequence of a correct fix rather than a reason not to make it.

Sort the authority and I will take it.

@ryanbr

ryanbr commented Oct 2, 2026

Copy link
Copy Markdown
Owner

Rebased onto current main as #2628, with both of your commits unchanged and still authored by you.

The conflict was Tools/parity_twin_map.json: #2617 landed its own refresh of the derived counts while
this sat open. A derived map cannot be hand-merged, so I took main's snapshot and re-ran
--refresh-derived over your rebased source, as a separate metadata commit.

The change itself needed no edits. Android keeping LETTER_NUMBER and OTHER_NUMBER to match Swift's
Character.isNumber is a stored-key divergence, not a cosmetic one, and the extended tables on both
sides make that visible. Your note about uncomposed marks still differing between grapheme clusters and
code points is the right thing to have stated rather than smoothed over.

com.noop.ingest.LabMarkerCsvImportTest passes 25 tests on the rebase; the Swift half rides the
swift-packages leg, since StrandImport cannot build on a Linux host without snapshot-enabled SQLite.

@ryanbr

ryanbr commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Thanks @kavemang. The parity analysis here is right and the oracle is the kind I trust: Swift's Character.isNumber keeps the Unicode Nl and No categories that Kotlin's isLetterOrDigit drops, so Android was underscoring characters Apple preserved, and the same list pinned verbatim on both sides is what proves it rather than reading the two implementations side by side. Moving to code points is also necessary rather than tidiness: forEachIndexed over Char would split the surrogate pair in 𐄇 and produce two underscores. Eighteen checks green.

One premise in the description is wrong, though, and it changes the conclusion.

A lazy migration is available

The description rules out an automatic migration because "the original marker name is not stored with a labMarker row, so the old key cannot safely be mapped back to its Unicode spelling". That is true of a stored row read in isolation, and it is exactly why a schema migration cannot do it. But it does not hold at the point that matters: the importer has the name in hand.

On import of a marker named name, the code already computes customKey(name). It could also compute the legacy key the old algorithm would have produced for that same name, and when rows exist under the legacy key and none under the new one, re-key them. No reverse mapping is required, because the name is the input, not something being recovered from the key.

Why it is worth doing rather than accepting the stranding

The affected spelling is not exotic. eGFR mL/min/1.73m² is one of the most ordinary panels there is, and your own summary uses it as the worked example. So the population is "Android users who have imported eGFR", not "users with unusual marker names". For them this lands as: existing readings stay under custom_egfr_ml_min_1_73m_, every new import goes to custom_egfr_ml_min_1_73m², and the marker silently splits in two with the history on the dead side. A lazy re-key at import closes that on the next import of the same marker, with no migration and no guessing.

Worth noting the divergence already bites across platforms in the same way: an Android row under the underscored key and an Apple row under the ² key are the same marker to a person and two different markers to the app, so a .noopbak moving between platforms already mismatches. That is an argument for the fix, not against it.

Also needed

The branch now conflicts with main in Tools/parity_twin_map.json, which is generated. Regenerate it with --refresh-derived rather than hand-merging the hunks, and the authority counts will reconcile against the two functions this adds.

The honest limitation note in the Kotlin doc comment, that NFC-composable names agree while uncomposed marks may still differ between Swift grapheme clusters and Kotlin code points, is the right thing to have written down. I would keep it.

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