Skip to content

OPENNLP-1894: Harden CJK dictionary loading - #1265

Merged
rzo1 merged 4 commits into
apache:mainfrom
ai-pipestream:OPENNLP-1894-lattice-followup
Sep 4, 2026
Merged

OPENNLP-1894: Harden CJK dictionary loading#1265
rzo1 merged 4 commits into
apache:mainfrom
ai-pipestream:OPENNLP-1894-lattice-followup

Conversation

@krickert

@krickert krickert commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Uses all categories listed by a char.def mapping while keeping the first category as primary.
  • Uses the mapped category template for zero-length, ungrouped unknown characters.
  • Rejects duplicate matrix entries, duplicate category definitions, empty surfaces, invalid word costs, and malformed lexicon encoding.
  • Builds MeCab and unigram lexicon tries iteratively so long entries do not overflow the thread stack.
  • Documents multi-category mappings in the tokenizer manual.

This is a follow-up to #1191.

Validation

Before the implementation change, the new tests produced eight assertion failures and a StackOverflowError for a 20,000-character surface.

  • ./mvnw -q -pl opennlp-core/opennlp-runtime -am -Dopennlp.forkCount=1 -Drat.skip=true test
  • ./mvnw -q -pl opennlp-docs -am -Dopennlp.forkCount=1 -Drat.skip=true -DskipTests package

Add tests for zero-length categories, multiple character categories, duplicate definitions, invalid word costs, malformed input encoding, and long lexicon entries.

Red evidence on current main: eight assertions failed and a 20,000-character surface raised StackOverflowError.
Honor all categories on char.def mappings, use the primary category for unknown-word settings, reject duplicate definitions and invalid word costs, and build lexicon tries without recursive calls.
@rzo1

rzo1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Here are some load bearing comments ;-)

I checked the DoubleArrayLexicon.Builder#insert recursion-to-iteration rewrite carefully, since values[] is indexed by sorted-surface position and the valueIndex numbering has to match. The reversed label loop plus LIFO push does preserve pre-order DFS in sorted order, and the backward childStart scan is equivalent to the forward start/end walk. That rewrite is correct.

Blocking

1. LatticeTokenizer#computeCategoryRunsIdentityHashMap and boxing on the tokenizer hot path.

final Map<Category, Integer> runEndByCategory = new IdentityHashMap<>();

computeCategoryRuns runs once per whitespace-delimited stretch of every analyze/tokenize call, and CategoryAssignment#runEnd does a getOrDefault + put with Integer boxing for every category of every character. The code it replaces was a single int next with zero allocation. Assign each Category a dense int id at load time and use an int[] indexed by that id, reset per stretch. CategoryAssignment#contains is also a linear scan inside that per-character loop — a bitmask over the same dense ids removes it too.

2. CategoryAssignment#runEnd — the run-extension rule diverges from MeCab.

Taking Math.max of each category's own contiguous chain means every character in the run must share one specific category with its neighbour chain. MeCab's seekToOtherType tests isKindOf — a bitmask intersection against the starting character's mask — so different characters may continue the run via different categories.

Counter-example: masks A={X,Y}, B={X}, C={Y}. This code stops the run at C (X-chain ends at B, Y-chain ends at A, max = 2); MeCab continues through C, since both intersect {X,Y}.

Please confirm against the reference implementation and cite it, or align. The manual text ("all listed categories can extend the character run") describes neither precisely.

3. opennlp-docs/src/docbkx/tokenizer.xml documents one of six changes.

The manual gains a note on multi-category mappings only. Not documented: empty lexicon surfaces now fail instead of being skipped, duplicate matrix.def entries and duplicate char.def categories are rejected, word costs must fit signed 16-bit, malformed lexicon encoding is rejected, and a category's own unk.def template is preferred over DEFAULT. The empty-surface change in readLexicon (continue -> throw) in particular can make a previously-loading IPADic/UniDic build fail. Please document all of them, and say so in the PR description.

4. CategoryTable.Builder#build — the pre-pass over mappings changes behavior with no comment.

Resolving every recorded Mapping before resolving bmp/winners means an undefined category name is now rejected even on a mapping line completely shadowed by a later line — previously it was only reported if it survived into the table. That is probably the intent, but nothing says so, and Mapping.from then names a code point the mapping does not actually own, so the error message points at the wrong character. Add the comment, and either use a code point the mapping still covers or say "declared at" rather than implying assignment.

