OPENNLP-1933: Remove per-call regex compilation from Leipzig, Morfologik and spellcheck dictionary loading; read event lines and MASC identifiers by their format - #1279
Draft
krickert wants to merge 36 commits into
Conversation
krickert
force-pushed
the
OPENNLP-1933-string-regex-calls
branch
from
September 7, 2026 13:18
9a8dd2a to
60c0eed
Compare
rzo1
reviewed
Sep 15, 2026
rzo1
left a comment
rzo1
left a comment
Contributor
There was a problem hiding this comment.
Little time, so here is a GPT 5.6-sol review instead for now
No blocking findings. No additional regression found in the replacement scans or resource-closing fixes.
Validation across the combined stack: 1,856 targeted tests, zero failures, one skipped.
krickert
force-pushed
the
OPENNLP-1933-string-regex-calls
branch
from
September 15, 2026 16:00
60c0eed to
cb4660d
Compare
Contributor
|
Here are some additional comments. The rewrites that are meant to be pure (Leipzig file names, Morfologik file name, spellcheck frequency dictionary loader) fuzz identical to the old calls. The event-stream and MASC parts change file formats without saying so, and the resource-leak fixes don't belong in this PR. Blocking
Minor
Verified:
|
1 similar comment
Contributor
|
Here are some additional comments. The rewrites that are meant to be pure (Leipzig file names, Morfologik file name, spellcheck frequency dictionary loader) fuzz identical to the old calls. The event-stream and MASC parts change file formats without saying so, and the resource-leak fixes don't belong in this PR. Blocking
Minor
Verified:
|
Contributor
|
Follow-up:
|
krickert
changed the title
krickert
force-pushed
the
OPENNLP-1933-string-regex-calls
branch
from
September 16, 2026 01:43
af1d535 to
51bd136
Compare
Contributor
Author
|
Dependency note: #1302 (OPENNLP-1955) adds |
krickert
force-pushed
the
OPENNLP-1933-string-regex-calls
branch
from
September 16, 2026 06:01
51bd136 to
fff4466
Compare
…in DefaultLemmatizerContextGenerator
…in DefaultPOSContextGenerator Add pinning tests for the accept and reject sides of both predicates.
… checks in FeatureGeneratorUtil Add pinning tests for the capPeriod accept and reject sides.
…enPatternFeatureGenerator Add a pinning test that non-letter sub-tokens do not produce st= features.
… char scan Matches (.+)-\w+ semantics: group(1) is everything before the last hyphen, the hyphen must not be at index 0, and the suffix must be non-empty word chars. Add pinning tests for outcomes without hyphen, hyphen at index 0, empty suffix, non-word suffix, and the normal accept case.
…n BrownCluster Replicates String.split(\t) semantics, including dropped trailing empty fields.
…explicit char scans in TokenSampleStream splitOnWhitespace replicates String.split(\\s+): a leading whitespace run yields one empty leading field, runs collapse, and trailing empty fields are dropped.
…mojiCharSequenceNormalizer The replaced pattern contains a high surrogate range, so the regex engine matches whole code points in the flattened range [U+D83C, U+10FC00]. The replacement scans code points, collapses each maximal matching run into a single space, and copies non-matching code points verbatim. Add pinning tests for unpaired surrogates, BMP chars above U+D83C, and supplementary code points beyond U+10FC00.
…xplicit scans in ConlluStream
splitOnHyphen replicates String.split("-"): every hyphen is a boundary,
empty fields between consecutive hyphens are kept, and trailing empty
fields are dropped.
extractTextLang replicates find() of text_([a-z]{2,3}): the first
occurrence of "text_" followed by two to three ASCII lowercase letters,
preferring three.
…ss scans in ParserTool
The two replaceAll passes are replicated by two cursor passes with the
same leftmost-first resume-after-match semantics, which matters for
overlapping pairs such as "x((" or "((a)(b))": a pair starting at the
second char of a match is only reconsidered by the second pass.
…cans in DownloadUtil parseChecksum now scans to the first ASCII whitespace character, replicating split(\s)[0] on the trimmed content. extractLinks replicates find() of the <a href="(.*?)">(.*?)</a> pattern with CASE_INSENSITIVE and DOTALL flags: the href value ends at the first "> and the first case-insensitive </a> closes the match, so nested link markup is swallowed by the outer match.
…meric patterns with explicit scans in ADNameSampleStream
splitOnWhitespace and splitOnUnderscores replicate run-based splitting: a
leading separator run yields one empty leading field, trailing empty
fields are dropped, and an all-separator input yields no fields.
matchHyphenatedToken replicates the three-branch hyphen pattern at code
point granularity, isAlphaNumeric replicates ^[\p{L}\p{Nd}]+$ via
Character.isLetter and Character.isDigit, and tagContent replicates
matches() of <(NER:)?(.*?)> including its optional NER: prefix.
…OSSampleStream
replaceWhitespaceWithEquals replicates replaceAll("=") of the \s+
pattern: every run of ASCII whitespace, including leading and trailing
runs, is replaced by a single equals sign.
…tenceSampleStream parseTextAndParagraph replicates matches() of the ^(?:[a-zA-Z\-]*(\d+)).*?p=(\d+).* pattern: after the optional ASCII letters and hyphens, the text id is the first ASCII digit run and the paragraph id is the digit run after the first "p=" that is followed by at least one digit.
…entenceStream replaceGuillemetPunctuation replicates replaceAll of the »\s+ punct patterns: every run of ASCII whitespace between » and the punctuation character is removed. parsePunctuationLine replicates matches() of the ^(=*)(\W+)$ pattern: the line consists of leading equals signs followed by one or more non-word characters, where a word character is an ASCII letter, digit, or underscore. A line of only equals signs matches, with the last equals sign as lexeme.
Adds StringUtil.isAsciiWhitespace, splitOnAsciiWhitespace, containsAsciiUpperCase, and containsAsciiDigit and removes the copies from the AD streams, the English TokenSampleStream, DownloadUtil, and the POS and lemmatizer context generators. NameFinderME.extractNameType delegates to BioCodec. Cases the new tests found first: the TokenSampleStream split returned one empty token for a whitespace-only line where the original split returned none, and matchHyphenatedToken accepted a single hyphen. BrownCluster.splitTabs now removes all trailing empty fields, as String.split does. Each helper has a test, parameterized where the inputs are a table, with the reject side and the edge cases: empty input, leading and trailing separators, non-ASCII spaces and digits, and supplementary-plane characters. Helpers only called from instance methods are no longer static; the block comments on the helpers are now Javadoc that states the behavior.
… quirks Scans now do what the code meant instead of what the regular expression would accept: - EmojiCharSequenceNormalizer replaces only supplementary-plane code points. BMP characters, hyphens and unpaired surrogates are kept. - ParserTool separates brackets on Unicode whitespace in one pass, so brackets that follow each other are all spaced and no space is doubled. - The FeatureGeneratorUtil capital-period feature needs one capital and one period; a trailing line break no longer qualifies. - BioCodec rejects a line terminator at any position of an outcome type. - DownloadUtil accepts a checksum file with leading Unicode whitespace and treats a blank file as missing. - The AD streams split on the toolkit whitespace definition, drop empty underscore parts and judge punctuation lines by code point. - ConlluStream fails a malformed multiword token id with an InvalidFormatException instead of skipping it. StringUtil adds isAsciiLetter, isAsciiDigit, endOfAsciiDigits, isLineTerminator and indexOfLineTerminator, shared by BioCodec, ADSentenceSampleStream and ConlluStream here and needed by the related OPENNLP-1929 to OPENNLP-1935 branches. isAsciiWhitespace and splitOnAsciiWhitespace are removed; callers use the Unicode-aware StringUtil.isWhitespace and WhitespaceTokenizer. The manual documents the bracket handling of the parser tool, the CoNLL-U multiword id and language code rules, the AD reader rules and the emoji normalizer in the language detector chapter. Tests cover the accept and reject side of each scan, the Unicode plane boundaries and Unicode whitespace separators.
SimpleEventStreamBuilder and RealValueFileEventStream split the context
part of each event line with String.split("\\s+"), which compiles the
pattern on every call. Both now use StringUtil.splitOnAsciiWhitespace,
which yields the same elements: a leading run gives one empty first
element, trailing empty elements are dropped, and only the six ASCII
whitespace characters separate fields, so a no-break space stays inside
a field. New tests pin those cases through the builder and the stream.
RealBasicEventStream split the context part of each event line with
String.split("\\s+"), compiling the pattern per line. It now uses
StringUtil.splitOnAsciiWhitespace, which yields the same elements
including the leading empty element after a double space and the
dropped trailing empties. A new test pins tab and multi-space runs and
a no-break space that must stay inside a field.
BasicContextGenerator.getContext still calls String.split with the
caller supplied separator. Its public constructor accepts any String,
so a caller may pass a pattern today; changing that needs a decision on
the API contract and is left as is.
LeipzigLanguageSampleStream accepted a sentences file when the first three characters of its name matched "[a-z]+", compiling the pattern for every directory entry. A small scan now checks that those characters are ASCII lower case letters, which rejects the same names: capitals, digits, punctuation, and letters outside ASCII. The length three is a named constant shared with the two other places that cut the language code from the file name. Tests cover the scan directly on both sides and read a temporary directory holding accepted and rejected names through the stream.
MascNamedEntityParser, MascPennTagParser, and MascWordParser removed the "ne-n", "penn-n", and "seg-r" prefixes from identifier attributes with String.replaceFirst, which compiles a pattern for every element. The new package-private MascIdentifiers holds the three prefixes as constants and a removeFirst helper that cuts the first occurrence with indexOf and substring and leaves later occurrences in place, as replaceFirst did. Tests check the helper against replaceFirst on both sides, drive each parser over inline annotation XML, and pin that a repeated prefix still fails to parse.
MorfologikDictionaryBuilder.build turned the metadata file name into
the dictionary file name with replaceAll("\\.info$", ".dict"), building
the pattern from the Morfologik extension constant on each call. The
new toDictionaryFileName checks endsWith(".info") and exchanges that
suffix with substring, leaving any other name unchanged. Both suffixes
are named constants. The test compares the helper with the former
replaceAll over names with a repeated, missing, or differently cased
suffix and checks that the built dictionary is written to the metadata
directory under the expected name. The module pom adds the parent
managed junit-jupiter-params test dependency for the parameterized
test.
FrequencyDictionaryLoader split each dictionary line with the pattern "[\t ]+". The new splitColumns walks the line and breaks it on runs of TAB and space only, with the result of Pattern.split: a leading run gives one empty first column, trailing empty columns are dropped, a separator-only line gives an empty array, and an empty line gives one empty column. A no-break space, a vertical tab, or a form feed stays inside a column, as before. The test compares the scan with the former pattern over those cases and reads unigram and bigram lines with mixed TAB and space runs through parseUnigrams and parseBigrams. The module pom adds the parent managed junit-jupiter-params test dependency for the parameterized test. SpellCheckingCharSequenceNormalizer keeps its URL_LIKE pattern: it combines three alternatives with URL schemes, an email shape, and a list of top level domains, which is a heuristic rather than a plain character scan.
New base removed StringUtil.splitOnAsciiWhitespace, so the event streams no longer compile. Red evidence: cannot find symbol at RealValueFileEventStream.java:141 and SimpleEventStreamBuilder.java:44 (RealBasicEventStream.java:64 next in line). The new tests pin the intended contract: whitespace runs (Unicode) delimit contexts, NBSP splits, and no empty-string predicate survives leading, repeated, or trailing runs.
Follows the red test commit. Replaces the removed StringUtil.splitOnAsciiWhitespace with WhitespaceTokenizer.INSTANCE.tokenize, matching the TokenSampleStream precedent: Unicode whitespace runs delimit, empty fields are dropped instead of becoming empty-string predicates. Full opennlp-ml-commons and opennlp-ml-maxent suites pass.
…intended Review pass over the scans on this branch. Where a scan had copied a String.split or replaceFirst quirk, it now does what the format means: - Event lines: RealValueFileEventStream.parseEvent takes the outcome as the first whitespace separated field, shared with RealBasicEventStream. A tab or a run after the outcome no longer breaks the line, an outcome only line is an event without contexts instead of the end of the stream, and a blank line fails with an InvalidFormatException. - SimpleEventStreamBuilder splits the outcome at the first slash, so a context may contain one, and rejects an event without contexts. - MascIdentifiers.parseId requires the prefix at the start followed by ASCII digits only; parseIds parses link targets separated by whitespace runs. A doubled or missing prefix is reported instead of parsed. - FrequencyDictionaryLoader.splitColumns returns the non-empty columns only, so leading and repeated separators make no empty column. Tests no longer compare a scan with the pattern it replaced; they pin the accepted and rejected inputs directly. The manual describes the event file line format, the Leipzig file name rule, and the dictionary column separators.
… rows (red) SimpleEventStreamBuilder accepts a valued context with no name before the separator, and the dictionary loader reports a line of no-break spaces as malformed although a line of whitespace is documented as skipped.
… by the toolkit rule SimpleEventStreamBuilder takes a valued context as the text before the first semicolon and the number after it, and rejects a context with an empty name, an empty value, or a further semicolon. FrequencyDictionaryLoader tests whether a line is blank with StringUtil.isBlank, so a line of no-break spaces is skipped like a line of spaces, as the spellcheck chapter of the manual states.
…nary columns Event lines: the outcome may contain a slash or an equals sign, the value follows the last equals sign in a context, a context without a number is kept as written and counts once, exponent and signed values are read, negative values are rejected with the context named, and CR, LF, and CRLF line terminators are all accepted by both stream classes. SimpleEventStreamBuilder: the outcome stops at the first slash, a valued context name may contain equals signs, the first context determines whether the event has values, and a value that is not a number is rejected. MASC ids: digits of other scripts, superscripts, roman numerals, and a supplementary digit are rejected in an id and in an id list, as is a number above Integer.MAX_VALUE. Leipzig: upper case letters in the language code are rejected by the check and by the directory scan, and files with CR, LF, or CRLF line terminators are read. Dictionary loader: skipped lines count toward the reported line number, each malformed line names the reason, signed and zero-padded counts are read, columns after the count are ignored, a comment after the byte order mark is skipped, and other whitespace remains inside a bigram word.
… (red) FrequencyDictionaryLoader reads a count written in Arabic-Indic or fullwidth digits as a number, where the rest of the stack accepts ASCII digits only. SimpleEventStreamBuilder accepts a negative value that RealValueFileEventStream rejects.
… values FrequencyDictionaryLoader checks the count column with StringUtil.endOfAsciiDigits after an optional sign, so a count in the digits of another script, a decimal point, or an exponent is reported as malformed with the column named, and an overflowing count as out of range. The spellcheck chapter of the manual states the rule. SimpleEventStreamBuilder rejects a negative value with the context named, as RealValueFileEventStream does.
… the mode RealValueFileEventStream.parseEvent and SimpleEventStreamBuilder.add split their fields with StringUtil.splitOnUnicodeWhitespace, so the same file gives the same events in each JVM and the shared WhitespaceTokenizer instance is not involved. A null line or event is an IllegalArgumentException, and the builder reports format errors as IllegalArgumentException as well. The tests pin U+0085, U+2028, U+3000, U+001C, U+200B and a supplementary character, show that setKeepNewLines on the shared tokenizer has no effect, and the maxent stream test keeps one smoke test instead of a copy of the parsing matrix. The manual paragraph covers the real-valued streams only.
XML attribute-value normalization has already turned tabs and line breaks into spaces, so identifier lists and region anchors split on runs of spaces through one package helper instead of the shared WhitespaceTokenizer instance; a tab or no-break space kept by a character reference is malformed. An identifier number that does not fit an int is reported with the identifier named, the word parser keeps the cause and message of a failure, and the three parser tests share one SAX helper.
…ites The Leipzig language code check reads the first three characters of the file name in place and is a private instance method; its tests use real Leipzig file names. The Morfologik file name helper is an instance method and its test rows are the reachable .info names. The frequency dictionary loader skips lines of Unicode whitespace independent of the mode, splits columns in a private instance method with named separators, and the two loader tests from the serialization test moved next to the other loader tests with one shared text resource helper. The spellcheck chapter states what strip() removes at the line ends.
krickert
force-pushed
the
OPENNLP-1933-string-regex-calls
branch
from
September 16, 2026 06:09
fff4466 to
9bd19f5
Compare
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
2 participants
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.
Based on #1275 and including its commits until that one merges. The diff over the #1275 tip is this PR.
What changes
Three groups of change, listed separately so that the reviewers' request to split them can be acted on.
1. Rewrites that behave as before
LeipzigLanguageSampleStream: thematches("[a-z]+")check on the first three characters of a file name is a scan of those characters in place. 3.2M generated names give the same answer as the old call.MorfologikDictionaryBuilder: the anchoredreplaceAllof the.infosuffix isendsWithandsubstring. Same output on 3M generated names, apart from a trailing line terminator thatDictionaryMetadatacannot produce.FrequencyDictionaryLoader: the[\t ]+column split is a scan over tabs and spaces. 3M lines through the per-line pipeline give the same entries, and the 82k-word dictionary and the tab-separated test resources load the same. Two details are now stated in the manual: whitespace at the line ends is removed asString.strip()does it, and a line of Unicode whitespace only is skipped, independent ofopennlp.whitespace.mode.2. Event line format (
RealValueFileEventStream,RealBasicEventStream,SimpleEventStreamBuilder)The fields of an event line are separated by runs of Unicode whitespace (the
White_Spaceproperty,StringUtil.splitOnUnicodeWhitespace). The definition is fixed: it does not depend onopennlp.whitespace.mode, and it does not go through the sharedWhitespaceTokenizer.INSTANCE(OPENNLP-1947). The old code split the outcome at the first space and the contexts withsplit("\\s+"), which gave these differences, all pinned by tests:a b=1["", "b"][b]a b"", contextaa, contextba<TAB>b ca<TAB>ba, contexts[b, c]White_Space)RealValueFileEventStreamthrewStringIndexOutOfBoundsException;RealBasicEventStreamreturnednulland ended the stream, dropping the rest of the fileInvalidFormatExceptionnulllineNullPointerExceptionIllegalArgumentExceptionSimpleEventStreamBuilder.add:o/a/b co, contexts[a/b, c]/a""o/ a;1["", "a;1"][a]with value[1.0]o/;0.5o/a;,o/a;0.5;1o/a;-1RuntimeExceptionIllegalArgumentExceptionA value follows the last
=of a context, so a context name may contain that character; a context with text after the last=that is not a number is kept as written with the value 1, and an error is logged. This was the behavior before and is now written down in the manual.FileEventStreamis not touched; it reads events without values and still splits on spaces, tabs, and form feeds throughStringTokenizer.No eval build is needed for this group: every in-repo caller of these three classes is under
src/test, and the existing test data parses identically.3. MASC identifiers (
MascIdentifiers, the three MASC parsers)An identifier is its fixed prefix followed by ASCII digits, and a list of identifiers or anchors is separated by runs of spaces. The XML parser has already turned tabs and line breaks written into an attribute value into spaces (attribute-value normalization), so a tab or no-break space that reaches the parser through a character reference is malformed. The old code removed the first occurrence of the prefix and parsed the rest, which gave these differences:
7,ne-n+7,7ne-nas an idSAXException0 1seg-r0as a target list[0, 10]SAXExceptionseg-r1 seg-r2,seg-r1seg-r1<NBSP>seg-r2,seg-r1	seg-r2ne-n2147483648NumberFormatExceptionwith the JDK messageIllegalArgumentExceptionnaming the identifier, asSAXExceptionfrom the parseranchorswith one or three numbersArrayIndexOutOfBoundsExceptionor the third number ignoredSAXExceptionnaming the reasonA malformed MASC id, target list, or anchor pair now fails with a
SAXExceptionthat includes the cause, instead of being parsed leniently. All 53 ids in the MASC test resources parse the same as before.Also in this diff
MarkableFileInputStream.close, the LeipzigFiles.linesstream) moved to OPENNLP-1948 with their tests, so they get a release note and a 2.x backport.RealValueFileEventStream.parseEvent(String)is public becauseRealBasicEventStreaminopennlp-ml-maxentcalls it. It is new API.\uescapes; the MASC parser tests share one SAX helper and the spellcheck loader tests share one text resource helper.Before merge
MascDocumentStream(MASC has no eval coverage).StringUtil.isAsciiLowerCase, that helper replaces it.Open decisions for the maintainers
Verification, with checkstyle,
-Dopennlp.forkCount=1: ml-commons 117, ml-maxent 51, formats 673, morfologik 15, spellcheck 189 tests, 0 failures.OPENNLP-1933