Conversation
This was referenced Sep 7, 2026
krickert
force-pushed
the
OPENNLP-1932-model-resolver-glob
branch
from
September 7, 2026 13:18
5d7315e to
8913d04
Compare
rzo1
requested changes
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
Request changes. The protected API removal breaks downstream subclasses.
Validation across the combined stack: 1,856 targeted tests, zero failures, one skipped.
krickert
force-pushed
the
OPENNLP-1932-model-resolver-glob
branch
from
September 15, 2026 16:00
8913d04 to
e7e5f06
Compare
Contributor
|
Here are some additional comments. The new matcher is correct and a real improvement: it agrees with a reference DP on 1M random and 87M exhaustive pairs, it is linear where the old regex could hang, and typical Blocking
Minor
Verified:
|
1 similar comment
Contributor
|
Here are some additional comments. The new matcher is correct and a real improvement: it agrees with a reference DP on 1M random and 87M exhaustive pairs, it is linear where the old regex could hang, and typical Blocking
Minor
Verified:
|
krickert
force-pushed
the
OPENNLP-1932-model-resolver-glob
branch
from
September 16, 2026 06:01
edb46b6 to
a363b77
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.
…ssPathModelFinder The fallback that reads java.class.path split the value with two compiled Patterns, ";" on Windows and ":" elsewhere. A package-private splitClassPath now walks the string once and cuts at the separator character, keeping the String.split result for that separator: a leading separator gives one empty first element, empty elements between consecutive separators stay, trailing empty elements are dropped, separator-only input gives an empty array, and empty input gives a single empty element. The test table covers those cases for both separators and checks each row against String.split. The module gains the managed junit-jupiter-params test dependency for the table.
AbstractClassPathModelFinder turned a wildcard such as "*opennlp-models-*" into a regular expression by escaping "." and rewriting "*" to ".*" and "?" to ".", then compiled it and called matches() on the file part of each URL. The package-private GlobMatcher now walks the wildcard and the input by code point: "*" absorbs any run of characters including none, "?" takes exactly one, and every other character stands for itself. As with the old ".", neither wildcard crosses a line feed, carriage return, next line, line separator, or paragraph separator, and the whole input must be covered. The protected asRegex and matchesPattern(URL, Pattern) are replaced by matchesWildcard(URL, String); DirectoryModelFinder and SimpleClassPathModelFinder keep the plain wildcard strings instead of compiled patterns, and the pattern cache in DirectoryModelFinder goes away because there is nothing left to compile. Characters the old code never escaped, such as parentheses, brackets, braces, "+", "|", "^", "$", and backslash, had regular expression meaning before or made Pattern.compile fail; they are now literal. GlobMatcherTest covers the accept and reject sides, dots, those characters, each line terminator, and supplementary-plane characters.
DirectoryModelFinder had no test. The new test copies the opennlp-models jars from the test class path into a temporary directory one level below the scanned root, runs the shared finder assertions against it, and checks the non-recursive mode, a narrowing jar prefix, an unknown prefix, and the null directory rejection.
GlobMatcherTest now calls asRegex and matchesPattern. Test compile fails with cannot find symbol, the same break a downstream subclass would hit.
Restores protected asRegex and matchesPattern as deprecated shims over GlobMatcher and guards null inputs. Old regex strings no longer apply.
Adds parameterized null glob and input cases plus finder null checks. All fail fast with NullPointerException carrying a must not be null note.
The private copy was byte-identical to the rewritten 1928 helper (same five terminators), so calls now go to StringUtil and the duplicate is gone. GlobMatcherTest still pins the ?-never-matches-a- terminator contract; model-resolver suite green.
…behavior (red) A wildcard covers a line terminator like any other character, empty class path entries are skipped wherever they appear, null arguments fail with an IllegalArgumentException, and the deprecated asRegex and matchesPattern pair keeps its regular expression contract so a subclass compiled against the previous API filters as before. The tests fail on the current implementation.
… contract of the deprecated matchers GlobMatcher treats a line terminator like any other character, since a wildcard for a file name has no reason to stop at one, and it reports a null argument with an IllegalArgumentException as the rest of the module does. splitClassPath skips empty entries wherever they appear because none of them names a jar file. asRegex again returns a regular expression, with the literal characters quoted and with DOTALL so that it accepts what matchesWildcard accepts; matchesPattern evaluates the Pattern as a regular expression over the complete file part. Both are deprecated for removal in favor of matchesWildcard, so a subclass compiled with the previous API links and filters as before. The manual describes the wildcard syntax and the class path split.
…oded names The glob matcher is checked with consecutive stars, a glob longer than the name, regex syntax as plain characters, path separators in the glob, jar entry paths, drive letters in the file part of a file URL, and percent-encoded file names, on the accept and the reject side. The deprecated asRegex translation is compared with the matcher on each of those rows, and matchesWildcard is checked on jar, file and http URLs together with the deprecated asRegex and matchesPattern pair. The file part of a jar URL is the inner URL with its scheme, and a query string is part of the file part while a fragment is not. Class path splitting is checked with drive letters, UNC paths, spaces, percent signs, jar entry suffixes and line breaks in entries.
krickert
force-pushed
the
OPENNLP-1932-model-resolver-glob
branch
from
September 16, 2026 06:09
a363b77 to
4bebe3c
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 independent of the other parts of the epic. It can be reviewed and merged in any order relative to them. This diff carries the #1275 commits until that one merges; the commits of this change alone: ai-pipestream/opennlp@OPENNLP-1928-regex-removal-trivial...OPENNLP-1932-model-resolver-glob
AbstractClassPathModelFinder,DirectoryModelFinder, andSimpleClassPathModelFinderconverted wildcard strings to regular expressions and split the class path with compiled patterns. A package-privateGlobMatchernow matches*(any run) and?(one code point) directly, anchored on the whole name and, as before, not crossing line terminators; the class path split is a character scan withString.splitsemantics.One intended behavior change: characters the old conversion never quoted (
( ) [ ] { } + | ^ $ \) had regex meaning or threwPatternSyntaxException; they now stand for themselves.API: the protected
asRegexandmatchesPattern(URL, Pattern)on the public abstractAbstractClassPathModelFinderare replaced by protectedmatchesWildcard(URL, String). No code in the repository used them.Verification: 39 globs by 46 inputs plus a 300,000-pair fuzz compared with the old regex path; all differences come from the unquoted characters above.
GlobMatcherTestadds 64 cases and the module gains its firstDirectoryModelFinderTest. opennlp-model-resolver: 119 tests in each of its three surefire executions, with checkstyle, offline,-Dopennlp.forkCount=1.OPENNLP-1932