Skip to content

OPENNLP-1893: Correct Hunspell parsing and analysis - #1266

Merged
rzo1 merged 5 commits into
apache:mainfrom
ai-pipestream:OPENNLP-1893-hunspell-followup
Sep 5, 2026
Merged

OPENNLP-1893: Correct Hunspell parsing and analysis#1266
rzo1 merged 5 commits into
apache:mainfrom
ai-pipestream:OPENNLP-1893-hunspell-followup

Conversation

@krickert

@krickert krickert commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #1190 after its merge.

This change:

  • applies FLAG and AF settings across the complete affix file
  • validates malformed encodings, counts, rule blocks, aliases, and numeric flag bounds
  • handles continuation flags, identity affixes, stacked suffixes, forbidden surfaces, and cross-product licensing
  • counts compound lengths and boundaries by Unicode code point
  • documents the validation and file-wide declaration behavior

The tests were committed first. On current main, the focused Hunspell suite reported 38 failures from the added cases. With the implementation applied:

Add focused cases for flag aliases, continuation classes, zero-material affixes, forbidden entries, compound Unicode boundaries, invalid limits, and malformed input encoding.

Red evidence on current main: the focused Hunspell suite reported 38 failures across these cases.
Apply file-wide FLAG and AF settings, reject malformed values and encodings, and count Unicode compound boundaries by code point. Handle continuation flags, identity affixes, and forbidden surface forms during analysis.
@rzo1

rzo1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Here are some load bearing comments ;-)

The real fixes in here are good and well-pinned: whole-file FLAG/AF resolution, identity-rule handling, code-point-based compound counting, CharacterCodingException on malformed input, BOM stripping, cross-product continuation licensing. But roughly 40% of the diff (285 comment lines removed, 400 added) is Javadoc rewording that changes nothing, and two behavior changes are shipped without documentation.

Blocking

1. HunspellDictionary#parseAffix switch — the default case now throws. This breaks published dictionaries.

default: went from i++; break; to throw new IOException("unsupported affix directive ..."), and twelve directives moved into the reject list: COMPOUNDMORESUFFIXES, COMPOUNDROOT, CHECKCOMPOUNDREP, SIMPLIFIEDTRIPLE, CHECKCOMPOUNDPATTERN, FORCEUCASE, COMPOUNDSYLLABLE, SYLLABLENUM, LANG, CHECKSHARPS, BREAK, FORBIDWARN.

HunspellRealDictionaryTest loads en_US, de_DE_frami and hu_HU, and its loadOrSkip Javadoc states a present pair failing to load "is a failure, not a skip". de_DE_frami and hu_HU declare LANG, BREAK, CHECKSHARPS, SIMPLIFIEDTRIPLE, and hu_HU declares COMPOUNDROOT/SYLLABLENUM. Please run with -Dopennlp.hunspell.dict.dir=... against those three pairs and paste the result. If they no longer load, this is a regression, not hardening.

Separately: LANG carries no rule that changes an analysis in this engine — it selects language-specific special cases in hunspell. Rejecting a purely informational directive alongside ICONV is over-strict. Either keep the reject list to directives that demonstrably change a stem when ignored (the existing six), or justify each of the twelve individually in the JIRA.

2. dev/README-hunspell-dictionaries.md is not touched and is now wrong.

stemmer.xml points at it for the supported feature set, and its "What the engine supports" section enumerates the exact accept/reject directive lists this PR changes. After this PR that section is stale on: the twelve new rejections, unknown-directive rejection, whole-file FLAG/AF, the 1..65000 numeric flag range, and twofold suffixes inside a cross-product. Please update it in this PR.

3. opennlp-docs/src/docbkx/stemmer.xml — the docs contradict the code.

The added paragraph documents FLAG/AF scope and the numeric range but says nothing about unknown directives now failing, while the line directly above still reads "cosmetic tables such as REP are skipped" and lists only six rejected directives. A reader concludes unknown directives are skipped. Extend the rejected-directive list and state the new default explicitly.

4. HunspellDictionary#forbiddenAtCompoundPosition invents a semantic.

