rzo1 commented on PR #1161: URL: https://github.com/apache/opennlp/pull/1161#issuecomment-4938604840
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-api/src/main/java/opennlp/tools/util/normalizer/CodePoints.java`: The PR description claims "No API changes", but this adds new public API to opennlp-api: the public class `CodePoints`, its public nested record `At`, and the public `Alignment.Builder(int)` constructor. `CodePoints` must be public for cross-module use (WordSegmenter, WordType, Confusables), but once shipped it becomes compatibility-bearing surface. Either annotate it with the existing `opennlp.tools.commons.Internal` annotation, relocate it to a non-exported package, or accept it as deliberate new API and update the PR summary accordingly. - `opennlp-api/src/main/java/opennlp/tools/util/normalizer/CharClass.java`: `foldUnlessClean` allocates a `CodePoints.At` record per code point and dispatches it through `MemberFold.apply`, a call site shared by three lambda classes (normalize, collapse, removeAll). If the site goes megamorphic in mixed workloads, inlining and scalar replacement fail, yielding one heap allocation plus a virtual dispatch per code point where the old loops were allocation-free primitives. A single-operation JMH run is monomorphic and would not expose this, so the "neutral on fold-saturated text" result does not rule out a regression; a mixed-operation benchmark or reverting the fold body to primitives is warranted before merge. - `opennlp-api/src/main/java/opennlp/tools/util/normalizer/CodePoints.java`: No dedicated test exists for the new class. The low-surrogate branch of `before()` is never verified: all trim tests end their reverse scan on BMP characters, so a defect in that branch (wrong code point or charCount) would pass the full suite. A small direct test of `at()`/`before()` with a paired supplementary character, lone high surrogate, lone low surrogate, and BMP chars would close this. - `opennlp-api/src/main/java/opennlp/tools/util/normalizer/CodePoints.java`: If the type stays public, two design issues should be fixed now while the class is new: it is a general text-scanning utility placed in the normalizer package but imported by tokenizer code (`opennlp.tools.util` would fit better), and `At.nextIndex(int)` vs `At.previousIndex(int)` have asymmetric index semantics (start index vs exclusive end index) with nothing tying an `At` to its decode direction, inviting silent misuse. - `opennlp-api/src/main/java/opennlp/tools/util/normalizer/CodePoints.java`: The BMP fast path in `at()` duplicates a check `Character.codePointAt` already performs, so the conversions in `WordSegmenter.forEachSegment` and `WordType.of` add record construction in the tokenizer's hottest loops for at best neutral gain, relying on escape analysis to stay allocation-free. Should be confirmed by the WordSegmenter JMH numbers rather than assumed. - `opennlp-api/src/main/java/opennlp/tools/util/normalizer/CharClass.java`: In `normalize()`'s `MemberFold` lambda the parameter named `ignoredIndex` is actually used (`cp.nextIndex(ignoredIndex)`). Rename it, or simplify the four-argument `MemberFold` shape since only `collapse()` needs the `text` parameter. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/Confusables.java`: The local `final Data data = data()` in `skeleton()` shadows the static volatile field `data`; renaming the local (e.g. `d`, as in the `data()` accessor) removes the hazard. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/DigitCharSequenceNormalizer.java`: The aligned-path behavior change (ASCII digits now recorded as `equal(1)` instead of `replace(1,1)`) is equivalent today but only justified by a code comment; the new test covers only the plain `normalize()` fast path. A `normalizeAligned()` test on mixed ASCII/non-ASCII digit text asserting the span mapping would pin the equivalence against future `Alignment` changes. 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]
