rzo1 commented on PR #1151:
URL: https://github.com/apache/opennlp/pull/1151#issuecomment-4938603956
Hi @krickert - as mentioned on Slack I currently dont have the time for a
manual review but I just let Fable do a comprehensive review on this PR. Here
is the result:
-
`opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/stream/SpellCorrectingTokenStream.java`:
The hand-rolled indexOf split loop that replaces `Pattern.split(line, -1)`
looks correct but has no tests for the edge cases where it could regress.
Existing tests only cover single-char delimiters and a mid-line duplicate
delimiter; please add cases for a leading delimiter, a trailing delimiter
(where the old split with limit -1 produced empty edge tokens that must be
preserved on re-join), and a multi-char delimiter. This is the one conversion
in the PR without characterization coverage.
-
`opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TwitterCharSequenceNormalizer.java`:
The `isAsciiWord` emulation of `\b` pins JDK 21+ semantics. On JDK 17 and
earlier the old regex behaved differently for non-ASCII word chars (e.g. "ért
x" stays unchanged on JDK 8/17 but becomes "é x" with the new scan and with the
regex on JDK 21+). Since the project requires Java 21 the byte-identical claim
holds on supported runtimes, but the comment referencing
`java.util.regex.Pattern.Bound` and the characterization tests should note this
version dependency in case the baseline is ever lowered.
-
`opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TwitterCharSequenceNormalizer.java`:
`normalize(null)` now throws IllegalArgumentException instead of the
historical NullPointerException, and the new contract is inconsistent within
the package (Twitter/Shrink/Url/Number throw IAE, untouched siblings like
EmojiCharSequenceNormalizer still throw NPE, and the `CharSequenceNormalizer`
interface javadoc implies NPE). Either keep NPE for compatibility or lift the
`@throws` documentation to the interface and align the package.
-
`opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TwitterCharSequenceNormalizer.java`:
Each cursor pass unconditionally builds a full StringBuilder copy plus
toString even when nothing matched, whereas the old `Matcher.replaceAll`
returned the input without copying on no-match text. With up to four passes per
Twitter normalize call (and two each for Url and Shrink) this adds avoidable
copying in the language-detector chain; track whether anything changed and
return the original text when unchanged. Same pattern in
`UrlCharSequenceNormalizer` and `ShrinkCharSequenceNormalizer`.
-
`opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java`:
The `isNumberLike` rewrite of the former `[+-]?[\d.,]*\d[\d.,]*%?` guard is
only tested on the accept side. Add reject-side cases where the scan structure
differs from the regex (e.g. "5%%", "+%", "1,2a", a lone "+") or a differential
test like the four runtime normalizers have, since a false positive would
silently exempt real words from spell correction.
-
`opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TwitterCharSequenceNormalizer.java`:
Verbatim duplication in two spots: the `ASCII_WHITESPACE` CodePointSet
constant plus the `asciiToLower`/`asciiCaseInsensitiveEquals` helpers are
copied between Twitter and `ShrinkCharSequenceNormalizer`, and the
`randomInput`/`escape` test helpers are copy-pasted identically into all four
new characterization test classes. Shared package-private helpers would prevent
the copies drifting apart.
-
`opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/ShrinkCharSequenceNormalizer.java`:
The self-referential javadoc opening ("A {@link ShrinkCharSequenceNormalizer}
implementation that...") was fixed in `NumberCharSequenceNormalizer` but
remains in the Shrink, Twitter, and Url normalizers even though their javadoc
was otherwise rewritten in this PR; apply the same one-word fix there.
Human review will follow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
