OPENNLP-1893: Support Hunspell affix dictionaries for stemming - #1190
Conversation
mawiesne
added
java
…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.
8722725 to
65a1c31
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).
3a3f977 to
6f2d9d8
Compare
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Thanks for the PR.
-
The PR description says "unsupported dictionary features fail loud at load time rather than degrading silently", but
parseAffixends indefault: i++; break;, which skips every directive it does not know without a word. The class javadoc and README say the opposite of the description, that rules using unsupported features "simply do not fire, so unsupported analyses are missed rather than invented". Only an unknownFLAGmode really fails loud. The gap matters because the silently skipped set includesICONV,OCONVandCOMPLEXPREFIXES, which change results rather than only reducing them, so a dictionary relying on them produces wrong stems with no signal. Please either fail loud on the directives that alter results and keep the silent skip for the cosmetic ones (REP,MAP,KEYand friends), or log them, and correct the description either way. -
Dictionary download tooling should be settled once for both this PR and #1191, not separately. I raised there that
install(URI, Path)fetches without verification and that I would rather dropdev/download-mecab-dictionary.shthan keep integrity checking in a script we do not ship. The same applies here, and this script is the weaker of the two:base="https://raw.githubusercontent.com/LibreOffice/dictionaries/master/${collection}"builds in a download location, pins it to a moving branch, and verifies nothing, while the mecab script at least accepts an expected SHA-256. Whatever we land on in #1191, one Java path with digest verification and no shell helper, please apply it to both so the two features do not ship with different policies.
Smaller things, none of them blocking:
load(InputStream, InputStream)callsreadAllBytes()on both streams with no ceiling, so a caller passing a network stream has no bound on what gets buffered. Unlike #1191 nothing here pre-sizes from a declared count, the affix rule count and the.dicentry count are both handled correctly, so this is the only allocation worth bounding. The.affcontent is also decoded twice, once as ASCII to findSETand once with the declared charset, so a load holds two full copies plus the line array before parsing starts.suffixesEndingWith(char)andprefixesStartingWith(char)go throughMap<Character, List<Affix>>, so each call boxes aCharacter. Same point I left on the trie in #1191.- The PR reads flags per code point, with an explicit comment about supplementary characters under
FLAG UTF-8, butAffixConditionmatches percharandbucketByBoundarykeys on a singlechar. An affix whose material starts or ends with a supplementary character buckets on half a surrogate pair. The two halves of the same change should agree. PART_CHECK_BUDGET = 2048silently truncates the compound search. It is documented on the private constant, but a caller cannot see that analyses were dropped. Please state it in the public class javadoc, and consider making it settable.lookuphands out the internal mutable lists, which does not quite match the "instances are immutable" claim.Morphemein #1191 copies defensively; same treatment would fit here.morphologyIndexis a heuristic and will cut an entry that happens to contain a two-letter run followed by a colon.unescapedSlashtreats\\/as an escaped slash, so a word ending in a literal backslash before its flag separator parses wrong.HunspellStemmeris not final whileHunspellDictionaryis.- This PR carries no
@sincetags and #1191 puts@since 3.0.0on every new type. Pick one for both.
6f2d9d8 to
d8cc10e
Compare
Also in that commit: defensive copy from Stream size ceiling, char vs code-point consistency on affix material, and |
|
Follow-up in 8cede5e.
Download tooling still deferred with #1191 (one Java digest path, no shell helper). |
|
Same download rework as #1191 in 0b27d5e: shared
The |
|
Description updated: it now states the fail-loud/skip split per directive class instead of the blanket claim. The three items without a commit, deliberately:
PART_CHECK_BUDGET stays a fixed constant: it is documented in the public class javadoc now, and no real dictionary has come close to it in testing. If a compound-heavy dictionary ever does, promoting it to a startup override like the download ceilings is a small follow-up. That covers every item from the 2026-08-04 review; ready for another look. |
|
While this PR is under re-review we ran an internal bug hunt against the branch and found two holes in the fail-closed policy, both now fixed on the branch in the usual shape (failing tests in fe53f4c, fix in 39b576e). Flagging them here so the re-review can cover them. First, three result-altering directives were silently accepted: COMPOUNDRULE, IGNORE, and KEEPCASE. Per hunspell(5), COMPOUNDRULE defines pattern compounds from flag sequences, IGNORE strips characters from dictionary words, affixes, and input before matching, and KEEPCASE forbids capitalized forms of the flagged words, which matters here because the stemmer analyzes a capitalized surface through its lowercase variant and would otherwise produce analyses hunspell's own analyzer rejects. All three now join ICONV, OCONV, and COMPLEXPREFIXES in the load-time rejection, with the same "unsupported affix directive" IOException. Second, affix rules that strip the entire stem were applied without the FULLSTRIP directive. Hunspell itself accepts such affix files but refuses to apply the rule unless FULLSTRIP is declared, so the fix gates the rule at match time rather than rejecting at load; with FULLSTRIP declared the rule applies as before. The manual's stemmer chapter documents the extended directive list and the FULLSTRIP behavior. |
|
I think that we need to wait with this one until we have discussed the download resource direction from #1191 (comment) |
| int i = 0; | ||
| while (i < lines.length) { | ||
| final String[] fields = split(lines[i]); | ||
| if (fields.length == 0 || fields[0].startsWith("#")) { |
There was a problem hiding this comment.
Please declare "#" as a named constant for better re-use (in loops etc.)
| final String ascii = new String(affixBytes, StandardCharsets.US_ASCII); | ||
| for (final String line : splitLines(ascii)) { | ||
| final String trimmed = trim(line); | ||
| if (trimmed.startsWith("SET ") || trimmed.startsWith("SET\t")) { |
There was a problem hiding this comment.
Please declare "SET" and "SET\t"as a named constant for better re-use (in loops etc.)
| /** | ||
| * Inclusive upper bound on bytes buffered from one affix or dictionary stream | ||
| * during {@link #load(InputStream, InputStream)}. Larger streams fail with | ||
| * {@link IOException}. |
There was a problem hiding this comment.
Please add a sentence on what the (current) default value is: "64 MB"
| } | ||
| final DictionaryCatalog catalog = DictionaryCatalog.loadDefault(); | ||
| final String prefix = "hunspell." + dictionaryId + "."; | ||
| download(catalog, prefix + "aff", targetDirectory); |
There was a problem hiding this comment.
Can we declare a constant for "aff" and "dic" suffixes somewhere and refer to it in the Hunspell related classes and tests? Would the class HunspellDictionary be a good place to declare those public constants? Do we instead require an opennlp-api interface for HunspellDictionary and make the class the DefaultHunspellDictionary its impl?
|
|
||
| /** | ||
| * Inclusive ceiling on bytes buffered for one {@link #download(URI, Path, String)}, | ||
| * 512 MiB unless overridden via {@link #MAX_DOWNLOAD_BYTES_PROPERTY}. |
There was a problem hiding this comment.
How is this default value (512 MB) motivated here? Wouldn't 64 MB be sufficient? Please re-consider this value and change it accordingly if no one finds a proper rationale for that huge value.
|
@krickert Can we rebase and resolve this PR next? I think we can make some progress here soon. |
…aries A clean-room reader for .dic and .aff files with PFX/SFX rules, strip strings, character-class conditions matched by a single scan, cross products, and char, long, and num flag modes. No dictionary data is bundled: users point at their own files, so dictionary licenses never attach to the jar. Unsupported affix features fail closed, missing analyses rather than inventing them. (cherry picked from commit 0ecc39c)
…lasses Suffix rules now carry the continuation flags declared on their affix text, and analysis undoes a stacked pair when the inner rule's classes allow the outer one, so derived-then-inflected forms reduce to their dictionary word. (cherry picked from commit b543dea)
…plete javadoc
- Fold the two per-kind bucketing loops in the HunspellDictionary constructor into a
single bucketByBoundary helper that takes the rule list, the kind, and the sink for
the rules with empty affix material.
- Fold collectSuffixedPartStem and collectPrefixedPartStem, which differed only in the
boundary they face, into one collectAffixedPartStem with a suffix marker and an
atEdge marker; document what atEdge means at each end.
- Extract a parseValue helper for the single-integer directives so COMPOUNDMIN and
COMPOUNDWORDMAX no longer share one case body that re-tests which directive it is.
- Extract PREFIX_TAG, SUFFIX_TAG, and NO_MATERIAL constants and use them at the affix
block header, the rule lines, and the strip and affix material checks.
- Give FORBIDDENWORD its own case in the flag directive switch instead of letting the
catch-all default assign it, and make that default throw for a directive listed on
the outer switch but not handled on the inner one.
- Add the missing javadoc on the AffixCondition and HunspellDictionary constructors,
the Affix record components, and the splitLines, splitOn, and split helpers.
- Convert the single-line accessor javadoc on the compounding and affix bucket getters
to the {@return ...} form, and replace the hand-written prose on
HunspellStemmerFactory.newStemmer with {@inheritdoc} plus the instancing note.
- Trim commentary that restates the code: the bucketing rationale duplicated in
HunspellStemmer, the LibreOffice Spanish anecdote on the code point flag reader, and
the sentence left dangling in testGermanCompoundsDecompose.
- Drop the defensive null and directory guards from the test helpers
writeAndLoadFixture and load, which no caller can trip, and document what the real
dictionary tests assert.
- Fold the repeated ByteArrayInputStream plumbing in HunspellStemmerTest into two load
overloads, one UTF-8 and one taking the charset the SET declaration test needs.
- Turn the four table-style stemming tests into parameterized tests over their word and
expected stem pairs, so a failing row names itself.
- Add testNullArgumentsAreRejected, pinning the exact IllegalArgumentException message
of every public entry point including the argument names the stream loader reports.
- Correct the stemmer manual: name the example files after the fixture the test loads
rather than en_US, and state that the printed stems are the fixture's, since which
stem a published dictionary yields is that dictionary's decision.
Reject ICONV, OCONV, and COMPLEXPREFIXES at load time; keep skipping cosmetic tables such as REP. Copy lookup results defensively and document the compound search budget on HunspellStemmer.
Reject affix and dictionary streams above MAX_STREAM_BYTES. Affix conditions and boundary bucketing use Unicode code points so supplementary characters agree with FLAG UTF-8. Document the ceiling in the stemmer chapter and pin both behaviors in tests.
…ownload ceiling The 512 MiB download ceiling becomes a default that opennlp.download.max.bytes can raise at JVM startup; absent or invalid values fall back. Keeps the file identical to the copy in the MeCab PR.
…the stream ceiling The affix boundary buckets move from a boxed Integer map to a sorted int index answered by binary search, closing the per-call boxing note from the review; the strip-only rule lists are frozen at load so no internal mutable list is handed out. MAX_STREAM_BYTES becomes public, matching its citation in the load javadoc, the manual chapter, and the dev README. A parameterized test pins trimming of word-list lines edged by the no-break space and the ideographic space on both edges, and the download and catalog test classes get the javadoc coverage the hunspell tests already carry.
…e in the manual DocBook XML defines no nbsp entity, so the PDF build rejects it.
…ng PR Both PRs carry byte-identical copies; this folds the sibling's parameterized invalid-limit test into this side's documented fixtures so the copies match again.
…d full-strip rules with failing tests COMPOUNDRULE, IGNORE, and KEEPCASE alter analyses when ignored, so the fail-closed loader policy requires them to fail at load time like ICONV, OCONV, and COMPLEXPREFIXES; the loader currently accepts them silently. A suffix rule whose strip string is the whole stem is applied without the FULLSTRIP declaration hunspell requires for it, inventing a stem for a surface form the dictionary does not license.
…ll-strip rules behind FULLSTRIP COMPOUNDRULE licenses pattern compounds, IGNORE drops characters before matching, and KEEPCASE forbids the capitalized variants this stemmer analyzes through lowercasing, so ignoring any of them would change stems with no signal; they now join ICONV, OCONV, and COMPLEXPREFIXES in the load-time rejection. An affix rule whose strip string consumes the whole stem is now undone only when the affix file declares FULLSTRIP; without the declaration the rule is skipped at match time, which is what hunspell itself does rather than rejecting the file. The manual and the dictionary README follow the loader.
Add named format constants, lower the download default to 64 MiB, and require applications to supply URL catalogs. Red: the focused test compile failed for missing suffix constants and the caller-supplied catalog overload before the implementation was added.
39b576e to
c833264
Compare
# Conflicts: # dev/README-hunspell-dictionaries.md # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/hunspell/HunspellDictionary.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/hunspell/HunspellDictionaryDownload.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/DictionaryCatalog.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/stemmer/hunspell/HunspellDictionaryDownloadTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/stemmer/hunspell/HunspellRealDictionaryTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/stemmer/hunspell/HunspellStemmerFactoryTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/util/DictionaryCatalogTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/util/DigestTestUtil.java
…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).
|
@mawiesne patching a few edge cases I found here.. nothing big but I'll tag you when I push it. |
Go for a new branch plz - makes the diff easier. The changes are in now. |
Add bounded downloads, SHA-512 verification, staged publication, tar and zip extraction, catalog entries, and adapters for the Hunspell and MeCab features merged through #1190 and #1191. Red evidence recorded while developing and reviewing this branch covered unsafe redirects and schemes, archive traversal and expansion limits, incomplete tar data, checksum failures, replacement of existing files, malformed catalog values, and validation order.
The per-feature download path that #1190 and #1191 added to DownloadUtil (the download(URI, Path, sha512) method, the opennlp.download.remote gate, the byte ceiling and its property, and configuredLimit) merged with those PRs and is deleted here, with its test. DictionaryCatalog owns the gate and installs through ResourceInstaller, and the byte ceilings live in ResourceInstaller.Limits, so nothing in the tree called the old path. Model downloads and the cached-model checksum verification are untouched.
Add bounded downloads, SHA-512 verification, staged publication, tar and zip extraction, catalog entries, and adapters for the Hunspell and MeCab features merged through #1190 and #1191. Red evidence recorded while developing and reviewing this branch covered unsafe redirects and schemes, archive traversal and expansion limits, incomplete tar data, checksum failures, replacement of existing files, malformed catalog values, and validation order.
The per-feature download path that #1190 and #1191 added to DownloadUtil (the download(URI, Path, sha512) method, the opennlp.download.remote gate, the byte ceiling and its property, and configuredLimit) merged with those PRs and is deleted here, with its test. DictionaryCatalog owns the gate and installs through ResourceInstaller, and the byte ceilings live in ResourceInstaller.Limits, so nothing in the tree called the old path. Model downloads and the cached-model checksum verification are untouched.
…y resources (#1211) * OPENNLP-1909: Add verified third-party resource installer Add bounded downloads, SHA-512 verification, staged publication, tar and zip extraction, catalog entries, and adapters for the Hunspell and MeCab features merged through #1190 and #1191. Red evidence recorded while developing and reviewing this branch covered unsafe redirects and schemes, archive traversal and expansion limits, incomplete tar data, checksum failures, replacement of existing files, malformed catalog values, and validation order. * OPENNLP-1909: Cover installer validation edges Red evidence on the rebased branch: ZIP extraction on a non-default file system threw UnsupportedOperationException, catalog filenames accepted empty and path-like values, tar metadata bypassed maxEntries, and null timeout messages did not match the public validation contract. * OPENNLP-1909: Harden installer validation Validate catalog filenames, support ZIP archives on non-default file systems, and count tar extension headers against the entry limit. Align timeout errors and update the installer documentation. * OPENNLP-1909: Fold the duplicated path walk and tar fixture helpers ResourceInstaller walked the directories below the target twice with the same link and file checks, once to prove a destination vacant and once to create the path for the move; one helper now does both. The lattice tests kept a second copy of the gzip tar builders that the installer tests had, so TarArchives owns them and both packages call it. The MeCab README names the catalog example as the test resource it is. * OPENNLP-1909: Return DownloadUtil to its model-only surface The per-feature download path that #1190 and #1191 added to DownloadUtil (the download(URI, Path, sha512) method, the opennlp.download.remote gate, the byte ceiling and its property, and configuredLimit) merged with those PRs and is deleted here, with its test. DictionaryCatalog owns the gate and installs through ResourceInstaller, and the byte ceilings live in ResourceInstaller.Limits, so nothing in the tree called the old path. Model downloads and the cached-model checksum verification are untouched. * OPENNLP-1909: Pin the review findings on staging, expansion, and zip listings (failing tests) Red evidence on the rebased branch: an atomic move replaced an existing destination on POSIX, so the vacancy check was advisory; a 4 MiB gzip of zeros expanded in full because no ratio bounded it; *.BIN and *.GZ names did not match the lower-case rules; a download file and a staging directory left by a killed installation stayed in the target forever; a failed first installation left the target directory it created; and a zip whose local headers and central directory listed different files installed the local-header content. The MeCab installer also left a stale scratch directory in the target. The move is extracted into a helper so the promotion guarantee can be tested on its own. * OPENNLP-1909: Stage on the target filesystem, bound gzip expansion, and enforce vacancy at the move The MeCab installer unpacks into a hidden scratch directory beneath the target instead of the system temporary directory, so the download, the unpacked tree, and the installed files share one filesystem; stale scratch directories from a killed run are removed first. Gzip content is bounded to 100 times its compressed size, with a 1 MiB floor for small sources, before the absolute expansion limit applies. The promotion move is a plain move, since an atomic move renames over an existing file on POSIX and made the vacancy check advisory. Zip file names read from the local headers must match the central directory's file entries, on the default filesystem through ZipFile and elsewhere through the zip filesystem. The *.bin and *.gz name rules match in any letter case. Work files left in the target by a killed installation are removed at the next installation into it, and a failed installation removes the empty target directory it created; the class documents that concurrent installations into one target are not supported. The two ResourceLimits archive constants and their properties, which nothing read after this change, are deleted so the expansion property has one meaning. The tokenizer manual names both accepted digests, and the model loading chapter states the ratio rule. * OPENNLP-1909: Bound zip expansion by the same ratio as gzip The expansion-ratio guard applied only to gzip content, so a zip archive was bounded by the absolute expansion limit alone. Deflate reaches roughly 1000 to 1, so a few megabytes of crafted zip expanded to the full 4 GiB ceiling in the caller's target directory, and install(URI, Path) accepts a file: archive with no checksum at all. The ratio now applies to every compressed source: gzip and zip alike expand to at most 100 times the compressed size, with the 1 MiB floor for small sources. * OPENNLP-1909: Make the expansion ratio a configurable limit The ratio was a private constant, so 100 to 1 was a hard ceiling no caller could lift. Raising opennlp.install.max.total.bytes does not help, because the ratio and the absolute limit are enforced independently and the tighter one applies, so a resource that legitimately compresses better than 100 to 1 could not be installed at all. maxExpansionRatio joins the other six values on Limits, with the builder method and the opennlp.install.max.expansion.ratio property the three byte and entry ceilings already have, and must be positive like them. The manual states that the ratio and the expansion limit are separate, since raising the byte limit alone reads like the fix and is not. --------- Co-authored-by: Richard Zowalla <rzo1@apache.org>
3 participants
Adds a stemmer that reads standard Hunspell
.dic/.affdictionary pairs, built on theStemmerFactoryseam from OPENNLP-1883.Supported affix features: alias compression (AF), NEEDAFFIX, ONLYINCOMPOUND, FORBIDDENWORD, CIRCUMFIX, twofold suffix analysis through continuation classes, and compound word positioning including linking forms as used in German compounds. Parsing is regex-free. Result-altering unsupported directives (ICONV, OCONV, COMPLEXPREFIXES, unknown FLAG modes) fail loud at load time; cosmetic directives (REP, MAP, KEY and similar suggestion-only data) are skipped, so analyses relying on them are missed rather than invented. The class javadoc states the same split.
Dictionaries are user-supplied and nothing is bundled: the in-tree tests use a project-authored miniature dictionary, and
dev/README-hunspell-dictionaries.mddocuments acquiring published dictionaries together with their license files. The manual gains a Hunspell section whose example is asserted byHunspellManualExampleTest.Additionally verified against published dictionaries for English, Spanish, Hungarian, and German through the gated
HunspellRealDictionaryTest(runs only when-Dopennlp.hunspell.dict.diris set; skipped otherwise, and no dictionary data enters the tree).