OPENNLP-1928: Stop EmojiCharSequenceNormalizer from blanking hyphens and BMP characters; reject malformed CoNLL-U multiword ids - #1275
Conversation
mawiesne
added
java
|
@krickert If you think this PR is ready for review, please put it out of draft. I'll review once ready. Think we can make some progress, as M6 vote seems to pass and will be processed soon, so we can add on top of that. |
|
Yes, I do think they're ready for review. Thank you! Let me know about anything you'd like to see changed. |
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Little time, so here is a GPT 5.6-sol review instead for now
Request changes. Three behavior changes need preservation or an explicit compatibility decision.
Validation across the combined stack: 1,856 targeted tests, zero failures, one skipped.
| if (matcher.matches()) { | ||
| return matcher.group(1); | ||
| int separator = outcome.lastIndexOf('-'); | ||
| if (separator > 0 && isWordChars(outcome, separator + 1)) { |
There was a problem hiding this comment.
The previous regex rejected line terminators in the type prefix. person\u2028-start now produces a type instead of null. Please preserve that rejection.
| * @return {@code true} for exactly that shape. | ||
| */ | ||
| private static boolean isCapPeriod(String token) { | ||
| return token.length() == 2 && isCapPeriodStart(token.charAt(0)) && token.charAt(1) == '.'; |
There was a problem hiding this comment.
A.\n previously produced cp; this now produces ic. The original $ anchor accepts a final line terminator. Please preserve the feature behavior.
| int i = 0; | ||
| while (i < text.length()) { | ||
| int cp = Character.codePointAt(text, i); | ||
| if (cp >= LOWER_CODE_POINT && cp <= UPPER_CODE_POINT) { |
There was a problem hiding this comment.
The original regex also matches literal hyphens. a-b previously normalized to a b, but now remains a-b. Please preserve this behavior or explicitly document and test the compatibility change.
|
So some of the "corrections" are preserving regex, not doing the intent. They would create an intentional bug in the code. The purpose of the ticket is to "de-regex" because regex overmatches and does unpredictable behavior / is slow (haha holding off on doing a regex rant here). If the regex is bugged, we're going to cover it in this ticket. We won't preserve bugs :) I'm going to be methodical about this one, and I am in favor of writing a ton of unit tests to ensure the behavior of our code does what it is intended to do, but not to preserve the behavior if it's buggy. This is a great opportunity to harden edge cases that have gone unnoticed. I'm going to take advantage of that. Please feel free to point out any edge cases I may want to try and they will be covered. I'm looking forward to this because we'll have deterministic behavior. I've even seen differences on regex matching between JDK versions. It hurts my eyes :) |
|
Go for adding tests . |
|
This is soooo regex - Tests are finding more bugs - that existed in both regex and our code. Fixing all of the regex domain ones and improving the test coverage. |
|
Poor sol - in one of the other branches your sol review suggested a regex fallback - I'm not going to do that. Fallbacks are weaksauce and by definition cause issues that go against the purpose of these tickets (and it's also a nasty habit most LLMs do to make their code pass where it should fail loudly). Instead I'll back up the decision with heavier testing. If regex is needed for that feature, we should be explicit and make it a separate feature and call out that it's a regex feature. I found one instance where we do accept patterns, I'll include a one-sentence brief as to how it is limited to the JDK char sets. |
|
The emoji change touches default langdetect features. I'll trigger the eval build before we merge. Still working on it though - almost ready to merge. |
|
Here are some additional comments. Def. needs an eval build for the compat changes (langdetect via Blocking
MinorStringUtil / DownloadUtil
AD readers
ConlluStream / ParserTool
Runtime / featuregen / emoji
Process
Sibling PRs (#1276–#1282, all draft)
|
|
Follow-up: the pre-existing issues found during review are now filed.
|
krickert
changed the title
694d650 to
fece12a
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.
…cans Index pages with CRLF line endings and uppercase anchors, href values with entities or percent escapes returned as written, and checksum files with CR or CRLF line endings, an uppercase hash, or a path after the hash. Outcomes with a line terminator at either end for the BIO codec, multiword token ids with digits from other scripts, trailing whitespace or an overflowing right side for CoNLL-U, paragraph keys followed by digits from other scripts for the AD reader, CR and CRLF lexicon lines for Brown clusters, and unpaired surrogates at either end plus combining marks after a supplementary run for the emoji normalizer.
StringUtil: contract-only Javadoc, a shared null message and offset check, isAsciiLowerCase next to isAsciiLetter, and indexOfLineTerminator returns -1 when there is no terminator, with the Pattern definition and the wider UnicodeWhitespace line break set named. EmojiCharSequenceNormalizer: readResolve, a shorter class Javadoc, no allocation when the text has no supplementary code point, one codePointAt per step, and a CJK Extension B case in the test. The serialver default equals the written value on main and here, so the UID is unchanged. The English TokenSampleStream splits on Unicode White_Space with a fixed definition instead of the shared WhitespaceTokenizer, and the Javadoc and tests state the changed behavior, quote state reset included. DownloadUtil: the hash is the first field of a Unicode whitespace split, a blank checksum file is an IOException that names the file, the anchor scan ends at an unclosed anchor, and case is ignored via regionMatches. ConlluStream: a MultiwordRange record, constants for the separator and messages, leading zeros rejected, an empty node id test, and the manual states what earlier releases did with malformed ids. ParserTool returns a line without brackets unchanged. The name finder validators call BioCodec directly. Feature generator tests are parameterized with boundary cases, and the manuals for the parser tool and the language detector are corrected.
…nder LEGACY CompatibilityMode in opennlp-api selects, for the classes that corrected their output in 3.0.0, between the corrected output and the output of the 1.x/2.x releases, built like WhitespaceMode and independent of it. Under LEGACY the EmojiCharSequenceNormalizer replaces hyphens, code points from U+D83C to U+10FC00 and unpaired surrogates in that range, as the old pattern did, so language detector models trained with an earlier release keep their n-grams until they are retrained. A test compares the legacy output with the old pattern on all code points. The manual describes the property in a new section.
fece12a to
1ecab10
Compare
3 participants
Replaces the regular expressions of sixteen small sites with character scans and adds the helpers they share to
StringUtil. Most sites give the same output as before; the ones that do not are listed below with the old and the new result. Where the two differ, the scan follows what the code was meant to do, and a test pins each corrected case.Behavior changes
EmojiCharSequenceNormalizer (deprecated, part of the default language detector chain). Only supplementary-plane code points, U+10000 and above, are replaced by a space now. The old pattern was parsed as the range U+D83C to U+10FC00 plus a literal hyphen, so it also blanked the hyphen, the BMP characters from U+D83C to U+FFFF (fullwidth and halfwidth forms, CJK compatibility ideographs, Arabic presentation forms, the private use area, U+FE0F, and unpaired surrogates from U+D83C to U+DFFF), and it kept U+10FC01 to U+10FFFF. Output differs for U+002D, U+D83C to U+FFFF, and U+10FC01 to U+10FFFF. A run of supplementary code points still becomes one space, so CJK Extension B ideographs are still removed. Text without a supplementary code point is returned as it is.
readResolvereturns the singleton. TheserialVersionUIDequals theserialverdefault before and after the change, so it stays. The factory Javadoc and the manual row say what changed and that models trained with an earlier release were built with the old output. Those models keep working without retraining under a new process-wide switch,-Dopennlp.compat.mode=LEGACY(opennlp.tools.util.CompatibilityModein opennlp-api, built likeWhitespaceModeand independent of it): underLEGACYthe normalizer produces the 1.x/2.x output, verified against the old pattern on every code point;CURRENTis the default. The introduction chapter documents the property; #1281 uses the same switch for its surrogate rule.TokenSampleStream (English). Tokens are split on runs of Unicode White_Space with a fixed definition (
StringUtil.splitOnUnicodeWhitespace), independent of the whitespace mode; before,split("\\s+")split on ASCII whitespace only. A leading space no longer yields an empty first token with the span (0,0). A line without a token resets the open-quote state. A token holding a letter or digit of any script is a word; before,[A-Za-z0-9]made non-ASCII words attach to the previous token as punctuation. All four are tested.FeatureGeneratorUtil. The
cpclass needs one capital and one period and no other character.A.\nwascpand isicnow; the trailing terminator is only reachable through U+0085 in legacy mode. No other token changes class (all code points compared), so no POS or lemmatizer eval build is needed.ParserTool. Brackets that follow each other are each separated:
a))bgivesa ) ) b, before it gavea ) )b. A line without a bracket is returned unchanged. Whitespace follows the toolkit whitespace mode, as the tokenizer that runs next does.ConlluStream. A multiword token id must be two runs of ASCII digits without a leading zero, joined by one hyphen, with the end not smaller than the start. Any other id fails the read with an
InvalidFormatExceptionnaming the id. Before,3-threwArrayIndexOutOfBoundsException,-4and3--4threwNumberFormatException, and4-3and1-2-3were read without an error.DownloadUtil. A blank checksum file is an
IOExceptionsaying so; before, the model was rejected with "Expected: null". The hash is the first field of a Unicode whitespace split.AD readers (
ADNameSampleStream,ADPOSSampleStream,ADSentenceStream,ADSentenceSampleStream). A leading_in a multiword name no longer yields an empty token; tags, contractions, and NER markup are separated by any whitespace of the toolkit whitespace mode, not only ASCII space; a line of non-ASCII letters is not a punctuation leaf. The training samples change for corpora with such lines. Moving these changes into #1276, which rewrites the same code, is an open decision (see below).Rewrites without a change in output
BioCodec.extractNameType(the old regex and the scan agree on every input tried),DownloadUtilindex page links,BrownClusterlexicon lines, thetext_xxlanguage code ofConlluStream, and the capital and digit features of the POS tagger and lemmatizer context generators, which stay ASCII-only because trained models depend on them.CompatibilityMode
New in opennlp-api:
CompatibilityModewithLEGACYandCURRENT, resolved fromopennlp.compat.modeat startup, overridable withsetActiveandreset, one warning per process underLEGACY, scheduled for removal in 4.0 like the whitespace switch.StringUtil
New helpers:
isAsciiLetter,isAsciiLowerCase,isAsciiDigit,endOfAsciiDigits,isLineTerminator,indexOfLineTerminator(returns -1 when there is none).isLineTerminatoris the setjava.util.regex.Patternuses; it is narrower thanUnicodeWhitespace.lineBreakCodePointSet, which also includes vertical tab and form feed.isLineTerminatoris used by the glob matcher of #1278 as well.Manual
The emoji row of the language detector chapter, the bracket paragraph of the parser tool section, a CoNLL-U section in the corpora chapter, and the AD reader paragraph.
Before merge
ArvoresDeitadasEval(AD readers, tokenizer training input).TokenSampleStream.Open decisions for the reviewers
Verification
opennlp-runtime 2718, opennlp-formats 520, opennlp-cli 60, opennlp-tools 624 tests with checkstyle, offline,
-Dopennlp.forkCount=1. No newjava.util.regeximport, no JDK whitespace predicate.