Minor

  • CategoryAssignment's constructor Javadoc says "Must not be empty" and does not check. primary() would throw ArrayIndexOutOfBoundsException, not the project's IllegalArgumentException. The fields.length < 2 guard in readCharacterDefinition makes it unreachable today; either validate or drop the unenforced claim.
  • The IdentityHashMap<String[], CategoryAssignment> cache works only because Builder#map stores the same String[] instance into bmp[c] for the whole range and into names. That invariant is load-bearing and invisible — one line of comment on Builder#map, please.
  • runEnd mutates the caller's map but is named as a pure accessor. extendRuns or similar.
  • dictionary.categoriesOf(codePoint).primary() in LatticeTokenizer#candidates where dictionary.categoryOf(codePoint) exists and does exactly that. After this PR categoryOf has no production caller left; either use it here or delete it and update the tests that call it.
  • new CategoryAssignment(new Category[] {categories.get(DEFAULT_CATEGORY)}) puts an unchecked get result into the array. The load path guarantees DEFAULT exists, so make that explicit rather than relying on it two classes away.
  • MecabDictionary class Javadoc: "Each instance keeps about 0.75 MB of category tables keyed by the 16-bit code-unit space, so load once and share" became "Load once and share an instance." The figure is still accurate after this change and is the reason for the advice. Restore it.
  • WordTrieBuilder.built is a mutable field on the builder that is never cleared, so the whole immutable trie stays reachable from the mutable tree until both are collected. Null it out after reading, or return from a local map.
  • The explicit stack in WordTrieBuilder#build allocates two BuildStep records per node. Acceptable for stack safety, but please note it in the method Javadoc so nobody "optimizes" it back to recursion.
  • (byte) 0xC3 appears bare in LatticeTokenizerTest and UnigramSegmenterTest. OPENNLP-1893: Correct Hunspell parsing and analysis #1266 introduces TRUNCATED_UTF8_LEAD_BYTE for the same byte. Name it here too, and consider a shared test helper.
  • testRejectsMalformedDictionaryEncoding and testRejectsMalformedLexiconEncoding assert only IOException.class while every other new test pins the exact message. A missing file would pass these. Assert the message.
  • DoubleArrayLexicon.Builder Javadoc: "The recursive sorted-range builder: each call places one node's children ... A moving watermark keeps the free-slot search near-linear over real lexicons" was shortened to "Builds the trie from sorted surface ranges." The watermark sentence still describes findBase and is the only explanation of why it is near-linear. Restore it, adjusted for the iterative walk.
  • Same "declared"/"every"->"all" style rewording as OPENNLP-1893: Correct Hunspell parsing and analysis #1266 in LatticeTokenizer and UnigramSegmenter class Javadoc, including dropping "This is the segmentation approach behind Japanese and Korean morphological analysis" and "it is lighter than the LatticeTokenizer". Both were orientation for a reader picking between the two classes. Please revert.

Credit where due: folding CategoryTable.CHARACTER_DEFINITION_FILE into MecabDictionary.CHAR_DEF and adding Locale.ROOT to the one String.format are exactly right.

Process

  • This PR reuses a closed issue's key. OPENNLP-1894 shipped as OPENNLP-1894: Add dictionary-based tokenization for Japanese, Korean, and Chinese #1191 (93136f6e). This follow-up carries new user-visible behavior — new validation, changed unknown-word fallback — and needs its own JIRA issue so the release notes describe it and OPENNLP-1894 stays closed.
  • Title. "Harden CJK dictionary loading" is implementation vocabulary. Suggest something closer to release-notes language, e.g. "Support multi-category char.def mappings and validate MeCab dictionary input".
  • The Javadoc rewording and shortening pre-existing comments without a functional reason also show up in OPENNLP-1893: Correct Hunspell parsing and analysis #1266 and your other open PRs. Please apply this to all open PRs likewise: new Javadoc for new behavior, untouched Javadoc for untouched behavior.
  • No model-affecting changes here, so no eval build is needed before merge.

Red evidence: LatticeTokenizerTest reported three failures for the MeCab category limit and mapping declaration diagnostics.
Red evidence: testCategoryOverlapCanConnectRun returned 2 tokens instead of 1, and testRejectsCharacterCategoryLengthAboveMecabLimit accepted LENGTH 16.
@rzo1
rzo1 merged commit e0f669c into apache:main Sep 4, 2026
10 checks passed
krickert added a commit to ai-pipestream/opennlp that referenced this pull request Sep 5, 2026
Record the merged apache#1190, apache#1191, apache#1265, and apache#1211 (now supplied by main), the
slimmed subword-API-only apache#1165 with its add-ons PR apache#178, the open apache#1266
hunspell follow-up, refreshed PR heads, and the regenerated uber and helper
tips.
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