OPENNLP-1894: Add dictionary-based tokenization for Japanese, Korean, and Chinese - #1191
OPENNLP-1894: Add dictionary-based tokenization for Japanese, Korean, and Chinese#1191krickert wants to merge 24 commits into
Conversation
…ENNLP-1895 recorded Restate the map against apache main a864230, cut as 3.0.0-M5 on 2026-07-24. apache#1177 (OPENNLP-1870) merged upstream and moves into the merged box, apache#1190 and apache#1191 are marked ready for review, and OPENNLP-1895 (quantized embedding tables) joins the diagram in its own colour: filed in JIRA with the pull request deliberately held until apache#1165 and apache#1152 move. Statuses now carry the measured GitHub draft flag and how far each head sits behind main, which surfaces three things the old text did not: apache#1182 is a draft again, apache#1167 is based on main rather than on apache#1155 and carries the seam and isBlank commits as copies, and apache#1152 reports conflicts only because its apache-hosted sentencepiece base has diverged from the refreshed head.
65579e9 to
db5f6cc
Compare
…preview-docs, record OPENNLP-1897 The 2026-07-24 map refresh (PR-head rebase record, apache#1190/apache#1191 ready, morfologik-fsa, OPENNLP-1895) was committed directly on kristian-3.x-features and would have been discarded by the next regeneration; preview-docs is the durable home. Also adds OPENNLP-1897-term-vectors to the held-PR section and diagram, and moves the state line to 2026-07-26 (apache main unchanged since the M5 cut).
b02948c to
6716542
Compare
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Thanks for the PR.
-
Where should this live?
LatticeTokenizer,UnigramSegmenter,MecabDictionaryandMorphemedepend on nothing beyondjava.*,Tokenizer,SpanandStringUtil, all of which are inopennlp-api, so they compile there unchanged. That module already holds resource-driven tokenizer implementations of the same kind,WordpieceTokenizerandBertTokenizer, while runtime holds the model-backed ones such asTokenizerMEandBPETokenizer. By that line these four belong in api and onlyMecabDictionaryInstaller, which does network and archive work, belongs in runtime. Worth settling before the rest of the rework, since it decides where the fixes land, and moving public classes later is more disruptive. -
The branch predates #1185, #1196 and #1197, and it adds four new parsers over user-supplied files without picking up any of that hardening.
AbstractModelReader.MAX_ENTRIESwas made public in #1197 with the javadoc "Public so that deserializers outside this package which implement their own binary format can apply the same bound to their count fields."MecabDictionary.loadallocatesnew short[leftSize * rightSize]directly from thematrix.defheader, guarded only againstInteger.MAX_VALUEoverflow, so a header line of46340 46340allocates about 4 GiB. Please bound the matrix dimensions and the lexicon entry count againstMAX_ENTRIES, using the same "exceeds safe limit of N" message style, and add limit tests along the lines ofSymSpellModelSerializerLimitsTest. Rebase on currentmainfirst; that is what brings the constant into scope. -
MecabDictionaryInstaller.extracthas no cap on per-entry size (the tar size field holds 12 octal digits), no cap on total extracted bytes, no entry count cap, and no bound on the gzip expansion ratio.install(URI, Path)reaches all of this over the network, so a crafted archive can fill the disk. It needs an explicit budget. -
install(URI, Path)downloads with no checksum, no connect or read timeout and no size bound, and README-mecab-dictionaries.md points users at it as the way to skip the shell script, so the recommended path is the unverified one. I would rather dropdev/download-mecab-dictionary.shentirely than keep integrity checking in a script we do not ship.DownloadUtilis not reusable as it stands, since everything public there isBaseModel-typed against the dlcdn model index andvalidateModel/calculateSHA512are private, so please extract a genericdownload(URI, Path, String expectedSha512)from it and giveinstallan optional expected digest parameter. That leaves one download path, in Java, with verification on it, and the README shrinks to the two Java steps. -
splitCsvsplits on every comma with no quote handling. A quoted field makesparseIntthrow, which fails the whole dictionary load rather than a single entry. MeCab's own reader supports"..."with""escaping, so the javadoc claim that commas are not representable "matching the plain-text lexicon format" does not look right. Which distributions did you validate end to end, IPADIC only or also UniDic and mecab-ko-dic? -
Two correctness gaps in the loader. Unlisted
matrix.defpairs silently keep cost0, the cheapest possible connection, so a truncated file yields wrong segmentation instead of an error, which is inconsistent with how loudly the rest of the loader fails. AndFiles.newDirectoryStream(directory, "*.csv")has no defined iteration order, while same-surface entries accumulate in encounter order and ties break on first-seen in bothrelaxand the boundary scan, so output varies by filesystem. Sorting the paths before reading fixes the second one.
Smaller things, none of them blocking:
UnigramSegmenter.WordTrieis aMap<Character, WordTrie>, so it boxes aCharacterper character per start position. Commitd887078removed exactly that from the lattice trie.readLinesandUnigramSegmenter.loadread whole files withreadAllBytesand then hold every line as a separate string. For IPADIC'smatrix.defthat is the file content plus 1.7M substrings before a single cost is stored. ABufferedReaderloop would cut peak load memory a lot.leftSizeandrightSizeinvert against intuition, sinceleftSizebounds right context ids andreadLexiconchecksleftId < rightSize. I traced it against MeCab'sconnector.hand it is correct, just transposed from their layout. One comment naming the convention would save the next reader the detour.unk.deftemplates naming a category thatchar.defnever defined are silently ignored. The mapping side got exactly this validation in6716542.char.defcategory flags treat anything other than"1"as false without complaint, andLENGTHis not checked for being non-negative.- The
GZIPInputStreaminextractis never closed, so its inflater is only released at GC. WrappingarchiveStreamin a non-closingFilterInputStreamlets you close the gzip stream and still honour the documented "not closed" contract. - The installer handles neither GNU long name (
L) nor PAX (x) headers and does not verify the tar header checksum. Probably fine for the two dictionaries named in the README, but the javadoc should say which tar dialect is supported. UnigramSegmenter.tokenizecan useSpan.spansToStrings, which is the idiom inAbstractTokenizer.- The "instances are immutable" claim on
MecabDictionaryis not quite true forunknownEntries, whose map and lists are not copied, unlikevaluesandMorpheme. Only package-private accessors touch them, so it is hygiene rather than a bug. - Each
MecabDictionaryretains anint[65536]and aCategory[65536], roughly 0.75 MB, and builds a transientInteger[65536]. Worth a note in the class javadoc, since users are told to load once and share. MecabDictionaryis 907 lines with four nested static classes.DoubleArrayLexiconwith itsBuilder, andCategoryTablewith its builder, would each stand on their own as package-private top level classes.@since 3.0.0on every new type is new for this codebase, which has about three@sincetags in total. I am in favour, but it should be a project-wide decision rather than something that arrives with one PR.- There is no CLI tool and no
TokenizerFactoryor model integration, so this is library-only. Fine as a follow-up, but please record the scope split in JIRA so it is not assumed.
…ctionaries A Viterbi decoder over word and connection costs segments languages written without spaces; the same engine serves Japanese and Korean because the language lives entirely in the dictionary. Unknown text is handled through the dictionary's character categories, and every span stays in original text coordinates. An installer fetches and unpacks a user-chosen dictionary archive at install time: nothing is bundled, no location is built in, and entry names are flattened so no archive path escapes the target directory. (cherry picked from commit a699c8a)
Common-prefix lookup walks a trie built at load time instead of probing substrings per length, terminating on the first missing prefix and allocating nothing per position. (cherry picked from commit e10ce4b)
A Viterbi search maximizing summed word log-probabilities segments Chinese and similar scripts from a plain word-count lexicon, with unlisted characters falling back to single-character words. The user supplies the lexicon and thereby accepts its license; nothing is bundled. (cherry picked from commit bff3f23)
…nigram segmenters, corrected javadoc
…s, add an EUC-JP loading example
…ng download helper
…e their category run, and validate context ids at load
…expressible dictionary values The lattice tokenizer rescanned the same-category run from every position, so a run of L characters cost on the order of L squared category lookups; a 16,000-character katakana run measured around half a second. One right-to-left pass per stretch now fixes every position's category and run end, and the same 16,000-character run tokenizes in about half a millisecond at 31 million characters per second. The character table holds Category instances instead of names, so the per-character path compares by identity with no name-map lookup, and a char.def mapping to an undefined category now fails at load naming the code point. The lexicon trie's children are sorted character arrays found by binary search, so a descent no longer boxes a Character per step. matrix.def loading rejects connection costs outside the 16-bit range instead of silently truncating them, and dimension products beyond the addressable array size fail at the header. The unigram segmenter's unknown-character fallback advances one code point, never one code unit, so an unknown supplementary character is stepped over whole and no span can split its surrogate halves.
…recoded labels The lexicon trie's per-node child lookup, a binary search over the node's fan-out, paid about a dozen comparisons at the root of a real dictionary; the classic base/check double-array makes every transition one array read and one comparison. Characters are recoded into dense labels ordered by descending frequency before the array is built, so the array stays compact although CJK surfaces draw on tens of thousands of distinct characters, and a character the lexicon never uses misses in the recode table before the array is consulted. On the IPADIC harness the prefix walk now matches the fastest previous implementation at 5.6M chars/s with strictly constant-time transitions, and building the array adds about a quarter second to the 392k-entry load.
…er-position lists The Viterbi lattice held one ArrayList per text position plus one fresh candidate list per position, pure allocation churn on long stretches. Nodes ending at a position now chain through their own link field behind a single head reference per position, and candidate gathering fills one scratch list reused across positions, so building the lattice allocates nothing besides the nodes themselves. IPADIC throughput on the 400k-character harness rises from 5.6M to 6.5M characters per second with identical output.
…example Add a lattice tokenizer section to the manual citing LatticeUsageExampleTest.
…lexicon accessors
…e tokenizer overrides
The frequency lexicon was trimmed with String.trim(), which strips only ASCII
control characters and the space. A line starting with an ideographic space
(U+3000), ordinary in hand-edited CJK text files, therefore kept that space as
part of the word and pushed the count field one token to the right, so the load
failed as a malformed count. The lexicon reader now trims with
StringUtil.trimUnicodeWhitespace, matching the White_Space convention the rest
of the tokenizer already scans by, and a test pins the leading U+3000 case.
The mecab reader's line and numeric-field trims move to the same call so one
class does not mix two whitespace judgments; those fields are ASCII in valid
dictionaries, so the behavior there is unchanged.
Both tokenizer views also gain {@inheritdoc} and their null contract, and the
unknown-candidate helper drops a static modifier it did not need.
…fold fixture duplication - Document the private lattice helpers decode, relax, and candidates, and the installer's boundedStream, with the parameter, return, and exception contracts the review expects every method to carry. - Document the WordEntry and Category record components and the double-array builder's findBase and ensureCapacity helpers. - Record on analyze, tokenize, and tokenizePos that a unk.def without a DEFAULT template leaves the lattice disconnected and makes them throw IllegalStateException. - State on readLines that it never returns an empty list, which is what lets the matrix.def header be read before the emptiness check. - Rename the Tokenizer override parameter from s to text in LatticeTokenizer and UnigramSegmenter, so the javadoc names a parameter that exists. - Hoist the matrix.def, char.def, and unk.def file names, the DEFAULT category name, the 0x code point prefix, the .. range separator, and the flag value into named constants in MecabDictionary, and let LatticeTokenizer reach the DEFAULT name through MecabDictionary instead of repeating the literal. - Name the tar block size, header field offsets, and field lengths in the TarGzArchives test helper instead of writing 512, 124, and 148 inline. - Replace the boolean[1] capture in candidates with a check that the candidate list is still empty, which is the same signal without the array. - Track the best boundary total in decode instead of recomputing the incumbent's connection cost on every comparison. - Drop the categories map field from MecabDictionary, which nothing read once the constructor resolved the DEFAULT category out of it. - Match the char.def code point prefix once, case insensitively, rather than testing 0x and 0X separately, and cut the range at the separator's own length. - Trim the matrix.def header before parsing it and report an empty first line as an empty matrix.def, since readLines never yields the empty list the previous check was looking for. - Split the omnibus malformed-dictionary test into named cases that pin the messages for a missing definition file, a char.def without DEFAULT, a lexicon with no entries, and an empty matrix.def. - Parameterize the malformed char.def cases and the malformed unigram lexicon cases, which were repeated assertThrows calls over one fixture shape. - Add a Morpheme test pinning the null and empty argument rejections and the defensive copy of the feature list. - Extend the invalid-argument tests to the entry points that were uncovered: MecabDictionary.load with a null directory or charset, the installer's null target, UnigramSegmenter's path and stream overloads, and both tokenize methods of each tokenizer. - Fold the repeated Files.write fixture calls into one write helper and hoist the shared lexicon, matrix, char.def, and unk.def fixture text into constants. - Correct dev/README-mecab-dictionaries.md to say that dicrc is the configuration file the distributions ship alongside the csv and def files a MecabDictionary reads, rather than implying the dictionary reads dicrc itself.
6716542 to
7a98076
Compare
Place LatticeTokenizer, UnigramSegmenter, MecabDictionary, and Morpheme with the other resource-driven tokenizers. Keep MecabDictionaryInstaller in runtime. Lift MAX_ENTRIES into ResourceLimits so api loaders can share the bound, and reject incomplete or oversized matrix.def payloads.
3–4. Extract budgets and verified download path not done yet. Planning one Java download+digest path shared with #1190, then drop the shell helpers.
Smaller items (WordTrie boxing, BufferedReader load, tar dialect javadoc, etc.) still queued. |
Bound per-entry size, total bytes, entry count, and gzip expansion during install. Fail loud on undefined unk.def categories; accept quoted CSV fields. Pin with installer and load tests; note the limits in the manual.
|
Follow-up in b79cc31. Extract budgets: per-entry size, total extracted bytes, entry count, and gzip expansion ratio. Fail loud with tests that exercise the ceilings through a package-private extract overload. Also in that tip: MeCab-quoted CSV fields, Verified download path and dropping the shell helpers still open; same shared design as #1190. |
|
Download path reworked in b81f348; the shell helper is gone.
README and the tokenizer manual chapter describe the Java path now. #1190 carries the same |
…t startup Keep the 512 MiB download and tar-entry ceilings and the 2 GiB total extraction ceiling as defaults, but read them from the system properties opennlp.download.max.bytes, opennlp.install.max.entry.bytes, and opennlp.install.max.total.bytes at class load, so dictionaries larger than the defaults, such as UniDic, install without a code change. Absent or invalid values fall back to the defaults. Tests pin the parsing and the defaults; the README and manual document the overrides.
|
Extraction and download budgets are now defaults with startup overrides, in e2a437a. The ceilings keep their values (512 MiB per download and tar entry, 2 GiB total extracted, 10k entries, 100x gzip expansion), and the byte ceilings read system properties at class load: Tests pin the parser (override, absent, invalid, non-positive) and the default values; the README and manual chapter document the overrides with a UniDic-sized example. |
…ntions Link MeCab, IPADIC, UniDic, mecab-ko-dic, and the POSIX ustar format where the javadoc names them, and write MeCab in its own casing across the prose. Document the entryCount parameter and its limit rejection on MecabDictionary.readLexicon. Replace three literal kanji in LatticeTokenizerTest with Unicode escapes, matching the file's ASCII-only convention. Convert DownloadUtilFileTest's invalid-limit loop to a parameterized test so a failing value is identifiable.
…ng PR Both PRs carry byte-identical copies; this folds the sibling's test javadoc into this side's parameterized fixtures so the copies match again.
…tionaries Downloads the two pinned distributions with digest verification, installs them, loads them, and checks segmentation. Opt-in via -Dopennlp.download.remote=true like the catalog itself; CI never touches the network. At the previous tip the mecab-ko-dic case failed twice: the matrix cell bound rejected its genuine 3822 x 2693 matrix, and the flattened user-dic templates broke the lexicon parse.
The cell-count check reused ResourceLimits.MAX_ENTRIES, sized for record-shaped entries, and rejected mecab-ko-dic 2.1.1, whose genuine matrix declares 3822 x 2693 = 10,292,646 cells of two bytes each. A new ResourceLimits.MAX_MATRIX_CELLS bounds two-dimensional cost tables at 2^27 cells (256 MiB of shorts), overridable at startup, and still refuses the roughly 4 GiB allocation a crafted 46340 46340 header would force. Unit tests pin the new bound on both sides with the ko-dic dimensions as the accepted case.
The installer flattened every csv and def file in the archive into the target directory, so mecab-ko-dic's nested user-dic templates, whose numeric fields are empty because they are mecab-dict-index input, landed beside the real lexicon and failed the load. On a case-insensitive file system a template could even overwrite a real lexicon file of the same base name. Entries deeper than one leading directory are now skipped.
|
All items from the 2026-08-04 review are in, and the branch now carries an opt-in end-to-end test that validated the whole path against the real distributions. Per item: api move (2630355), ResourceLimits bounds with limit tests (2630355), extract budgets (b79cc31, startup overrides e2a437a), verified Java-only download with the shell script removed (b81f348), quoted-CSV fields (b79cc31), fail-loud incomplete matrix and sorted lexicon order (b79cc31), unboxed trie descent, buffered reads, the connector.h transposition comment, and unk.def category validation. On which distributions are validated end to end: MecabCatalogEndToEndTest (8e14a69, gated on -Dopennlp.download.remote=true like the catalog itself) downloads both pinned distributions, installs, loads, and segments. IPADIC 2.7.0 reproduces the reference segmentation of the classic sumomo sentence; mecab-ko-dic 2.1.1 loads and segments Korean with clean spans. UniDic remains format-level: same reader, covered by the UniDic-shaped char.def range test, but no end-to-end claim and no catalog entry, since current UniDic archives exceed the default extraction budgets (the overrides exist for it). Writing that test caught two real bugs at the previous tip, both fixed:
Ready for another look. |
| System.getProperty("OPENNLP_DOWNLOAD_MODEL_PATH", "models/ud-models-1.3/"); | ||
| private static final String OPENNLP_DOWNLOAD_HOME = "OPENNLP_DOWNLOAD_HOME"; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Is it possible to break out the changes to model download into its own PR?
| } | ||
| final Path file = targetDirectory.resolve(baseName); | ||
| try (InputStream entry = boundedStream(tar, size)) { | ||
| Files.copy(entry, file, StandardCopyOption.REPLACE_EXISTING); |
There was a problem hiding this comment.
The file is downloaded and if there is an existing file, the existing file is overwritten by the new file. Would it be helpful to extract to a temp directory, verify the files, and them move them? I'm thinking of cases where the file system could be left in a bad state with a mix of old files, new files, or files extracted incorrectly.
Or perhaps it's simpler to remove REPLACE_EXISTING so we don't have to worry about getting into a bad state.
| * {@code archive} is not an absolute URI, or {@code archive} is not a | ||
| * {@code file:} URI. | ||
| */ | ||
| public static int install(URI archive, Path targetDirectory) throws IOException { |
There was a problem hiding this comment.
Could the Javadoc say this overload treats the local archive as trusted caller input and performs no cryptographic integrity verification?
Since this overload intentionally permits an unverified local archive, the extractor should still validate the standard tar header checksum before trusting header metadata. It would make this path safer and more format-compliant and hopefully prevent a security patch down the road.
|
Correct me where needed here --- For a long time OpenNLP didn't provide the ability to download models - users had to either download a model from SourceForge or train their own and then point the code at their model to use it. A few years ago after we got the ok to release models under the Apache license and got the models hosted on the ASF, we added the ability to download those models with the goal of lowering the barrier to entry for new users. What concerns me about this change is the files can be downloaded from anywhere and it could enlarge OpenNLP's surface for security issues. Now I'm not saying the current implementation is perfect and I'd have to look to see where it stands now, but I do pause when I see the ability to download files from non-ASF servers. (I'm not saying it doesn't need improvements, too.) Maybe I'm over-thinking it and too paranoid so please tell me if I am. I don't want us to get into a position where we have more security issues coming in but still the same size team to deal with those additional issues. So I guess my questions are:
|
|
So we added UTF support, naturally multi-language goes next. The user base won't land until those features are added. It'll help gain wider support. So there's a lot of defenseiveness to the code that I have added - but it's on a new PR. Including this, we now have three copies of this machinery. I've created a branch for a resource-installer and it's by far the strongest. My latest rounds covers things #1191's copy doesn't:
So this is best met by converging on one shared, hardened installer rather than one per feature PR. So here it is #1211 but just in draft form Earlier in the thread I had told rzo1 that whichever of #1190/#1191 merges second rebases onto the other. So we'll converge on them fairly soon I kept it as draft becaise I didn't heavily test it yet. Feel free to edit / critique though. If you don't mind, we'll keep this as-is and just make sure we |
3 participants
Adds Viterbi lattice segmentation over user-supplied MeCab-format dictionaries, covering the Japanese (IPADIC) and Korean (mecab-ko-dic) dictionary layouts, plus a frequency-driven unigram segmenter for Chinese.
The lexicon is held in a double-array trie with frequency-recoded labels, and lattice nodes chain intrusively so the hot path allocates no per-position lists. Measured on the real IPADIC dictionary: about 5 million characters per second single-threaded, with 392k entries loading in under one second. Segmentation of the standard connection-cost-sensitive test sentences matches the reference implementation's output on IPADIC.
Dictionaries are always user-supplied and never bundled; test fixtures are project-authored miniature lexicons written by the tests themselves, and
dev/README-mecab-dictionaries.mdplus a checksum-verifying download script document dictionary acquisition. The tokenizer manual gains a section whose example is asserted byLatticeUsageExampleTest.