return position != CompoundPosition.END && contains(flags, compoundForbid);

man 5 hunspell says COMPOUNDFORBIDFLAG applies to suffixes ("Suffixes with this flag forbid compounding of the affixed word") — which is what the existing forbidsInCompound(Affix) already implements correctly. Applying it to dictionary entry flag sets, and then carving out END so housedog still decomposes (testCompoundForbidFlagBarsDictionaryEntryBeforeEnd), matches nothing I can find in the format documentation. Please cite the source (man page section, or affixmgr.cxx in a named hunspell release) or drop it.

5. Restore the gutted class Javadoc on HunspellDictionary and HunspellStemmer.

The concrete supported-feature list on HunspellDictionary was replaced with "Supported features include prefix and suffix rules, continuation classes, flag modes and aliases, character encodings, compounds, blocking flags, circumfixes, and full-strip rules." That tells a user nothing about whether their dictionary will work — it was the one place the answer lived for a public API. Restore it and extend it with what this PR adds. Same for the HunspellStemmer paragraph.

6. 18 x Never {@code null}. stripped from @return tags across both classes.

That phrasing is this package's own convention, introduced with #1190: 20 occurrences across HunspellDictionary, HunspellStemmer, AffixCondition. This PR removes it from two of the three, leaving AffixCondition inconsistent with its siblings. Partial alignment is worse than none — restore them, or drop it from the whole package in a separate commit.

7. Unrelated prose churn throughout — please revert it.

A representative sample, none of which is a functional change: "carries" -> "contains" (~20x), "declared" -> "selected"/"specified" (~15x), "Never {@code null}." removal, "every" -> "all", "forbids" -> "rejects". One is a factual regression: FlagMode.LONG went from "each pair of characters is one flag" to "each consecutive character combination is one flag", which no longer says what long mode does. Revert the rewording and keep only the Javadoc that documents new behavior; it will cut this diff roughly in half and make it reviewable.

Two comments deleted with no replacement should come back:

  • // PSEUDOROOT is the directive's name before hunspell renamed it. That is exactly the "reader in five years" comment.
  • The AF header/alias explanation. The logic moved to readFlagAliases, so move the comment there; case "AF": i++; break; now looks like a silent no-op and needs a one-liner saying it was pre-parsed. Same for case "FLAG".

Minor

  • readFlagMode and readFlagAliases each scan all lines and call split(...) per line, then parseAffix scans and splits a third time. On a 64 MiB affix file that is three full passes and three String[] allocations per line. Split once into a String[][] and pass it to all three.
  • "FLAG" and "AF" are now repeated string literals in two places each, in a class that already declares PREFIX_TAG, SUFFIX_TAG, SET_TAG. Please declare FLAG_TAG and ALIAS_TAG as constants.
  • "numeric flag outside 1..65000 at line " hard-codes the value that MAX_NUMERIC_FLAG already holds. Build the message from the constant. Also add a reference to the constant's Javadoc for where 65000 comes from.
  • result.compoundMin is assigned, validated, then reassigned. Use a local, validate, assign once. Same for compoundWordMax.
  • COMPOUNDMIN is only bounded below. That is why HunspellStemmer now carries 2L * min and (long) remaining casts in two places. Bound COMPOUNDMIN at parse time instead and drop the long arithmetic.
  • supports(List, Affix, Affix) overloads supports(List, int) at a different level of abstraction. Rename to supportsCrossProduct.
  • In undoCrossProductSuffix the comment says "Each rule satisfies a needs-further-affix marker on the other", but the code directly under it rejects the case where both do. Update the comment.
  • The identity-rule branch is written out three times (inline in undoPrefix, in removeAffixInCompound, in removeSuffixAllowingIdentity). Add removePrefixAllowingIdentity for symmetry with the suffix helper and route all three through the pair.
  • isIdentityRule is private static while every other private helper in the class is an instance method. Make it non-static for consistency.
  • In analyze, read the guard as if (validStandalone) { ... } else if (anyForbidden) { return; }; the !validStandalone && conjunction hides that these are alternatives. Also: with homonyms where one set is standalone-valid and another carries FORBIDDENWORD, this lets the valid one win. Hunspell treats FORBIDDENWORD as dominant. Intentional? A fixture either way, please.
  • search now calls word.codePointCount(from, word.length()) and word.offsetByCodePoints(word.length(), -min) per recursion node, both O(n). That turns O(1) index arithmetic into an O(n) cost at every node of the split search. Compute the code-point offsets once per decompose and index into that array.
  • Numerals in prose: "at most 2 suffixes", "one or 2 suffixes", "a 2-suffix analysis", "2 NEEDAFFIX markers". Inconsistent with the spelled-out "one prefix" alongside it — please write them out.
  • Tests: testAliasTableCountIsValidated (3 cases), testContinuationFlagLicensesCrossProductPartner (2 fixtures), testCompoundBoundaryChecksUseCodePoints (2 fixtures), testMalformedFileEncodingIsRejected (2 fixtures) each bundle independent cases into one method. Split into @ParameterizedTest so a failure names the input.

