OPENNLP-1869: Add emoji and emoticon normalizers to text normalization#1164
OPENNLP-1869: Add emoji and emoticon normalizers to text normalization#1164krickert wants to merge 3 commits into
Conversation
|
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:
Minor:
Human review will follow. |
Adds an opt-in, offset-aware fold between emoji and ASCII emoticons in both directions, backed by a bundled, project-authored provenance-tagged table (emoji-emoticons.txt; no third-party data set is copied). - EmojiEmoticons: lazy loader plus the shared longest-match sequence substitution pass; both directions substitute code point sequences (an emoticon source is multi-character, an emoji-presentation source is a pictograph plus U+FE0F folding as one unit), which the per-code-point CharClass primitive cannot express. - EmojiToEmoticonCharSequenceNormalizer: emoji to canonical ASCII emoticon, many to one, unguarded (a pictograph is unambiguous in running text). - EmoticonToEmojiCharSequenceNormalizer: emoticon to canonical pictograph, folding only a whitespace-delimited unit so embedded sequences such as the :/ in https:// are never corrupted; a missed fold costs nothing, a false fold is irreversible. - The two directions close over each other (audited by test), so a round trip converges on canonical forms. - Wiring: Dimension.EMOJI_FOLD (emoji-to-emoticon, the per-token-meaningful direction), TermAnalyzer.Builder.emojiFold(), TextNormalizer.Builder emojiToEmoticon()/emoticonToEmoji(); both rungs compose into buildAligned(). - Deprecates EmojiCharSequenceNormalizer: its surrogate-block pattern blanks every supplementary-plane code point (not only emoji) and misses BMP pictographs, and deleting the symbol destroys the signal downstream. - Settles the epic's WordType question with no tokenizer change: folding emoticons to emoji before tokenization makes both one EMOJI class, proven by test against the UAX #29 word tokenizer. - Manual updates in the Text Normalization chapter.
…ng, emoji fold coverage The pictographic fold is now sequence-aware: a mapped pictograph does not fold when it adjoins a ZWJ sequence (HEART ON FIRE and the family emoji pass through intact instead of folding an embedded heart and leaving a dangling joiner) or when followed by U+FE0E, which requests text presentation. A trailing U+FE0F after any mapped pictograph is absorbed into the fold, so the no-dangling-selector guarantee now holds for every mapped source, not only those with explicit selector rows; the alignment maps the absorbed selector as part of the replaced block. Tests cover all of these cases in both plain and aligned form. The default language detector chain keeps the deprecated emoji normalizer deliberately: its models were trained with pictographs blanked, and folding to ASCII emoticons would inject n-grams they never saw. That is now documented in the deprecation note and suppressed at the use site. Adds the missing TermAnalyzer emoji fold coverage (EMOJI_FOLD layer on a pictograph token, pass-through on plain tokens, default normalizer wiring), extends the longest-first table audit to both directions, and short-circuits the per-code-point map lookup with first-code-point range bounds.
|
Thanks, the ZWJ finding was a real bug. Addressed in d21612e (branch also rebased onto the current OPENNLP-1868 head): Blocking:
Minor:
|
There was a problem hiding this comment.
Thanks - functionally this looks good. Requesting changes before this can go in:
1. PR title
The title describes the implementation ("fold rungs", "bidirectional", "offset-aware"), not the feature. Please rename to something a reader of the release history understands without knowing the internal vocabulary, e.g.:
OPENNLP-1869: Add emoji/emoticon normalizers to text normalization
2. Please cut the commentary down substantially
Large parts of the added prose explain rationale and design history rather than the code, and that belongs in the JIRA issue / PR description, not in the sources where it goes stale. Examples: the three-paragraph class javadoc essays on EmojiEmoticons, EmojiToEmoticonCharSequenceNormalizer and EmoticonToEmojiCharSequenceNormalizer; the inline block in LanguageDetectorFactory.getContextGenerator(); the multi-paragraph @deprecated text on EmojiCharSequenceNormalizer; the ~25-line header essay in emoji-emoticons.txt. Please reduce class javadoc to a few sentences stating what the class does and the one or two contracts a caller must know (emoticon direction only folds whitespace-delimited units; emoji direction skips ZWJ sequences). The deprecation note needs one sentence plus the replacement link. The design narrative ("a missed fold costs nothing while a false fold is irreversible", "no published standard defines…", epic cross-references) should move to OPENNLP-1869.
3. Avoid static where it isn't needed
EmojiEmoticons is an all-static utility with a volatile field and hand-rolled double-checked locking. Please restructure this as a plain instance: load the tables once (e.g. in the private constructors of the two normalizers, or an EmojiEmoticons instance they share) and hold them in final fields. That removes the mutable static, the DCL boilerplate, and makes the data dependency explicit.
4. Javadoc on every method
Several methods have none or use // comments where javadoc belongs: emojiToEmoticon() / emoticonToEmoji() in EmojiEmoticons, Direction.candidates(int), matchAt (block comment above the signature), parse (block comment instead of javadoc). Please add proper javadoc throughout, including package-private methods — this package is new API and the next maintainer shouldn't have to reverse-engineer the (index << 32) | consumed encoding from a side comment.
5. {@inheritDoc} on overrides
normalize(CharSequence) and normalizeAligned(CharSequence) in both normalizers override interface methods but carry no javadoc. Please add {@inheritDoc}, plus a sentence only where the implementation adds a caller-relevant detail.
6. Argument validation
The null checks sit only on the internal EmojiEmoticons.substitute/substituteAligned. Please validate at the public API boundary instead: normalize / normalizeAligned in both normalizers should Objects.requireNonNull(text, "text must not be null"), and parse(InputStream) should reject a null stream. Matching the parameter-validation style used elsewhere in opennlp.tools.util.normalizer.
krickert
changed the title
…erage, boundary validation
2 participants
Summary
Stacked on #1138 (OPENNLP-1868); merge bottom-up. When #1138 merges, retarget this PR to main before deleting its branch.
Adds the bidirectional emoji/emoticon fold to the normalizer stack:
emojiToEmoticon()/emoticonToEmoji()rungs in the canonical builder order.Test plan
EmojiEmoticonsTest,EmojiToEmoticonCharSequenceNormalizerTest,EmoticonToEmojiCharSequenceNormalizerTestopennlp-apiandopennlp-runtimesuites green (1,380 tests)https://issues.apache.org/jira/browse/OPENNLP-1869