Repository navigation
feat: import a DSL dictionary, resumably - #29
Merged
Merged
Conversation
PR Summary by QodoAdd a streaming, resumable ABBYY DSL dictionary importer
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
The last layer of the BKRS importer: a streaming, resumable pipeline, and `hanreader-dictgen dsl` to drive it. The real 350 MB set imports end to end — 3,434,224 cards to 3,442,623 entries with zero diagnostics — and an import interrupted four times with `kill -9` resumes to a container **identical** to one never interrupted. **Resumability is a requirement, not a refinement**, because on iOS the app will be suspended or killed part way through a minute-long import. Three things make it work: *The checkpoint and the rows it describes are one transaction.* Written separately, a crash between them leaves the checkpoint behind the rows and the next run writes those rows again. *Every entry carries its own card's offset.* This is the bug the gate was for. Stamping a whole batch with the offset of the card that happened to fill it means a resume deletes the batch and re-reads only its last card: measured over four interruptions of a 574,709-entry import, **14,997 entries vanished and it reported success**. The stored card count likewise excludes the checkpoint card, since a resume re-reads it — otherwise the total grew by one per interruption. *WAL, not `journal_mode = OFF`.* Compiling CC-CEDICT runs with durability off because the output is a build artefact regenerated in seconds. A reader's own import is not that, and under `journal_mode = OFF` a process killed mid-transaction can leave the file corrupt — which cannot be resumed. The two requirements are mutually exclusive and resumability is the one that was asked for, so every committed batch survives the process dying. A power cut would need `FULL`; a dictionary import is not worth an fsync per batch, and the job resumes from the last batch that reached disk. The source is identified by name, size and modification time, never by path: on iOS the container directory's UUID changes across installs and restores, so a stored path is stale by the time it matters. A job from a different file or a different parser version is discarded rather than continued — no schema migration reconciles output from two parsers, and half a dictionary from each is worse than either. **Memory is bounded by the batch.** Measured peak over the full set is 55.9 MB against a 60 MB ceiling, with the card-reading phase steady at 25–30 MB. Getting there meant finding two things that only appear at scale, neither of them in the import loop: - `LexiconBuilder.characterReadings` took every single-character entry as an array. Those are the densest cards in a dictionary — 38,618 of them carrying 27 MB of encoded senses, 一 and 打 with 57 each — and decoding them all at once took peak memory to **857 MB**. It is now a collector fed one entry at a time through a cursor, keeping five small values per candidate reading. - `DictionaryContainer.lexicon()` in the `--verbose` summary fetched all 1,806,893 lexemes to print one number, which cost **450 MB** and looked for a while like the import itself. Added `lexemeCount()`. **The lexicon is capped at four characters for a dictionary like this one.** At the general cap of six, BKRS contributes 2,804,554 lexemes — a segmentation lexicon in which almost every run of Han characters is a word, which is the condition under which maximum-matching produces confident nonsense, and 176 MB of index for it. Four is where the measured length distribution says to stop: 1→33,372, 2→427,842, 3→455,438, 4→959,810, then 5→585,617 and 6→439,262 of increasingly phrase-like entries. Character readings and the syllable inventory are derived at the end from what is on disk rather than accumulated while writing: both need to see the whole dictionary, and accumulated state is state a crash destroys and a resume cannot rebuild. The source index is dropped once the import finishes — 53 MB the reader would carry for a resume that can no longer happen — and recreated if one ever starts again. **Nothing about the dictionary is hardcoded.** Name, languages and version come out of the file's own directives; the predecessor took the name from a constant in its source, so it mislabelled any other DSL dictionary. No licence is claimed: BKRS states none, and inventing one would be worse than admitting it, because the Acknowledgements screen renders whatever is here verbatim. ### Two gates are met and two are not | gate | measured | |---|---| | `kill -9` mid-import resumes clean | **passes** — identical after 4 kills | | < 60 MB RSS over baseline | **passes** — 55.9 MB peak | | 350 MB set in < 90 s | **205.5 s** | | on-disk size | **868 MB** | Both misses have one cause, and it is measurable: the `entry` table is 625 MB of the 868, and only **40.7%** of a sense payload is text — the rest is JSON syntax and key names. The `summary` column adds a further ~160 MB duplicating a gloss that is already inside the payload, and `UsageExample` stores its text twice, as `raw` and again as the split halves. A compact binary `SensePayloadCodec`, which the architecture plan already calls for, addresses the size and most of the time at once; it changes the container format and the bundled CC-CEDICT dictionary with it, so it is its own change rather than a rider on this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven correctness fixes raised in review, plus one the review's own test exposed, all in the resume path. **Resuming at a file boundary read the byte-order mark as content.** An end-of-file checkpoint stores byte offset zero to mean "from the beginning of the next volume", and handing that straight back to the byte reader starts it *at* the mark — which then arrives as the first character of the first headword. Every multi-file resume at a boundary corrupted one headword, and nothing would ever find that word again. Zero now means nil to the reader, which is where the mark is skipped. **A card assembled across a file seam lost its headword.** The carried headwords were cleared before the batch containing that card committed, so a checkpoint whose frontier was the seam card restarted the next volume without it: the card's entries were deleted on resume and its article could not rebuild them. They are now cleared only when a real card offset is committed. **Only the first volume's identity was checked.** A changed third volume with an unchanged first one resumed onto entries from the old version — a dictionary half from each, reporting success. The same hole covered the syllable inventory, which decides how a reading is split and therefore what the cross-dictionary merge key is, so re-running with a different `--bases` mixed readings parsed two ways. Both are now in one fingerprint covering every volume's name, size and modification time and the sorted bases. Spelled out rather than hashed: `Hasher` is seeded per process, so `hashValue` is not comparable across runs at all, and the bug would have been invisible and intermittent. **Discovery's own complaints were dropped on the floor.** A volume an `#INCLUDE` names but that is not there, or a set longer than discovery will follow, produced a dictionary missing part of itself and a summary that said nothing. They are reported separately from the in-file diagnostics, because a reader has to be told about these whatever the import otherwise achieved. **Diagnostic counts drifted across an interruption**, in both directions. A frontier card's parse diagnostics were counted and then counted again on replay; the reader's own were added only at end of file, so a resume seeking past the lines that caused them lost them. The split is now by whether a resume will re-read the line: the reader's complaints describe lines it has already gone past, so they are settled and belong in the checkpoint; the frontier card's do not. That last one is where the review's test found a bug of mine rather than the reverse. My first fix snapshotted the totals before counting the reader's diagnostics, which put an orphaned article line on the wrong side of the boundary — and the test for it was flaky, passing alone and failing under the full suite, because whether it bit depended on where the interruption landed. Ordering the two counts correctly makes it deterministic. **And `verify` rejected any dictionary that was not CC-CEDICT.** Its content checks are CC-CEDICT's own words and its entry and lexeme floors are CC-CEDICT's size; a container compiled from someone's DSL dictionary is a valid container that does not contain 中国 with the reading `zhong1 guo2`. Those checks now apply to the format they describe, and the structural ones to everything. It also fetched the whole lexicon to take a maximum, which on a BKRS container is 1,806,893 rows; `longestLexeme()` asks the database. Verified on a two-volume set killed three times, the third kill landing inside the second volume: 574,708 entries, 572,236 cards, 345,038 lexemes, zero diagnostics and zero byte-order-mark-corrupted headwords — row for row identical to an uninterrupted import. Each fix was mutation-tested; four of the first six survived every test I had written for them, because a completed import clears its job and a test that runs one to the end and then runs it again is testing a fresh import and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
widgetii
force-pushed
the
feat/m6-import-pipeline
branch
from
October 13, 2026 16:24
5e7318c to
aabb4b5
Compare
widgetii
enabled auto-merge (squash)
October 13, 2026 16:32
Three resume tests passed locally and failed on CI, all with `resumed: false` — the import had finished before the test could cancel it, so there was nothing interrupted and nothing to resume. Waiting for a progress value and then cancelling is not enough on its own: the importer carries on in the meantime, and a twenty-card import takes less time than an async test needs to react. The callback now signals the test and then *blocks*, so the cancellation is in place before the importer can take another step. Progress is reported on the first batch to commit, so afterwards exactly one batch is in the container and the rest of the file is not. The racy shape is gone from all four interruption tests rather than patched in the three that happened to fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`shouldContinue` stops at the next card boundary and returns a summary with `completed == false`: the work so far is committed, the job row stays, and nothing has thrown. That is what a Cancel button wants. Cancelling the task still throws `CancellationError`, for a caller that is tearing down rather than pausing. It also replaces the only mechanism the resume tests had for interrupting an import, and the two earlier attempts at that were both wrong in instructive ways. Sleeping and then cancelling **raced** — a twenty-card import finishes before an async test can react, which passed locally and failed on CI with `resumed: false`. Signalling the test and blocking the callback until it cancelled was deterministic but **blocked a thread of the cooperative pool**, and with Swift Testing running these tests in parallel the pool deadlocked: CI sat in that one suite for twenty minutes before I cancelled it. Asking the importer to stop needs no concurrency at all. Progress is reported when a batch commits and `shouldContinue` is asked at the next card, so the test stops the import with exactly one batch written and the rest of the file unread — synchronously, with no threads and nothing to race. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two performance gates M6 missed, closed by measurement rather than by
guessing where the bytes were. Over the full 350 MB BKRS set:
| | before | after |
|---|---|---|
| on disk | 868 MB | **493 MB** |
| elapsed | 206 s | **74.7 s** |
The bundled CC-CEDICT dictionary goes from 32 MB to **24 MB** with it, and
3,434,224 cards still produce 3,442,623 entries and zero diagnostics.
**A packed binary codec for sense payloads.** The `entry` table was 625 MB of
the 868, its payloads 327 MB of that, and **only 40.7% of a payload was text**
— the rest `{"gloss":{"text":…},"level":0,"kind":…}` written out once per
sense, 3,691,468 times. The packed form is 168 MB, within 4% of the raw text
it carries, which is about as close as an uncompressed encoding gets. Three
ideas earn their complexity: a presence byte, since the common sense is a
gloss and a level and nothing else; varints, since every integer here is a
level, a parent, an offset or a count; and storing a usage example once, as
its raw text plus the two boundaries inside it, where before `raw` and the
heuristic split of it were both written out in full and cost the same again.
`SensePayloadCodec` is a seam, not a single implementation, because this
choice will be made again — a block-compressing codec is the obvious next tier
and the plan already names it. A container records which codec wrote it, so
adding one does not invalidate the dictionaries already on disk, and JSON
still reads. An unrecognised identifier is an error rather than a fallback,
because misreading a payload produces plausible nonsense.
**The `summary` column is gone.** The denormalised first gloss, written since
v1 and **read by nothing** — 160 MB of Cyrillic prose already sitting in the
payload beside it. It was there so a result list could render without decoding
a payload per row; with the packed codec that decode is microseconds for the
ten rows a list shows.
**The lookup indexes are built once, at the end.** Maintained while rows
arrive, `entry_on_simplified` is a random-order insert into an 84 MB B-tree
three and a half million times. Built in one pass over rows that are all
already there it is also 63 MB smaller, because it carries none of the page
splits random insertion leaves behind. That one change is most of the
130-second difference.
It costs memory, and that is the honest shape of it: `CREATE INDEX` is a
single atomic statement, so the whole new index is dirty at once — about
92 MB, which no `cache_size`, `temp_store`, `threads`, `soft_heap_limit` or
`hard_heap_limit` pragma reduces. All five were tried. So the two budgets
trade against each other and the choice is explicit:
| `Indexing` | time | peak memory | size |
|---|---|---|---|
| `.atTheEnd` | **75 s** | 134 MB | **493 MB** |
| `.whileWriting` | 188 s | **53 MB** | 556 MB |
`.atTheEnd` is the default because compiling a DSL set is a desktop operation
by design — a Mac compiles it once and a phone imports the compiled container
— so the 60 MB ceiling, which exists for iOS, is not the binding constraint
on this path. `.whileWriting` exists for the day something runs an import
where it is, and both numbers are on the type so the trade is not rediscovered.
What remains above the plan's 210–240 MB estimate is no longer payload. Of
493 MB: 168 MB of senses that are 96% text, 84 MB of headword index, 104 MB of
segmentation lexicon, ~75 MB of headwords and readings, and the row overhead
of three and a half million rows. The estimate was computed from the
predecessor's flattened definitions and did not account for the lexicon, the
index, or storing sense structure and examples at all. Compressing the payload
is the next real lever and the seam is there for it.
Verified by round-tripping all 523 senses of the committed real-data slice,
field for field; by a two-volume import killed three times, the third landing
inside the second volume, coming back row-for-row identical to an
uninterrupted one; and by truncating a payload at every length from zero,
which throws rather than trapping at each.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
The last layer of the BKRS importer: a streaming, resumable pipeline, plus
hanreader-dictgen dslto drive it. The real 350 MB set imports end to end — 3,434,224 cards to 3,442,623 entries, zero diagnostics — and an import interrupted four times withkill -9resumes to a container identical to one never interrupted.Resumability is a requirement, not a refinement
On iOS the app will be suspended or killed part way through a minute-long import. Three things make it work:
The checkpoint and the rows it describes are one transaction. Written separately, a crash between them leaves the checkpoint behind the rows and the next run writes those rows again.
Every entry carries its own card's offset. This is the bug the gate existed to find. Stamping a whole batch with the offset of the card that happened to fill it means a resume deletes the batch and re-reads only its last card — measured over four interruptions of a 574,709-entry import, 14,997 entries vanished and it reported success:
With per-entry offsets, the same test gives
entries=574709,cards=572237,lexemes=345038— every number equal to the reference, and a row-by-row comparison of all 574,709 rows identical. The stored card count also excludes the checkpoint card, since a resume re-reads it; otherwise the total grew by one per interruption.WAL, not
journal_mode = OFF. Compiling CC-CEDICT runs with durability off, which is safe because the output is a build artefact regenerated in seconds. A reader's own import is not that, and underjournal_mode = OFFa process killed mid-transaction can leave the file corrupt — and a corrupt file cannot be resumed. The two requirements are mutually exclusive and resumability is the one that was asked for. A power cut would needFULL; a dictionary import is not worth an fsync per batch, and the job resumes from the last batch that reached disk.The source is identified by name, size and modification time, never by path: on iOS the container directory's UUID changes across installs and restores, so a stored path is stale by the time it matters — the same mistake the predecessor made with absolute audio paths. A job from a different file or a different parser version is discarded rather than continued; no schema migration reconciles output from two parsers, and half a dictionary from each is worse than either.
Memory is bounded by the batch
Peak 55.9 MB over the full set against a 60 MB ceiling, with the card-reading phase steady at 25–30 MB (sampled every 6 s through a whole run). Getting there meant finding two things that only appear at scale, neither of them in the import loop:
LexiconBuilder.characterReadingstook every single-character entry as an array. Those are the densest cards in a dictionary — 38,618 of them carrying 27 MB of encoded senses, 一 and 打 with 57 senses each — and decoding them all at once took peak memory to 857 MB. It is now a collector fed one entry at a time through a cursor, keeping five small values per candidate reading.DictionaryContainer.lexicon()in the--verbosesummary fetched all 1,806,893 lexemes to print one number: 450 MB, which for a while looked like the import itself had blown its ceiling. AddedlexemeCount().The lexicon is capped at four characters for a dictionary like this one
At the general cap of six, BKRS contributes 2,804,554 lexemes — a segmentation lexicon in which almost every run of Han characters is a word, which is precisely the condition under which maximum-matching produces confident nonsense, and 176 MB of index for the privilege. Four is where the measured length distribution says to stop: 1→33,372, 2→427,842, 3→455,438, 4→959,810, then 5→585,617 and 6→439,262 of increasingly phrase-like entries.
Character readings and the syllable inventory are derived at the end from what is on disk rather than accumulated while writing. Both need to see the whole dictionary — a character's readings are ranked against each other — and accumulated state is state a crash destroys and a resume cannot rebuild. The source index is dropped once the import finishes (53 MB the reader would carry for a resume that can no longer happen) and recreated if one ever starts again.
Nothing about the dictionary is hardcoded
Name, languages and version come from the file's own directives — the predecessor took the name from a constant in its source, so it mislabelled any other DSL dictionary. The volume suffix is stripped, because
( 1 / 3 )belongs to the file and the three volumes compile into one container.No licence is claimed. BKRS states none, and inventing one would be worse than admitting it, because the Acknowledgements screen renders whatever is here verbatim. The attribution says the file came from the reader, that HanReader does not redistribute it, and what changes the import made.
Two gates are met and two are not
kill -9mid-import resumes cleanBoth misses have one cause, and it is measured rather than guessed. The
entrytable is 625 MB of the 868, and of a sense payload only 40.7% is text — the rest is JSON syntax and key names. On top of that thesummarycolumn duplicates a gloss already inside the payload (~160 MB), andUsageExamplestores its text twice, asrawand again as the split halves.A compact binary
SensePayloadCodec— whichDocs/and the architecture plan already call for, with block compression as a later tier behind the same seam — addresses the size and most of the time together. It changes the container format and the bundled CC-CEDICT dictionary with it, so it belongs in its own change rather than as a rider on this one.A
VACUUMis not the answer, for the record: it recovers 66 MB of 868 and leavesentryat 625 MB. The bytes are real.Validation
13 new tests, including a deterministic interruption: the first progress value means the first batch has committed, so waiting for it and then cancelling interrupts at a batch boundary with work on both sides — no sleep, no race. (A sleep did race: on a quiet machine the whole 400-card import finished inside 40 ms.)
Full suite green, lint clean, no warnings under
-warnings-as-errors.🤖 Generated with Claude Code