rzo1 commented on PR #1164: URL: https://github.com/apache/opennlp/pull/1164#issuecomment-4938605239
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: Blocking / should be addressed: - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/EmojiEmoticons.java`: The emoji-to-emoticon fold matches a mapped pictograph even when it is part of a larger ZWJ sequence or is followed by U+FE0E. For HEART ON FIRE (U+2764 U+FE0F U+200D U+1F525) or COUPLE WITH HEART, the embedded `2764 FE0F` row fires and produces `<3` plus a stray U+200D, corrupting a distinct emoji and leaving a dangling invisible joiner; U+263A U+FE0E similarly folds to `:)` plus a dangling U+FE0E. This contradicts the javadoc claim that no dangling variation selector is left behind. The match should be suppressed when the source is adjacent to U+200D (and arguably when followed by U+FE0E). HEART ON FIRE is common in real social-media text, so this is not a theoretical input. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/EmojiCharSequenceNormalizer.java`: The PR deprecates this class while `LanguageDetectorFactory` (line 52 on main) still wires it into the default language-detector context generator, which the PR does not touch. The build now compiles against a deprecated API with no `@SuppressWarnings` and no migration, and the suggested replacement has unsuitable semantics for that caller: folding emoji to ASCII `:)` would inject ASCII n-grams into language detection where the old normalizer blanked them. Either add a `@SuppressWarnings` with a JIRA-referenced follow-up, or document in the deprecation note that the langdetect default chain intentionally keeps it for model compatibility. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TermAnalyzer.java`: The new per-token EMOJI_FOLD dimension and `TermAnalyzer.Builder.emojiFold()` are completely untested. No test builds a `TermAnalyzer` with the emoji fold, asserts a token's EMOJI_FOLD layer, or checks `Dimension.EMOJI_FOLD.defaultNormalizer()` wiring, even though `DimensionTest` establishes exactly that pattern for the other dimensions. The most interesting behavior (a token layer whose length differs from the token, e.g. one pictograph becoming `:D`) is unverified. Minor: - `opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/util/normalizer/EmojiEmoticonsTest.java`: `candidatesAreSortedLongestFirst` audits only the `emoticonToEmoji` table, but the `emojiToEmoticon` table depends on the same longest-first invariant (`263A FE0F` vs `263A`, `2764 FE0F` vs `2764`). A regression that drops the emoji-direction sort would pass this test while silently leaving dangling U+FE0F selectors behind bare-pictograph matches. Extend the loop to cover both tables. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/EmojiToEmoticonCharSequenceNormalizer.java`: The javadoc claim that no dangling variation selector is left behind only holds for the three pictographs with explicit FE0F rows in `emoji-emoticons.txt`. Input U+1F642 U+FE0F matches only the bare `1F642` row and produces `:)` plus a dangling U+FE0F. Either have the matcher generically absorb a trailing FE0F after a fold, or scope the javadoc claim to mapped sequences; a test for FE0F following a supplementary-plane emoji is also missing. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TermAnalyzer.java`: The same `Dimension.EMOJI_FOLD` rung is exposed under two different builder names: `TermAnalyzer.Builder.emojiFold()` vs `TextNormalizer.Builder.emojiToEmoticon()`. Both are new in 3.0.0, so aligning the names is still free now and impossible after release. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/EmojiToEmoticonCharSequenceNormalizer.java` and `EmoticonToEmojiCharSequenceNormalizer.java`: Serializable singletons without a `readResolve()`, so deserialization silently produces a second instance and reference-equality checks against `getInstance()` fail. A one-line `readResolve()` returning the instance would make the documented contract hold; worth fixing across the new singleton normalizers in this stack. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/EmojiEmoticons.java`: `substitute()`/`substituteAligned()` perform a boxed `HashMap.get(Integer)` for every code point of the input. Recording the min/max first code point in `Tables` and short-circuiting before `table.get()` would make the common no-match case a bare integer compare and remove the per-character boxing in this per-document hot path. 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]
