rzo1 commented on PR #1177:
URL: https://github.com/apache/opennlp/pull/1177#issuecomment-5025685334
> **Automated Claude Opus review. Human review will follow.**
**Overview.** Adds an emoji annotation layer
(`EmojiAnnotation`/`EmojiAnnotations`/`EmojiAnnotator` + derived `EmojiFlags`,
join hook `EmojiAnnotationJoin`) with a bundled 40-symbol data file, and opt-in
wiring into NameFinder (feature-gen factory), Doccat (`FeatureGenerator`), and
Sentiment (factory + context generator). Defaults are untouched and tests pin
that. The layering and fail-loud loading are solid; the findings are mostly the
usual categories: distr licensing, sibling consistency, commentary volume,
constants, and duplication.
## Blocking
- `opennlp-distr/src/main/readme/LICENSE:615` — the binary distribution's
LICENSE enumerates the bundled Unicode files (`confusables.txt`,
`CaseFolding.txt`, `WordBreakProperty.txt`, …) but was not updated for
`emoji-annotations.txt`, while root `LICENSE` and `src/license/NOTICE.template`
were. All three levels must cover the new bundled data — add the same
Unicode-derived-values entry there (and verify the distr NOTICE picks up the
template change).
-
`opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/doccat/EmojiFeatureGenerator.java:120`
— `extractFeatures` throws IAE on `null` text, but the sibling
`BagOfWordsFeatureGenerator` in the same package uses `Objects.requireNonNull`
(NPE) for the same contract. Partial alignment is worse than none: either put
the `null` contract (+ `@throws`) on the `FeatureGenerator` interface and align
all implementations, or drop the check here. As written, swapping generators
changes the exception type. The `@throws IllegalArgumentException` is also
missing on the override's Javadoc.
- Commentary discipline — the "annotations are not `Dimension` constants"
design essay appears **three times** (`EmojiAnnotation.java:35`,
`EmojiAnnotations.java:44`, `EmojiAnnotator.java:29`, the last ~20 lines long)
plus a fourth prose version in `normalizer.xml` and a fifth in the PR body.
Keep one condensed 2–3 line version on `EmojiAnnotator` ("Annotations are
per-symbol metadata, not text transforms; they are a parallel surface beside
`Term`, see OPENNLP-1870.") and delete the rest. Same for the triplicated
inline comment "Every annotatable symbol starts beyond ASCII (audited in
EmojiAnnotationsTest)" (`EmojiFeatureGenerator.java:125`,
`EmojiSentimentContextGenerator.java:57`,
`EmojiAnnotationFeatureGenerator.java:60`) — extract the guard into one shared
helper (e.g. package-private on `EmojiAnnotator`) and comment it once. A reader
in five years does not need the migration rationale in three class headers.
-
`opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/EmojiAnnotations.java:156,246`
— please declare `";"`, `"#"`, and `" "` as constants in this class; the
sibling `EmojiEmoticons` in the same package already defines `FIELD_SEPARATOR`,
`COMMENT_PREFIX`, and `MAPPING_CODE_POINT_SEPARATOR` with Javadoc for exactly
this format. Either reuse or mirror them; also `stripped.split(" ")` runs
inside the per-line loop.
- Feature vocabulary inconsistency — Doccat/Sentiment emit `emojiSentiment=`
/ `emojiEntityType=` (`EmojiFeatureGenerator.java:45-48`,
`EmojiSentimentContextGenerator.java:44-47`, identical duplicated constant
sets), while the NameFinder generator emits `emoji.sentiment=` / `emoji.type=`
(`EmojiAnnotationFeatureGenerator.java:48-51`). These names freeze into trained
models at release; decide one naming scheme now (or document why they differ)
and hoist the shared prefixes to a single location instead of three copies.
- Javadoc — `EmojiSentimentFactory.java:31` and
`EmojiAnnotationFeatureGeneratorFactory.java:36`: public constructors without
Javadoc. The overrides `getContext` (`EmojiSentimentContextGenerator.java:52`),
`extractFeatures`, `createFeatures`, `create()` should carry `{@inheritDoc}`
plus at most one sentence.
## Minor
- `EmojiAnnotator.java:118` — a flag token is decoded twice:
`EmojiFlags.isFlag(symbol)` (lenient decode) then
`isoRegion(symbol).orElseThrow()` (strict decode). Expose the lenient decode
package-private (returning `null`/`Optional`) and call it once; this is the
per-token hot path.
- `EmojiSentimentContextGenerator.java:53` — the base `getContext(String[])`
simply returns `text`; don't re-derive "every token is a feature" with a manual
loop. Use `Collections.addAll(context, super.getContext(text))` so the subclass
can't drift from the default.
- `EmojiSentimentContextGenerator.java:49` — hard-codes `new
EmojiAnnotator()`; the other two generators expose an `(EmojiAnnotator)`
constructor for a join-configured annotator. Add the same seam here, or the
join layer is unreachable from sentiment.
- `EmojiFlags.java:112` — `isRegionalIndicator(int)` is `public` with no
production caller outside the class. New public API is frozen at release; make
it private until a real caller exists.
- `EmojiFlags.java:122` — for input `🇩` + `"E"` the error message says "got
a lone indicator", which is wrong for that case; the length check conflates
"nothing follows" with "a BMP char follows". Reword or split the check.
- `EmojiAnnotations.java:66` — lazy double-checked `volatile` load, while
sibling `EmojiEmoticons` uses an eager `static final INSTANCE` holder. Align
the loading pattern within the package, or say why lazy is needed here.
- Tests: `cp(int...)` is copy-pasted into **five** test classes — extract a
shared test utility. The malformed-input batteries
(`EmojiFlagsTest.malformedRegionalIndicatorSequencesFailLoud`,
`malformedTagSequencesFailLoud`, `EmojiAnnotationsTest.parseFailsLoud*`) should
be `@ParameterizedTest` so the failing input is identifiable. `assertTrue(!…)`
→ `assertFalse` in `EmojiAnnotationsTest` (`notes().isEmpty()`) and
`EmojiAnnotatorTest` (`source().isEmpty()`). `EmojiAnnotation.Value` null
`value`/`source` are untested (only empty).
- `emoji-annotations.txt:16-55` — ~40 lines of design rationale in the
data-file header; trim to format + provenance + coverage claim, the rest
belongs on OPENNLP-1870.
- Serialization: nothing here is `Serializable` and the doccat generator is
manifest-re-instantiated via its no-arg constructor — verified, no
serialVersionUID work needed.
- docbkx: indentation matches the surrounding sections and the claims
(defaults unchanged, manifest re-creation) check out against the code.
## Process
- Defaults are untouched and pinned by tests, so no eval build is strictly
required — but since three components gain new opt-in feature paths, a quick
eval sanity run before merge is still cheap insurance.
- The feature-name freeze (blocking item above) is the one thing that cannot
be fixed post-release; settle it before merge.
- If there are other emoji-layer PRs open for the ticket's follow-ups,
please apply the `cp()` test-utility and constants findings there likewise.
PR title is fine as release-notes language.
--
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]