Process

  • This PR reuses a closed issue's key. OPENNLP-1893 shipped as OPENNLP-1893: Support Hunspell affix dictionaries for stemming #1190 (1f929fad). This follow-up carries new user-visible behavior — fail-loud directive handling, new validation — and needs its own JIRA issue so the release notes describe it and OPENNLP-1893 stays closed.
  • Title. "Correct Hunspell parsing and analysis" is implementation vocabulary. Suggest something closer to release-notes language, e.g. "Apply Hunspell FLAG and AF declarations file-wide and validate affix input".
  • The Javadoc rewording, the Never {@code null}. stripping, and shortening pre-existing comments without a functional reason also show up in OPENNLP-1894: Harden CJK dictionary loading #1265 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.
  • Please do not merge before the HunspellRealDictionaryTest run against en_US/de_DE_frami/hu_HU is posted (Blocking 1).

@rzo1 rzo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the comments above

Preserve ResourceInstaller and the merged CJK implementation. Document skipped-directive limitations and restore pre-existing stemmer comments.

The affected reactor passes 3,091 tests with no failures, errors, or skips, including 128 Hunspell fixture cases and four real-dictionary cases. The separate run with LibreOffice en_US and the de_DE_frami and hu_HU packages also passes all four cases. Javadocs, DocBook HTML/PDF, Checkstyle, and forbidden API checks pass.
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.
krickert added a commit to ai-pipestream/opennlp that referenced this pull request Sep 5, 2026
# Conflicts:
#	opennlp-docs/src/docbkx/stemmer.xml

@rzo1 rzo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All blocking and minor items from the review above are addressed in e695b66d, 6e185321 and 8baa234d, and the net diff against main is confined to the five Hunspell files.

Two notes for the record, neither blocking:

  • The six result-altering directives (ICONV, OCONV, COMPLEXPREFIXES, COMPOUNDRULE, IGNORE, KEEPCASE) moved from reject to skip, not just the twelve added ones. That reverses the fail-loud stance from #1190. It is documented in dev/README-hunspell-dictionaries.md, stemmer.xml and the class Javadoc, so I am fine with it, but it is a deliberate behavior change and not merely the revert I asked for.
  • The HunspellRealDictionaryTest run against en_US, de_DE_frami and hu_HU is reported in the 8baa234d commit message rather than in this thread. Since the default: throw is gone the regression risk is moot, so I am not asking for a repost.

Please still file a separate JIRA issue for this follow-up so the release notes describe it and OPENNLP-1893 stays closed.

@rzo1
rzo1 merged commit 34ff57b into apache:main Sep 5, 2026
10 checks passed
@krickert

krickert commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

I’ll file a follow-up JIRA to complete Hunspell compatibility for stemming and analysis. Until those features are supported, I’d propose failing at load time for unsupported directives that could change the results, with an explicit option for partial compatibility. The error should identify the unsupported feature, not imply the dictionary is invalid. We should implement those rules rather than require users to remove them. I'll do it now.

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