OPENNLP-1975: Regex removal (9/10): Correct the output of TokenSampleStream, ParserTool, ConlluStream, DownloadUtil and FeatureGeneratorUtil by rzo1 · Pull Request #1314 · apache/opennlp

rzo1 · 2026-09-18T10:10:05Z

Part 9 of the regex removal (OPENNLP-1926). Split out of #1275. Based on main; part 1 (#1311, OPENNLP-1928) is merged.

These sites lose their regular expressions like the rest of the epic, but the scans follow what the code was meant to do instead of copying the regex quirks, so the output changes. Each corrected case is pinned by a test.

Behavior changes

TokenSampleStream (English). Tokens are split on runs of Unicode White_Space (StringUtil.splitOnUnicodeWhitespace), independent of the whitespace mode; before, split("\\s+") split on ASCII whitespace only. A leading space no longer yields an empty first token with the span (0,0). A line without a token resets the open-quote state. A token holding a letter or digit of any script is a word; before, [A-Za-z0-9] made non-ASCII words attach to the previous token as punctuation.

ParserTool. Brackets that follow each other are each separated: a))b gives a ) ) b, before a ) )b. A line without a bracket is returned unchanged. Whitespace follows the toolkit whitespace mode. Documented in the parser chapter.

ConlluStream. A multiword token id must be two runs of ASCII digits without a leading zero, joined by one hyphen, with the end not smaller than the start. Any other id fails with an InvalidFormatException naming the id. Before, 3- threw ArrayIndexOutOfBoundsException, -4 and 3--4 threw NumberFormatException, and 4-3 and 1-2-3 were read without an error. The text_xx language code is read with a scan, same output. New CoNLL-U section in the corpora chapter.

DownloadUtil. A blank checksum file is an IOException saying so; before, the model was rejected with "Expected: null". The index page links are read with a scan, same output.

FeatureGeneratorUtil. The cp class needs one capital and one period and no other character. A.\n was cp and is ic now. No other token changes class.

Before merge

Tokenizer eval build that reads through TokenSampleStream.

Commits by @krickert from #1275, regrouped.

krickert · 2026-09-20T22:01:33Z

eval should read its samples through opennlp.tools.tokenize.lang.en.TokenSampleStream, since that is the class changed here. I couldn’t find an existing eval using it, so we should (w/the command, corpus, and before/after etc etc etc..)

I don't mind doing this it's a quick task.

krickert

I don't mind doing the changes - if they're not in by tomorrow I'll push it :)

HTML scanning
Unicode capitalization
Eval tokenizer stuff

krickert · 2026-09-20T23:28:53Z

Actually, I'll just fix it :) I see it's not a fork.

krickert · 2026-09-20T23:46:05Z

Made all the changes requested from my own review. Ready for a review @rzo1 and @mawiesne

krickert · 2026-09-20T23:49:19Z

I had to merge since main had corpora.xml touched since. I'll run the eval once the other eval is done in about 20 minutes.

Replace the remaining scans in the English token sample reader, parser bracket spacing, CoNLL-U ranges, download links and checksums, and token feature classification. Include single-quoted HTML links, Unicode capital-period features and the English token sample stream evaluation. Squash the reviewed public history without changing its final tree. Hosted Java CI passed on the original head 3e43b02. The evaluation source is present; a completed evaluation run still needs confirmation before merge.

rzo1 · 2026-09-30T18:56:44Z

@krickert This is your change, I only ported it to a branch on apache/opennlp (because of the org-fork thing) so we can both push to it. Could you please re-review it?

I rebased it onto current main. There were no conflicts and no content changes, and the tests for the touched classes pass. Your fixes from Sep 20 are all in. If anything is missing or you'd like to change something, feel free to push directly to the branch.

krickert · 2026-09-30T22:54:46Z

@krickert This is your change, I only ported it to a branch on apache/opennlp (because of the org-fork thing) so we can both push to it. Could you please re-review it?

I rebased it onto current main. There were no conflicts and no content changes, and the tests for the touched classes pass. Your fixes from Sep 20 are all in. If anything is missing or you'd like to change something, feel free to push directly to the branch.

Thank you. And sorry about not putting it on an Apache branch first. I'll do that for now on.

rzo1 · 2026-10-01T11:54:17Z

https://ci-builds.apache.org/job/OpenNLP/job/eval-tests-configurable/93/

jzonthemtn · 2026-10-01T12:33:33Z

+    }
+    final int first = token.codePointAt(0);
+    final int periodOffset = Character.charCount(first);
+    return Character.isUpperCase(first) && periodOffset + 1 == token.length()


This widens cp a bit. The old pattern was ^[A-ZÄÖÜ]\.$, but Character.isUpperCase accepts capitals from any script. So É., Ñ., etc. change from ic to cp, and supplementary-plane capitals change from other to cp.

That changes the features existing NameFinder models see for non-English text. Could we limit this PR to A–Z plus Ä/Ö/Ü and propose the Unicode change separately?

Open to other ideas, too.

Typically what we've been doing is to have a LEGACY mode switch for the old models and support unicode for 3.0. We have this switch already with a few other features - I'll go ahead and push a commit to it to see what you think (I can easily back off it too - just quicker to show you what I mean).

I'd rather support more characters as most other features support international as well and it would be a strange one-off to support an old way that's more ASCII focused.

Pushed the switch.

In the default mode a single capital of any script followed by a period is cp. Under -Dopennlp.compat.mode=LEGACY the old set applies, A to Z plus Ä Ö Ü, including the one trailing line terminator the old $ accepted, so a model trained before 3.0.0 gets what it was trained with.

I had a test written first, then applied change; the legacy rows pin É., Ω., Я. as ic and the line-terminator cases.

For the models in the eval suite nothing moves, since their initials are ASCII or umlauts. The only tokens that change class are initials from other scripts, like Я., and only in the default mode.

This is the same approach as the emoji normalizer in #1313: we made Unicode is default for 3.0, and the switch is what keeps old models whole.

An ASCII-only rule here would be the one exception to that, which is why I pushed back.

Thanks @krickert, the LEGACY switch looks good to me. It reproduces the old ^[A-ZÄÖÜ].$ behaviour exactly, including the trailing line terminator, and it's consistent with what we did for the emoji normalizer in #1313. @jzonthemtn, does that work for you? If so, please resolve the thread.

One blocker before merge: eval #93 (https://ci-builds.apache.org/job/OpenNLP/job/eval-tests-configurable/93/) on 4ea6f1c has a failure:

ConllXPosTaggerEval.evalSwedishMaxentQn
expected: <0.9377652050919377> but was: <0.934052333804809>

@krickert Can you take a look ?

jzonthemtn · 2026-10-01T12:37:04Z

Just the one comment in FeatureGeneratorUtil.java.

Under opennlp.compat.mode=LEGACY a token is cp only for an ASCII capital or a German umlaut capital followed by a period, with at most one line terminator after it, as before 3.0.0.

…y mode FeatureGeneratorUtil classifies a single capital followed by a period as cp for any script in the default mode. Under opennlp.compat.mode=LEGACY the old set applies, A to Z and the three German umlaut capitals, with the one trailing line terminator the old pattern accepted, so models trained before 3.0.0 see the features they were trained with.

krickert · 2026-10-01T21:15:12Z

@rzo1 looking into it now -

The cp token class accepts a capital of any script since this change, and the Swedish talbanken05 training data holds one token of that shape outside the old set, "Å.". With that feature the quasi-Newton trainer converges to a slightly different model, 0.934052333804809 instead of 0.9377652050919377 on the test set. The GIS model trained on the same features keeps its pinned accuracy, and the old value returns under opennlp.compat.mode=LEGACY, so the difference is the optimizer path, not a worse feature.

krickert · 2026-10-01T22:25:20Z

Looked at eval 93.

The Swedish quasi-Newton drop comes from this PR, and it is the cp change: the default POS feature set includes TokenClassFeatureGenerator, and the Swedish talbanken05 training data has exactly one token of the new kind, Å. (the old set had Ä Ö Ü but not Å). The test set has none. With that one feature different, the quasi-Newton trainer converges to a slightly different model, 0.93405 instead of 0.93777.

Three checks say it is the optimizer path and the feature carries the same information as before: the Swedish GIS model trained on the same features keeps its pinned accuracy; main at this PR's base and main before #1321 both reproduce the old value locally; and this head under -Dopennlp.compat.mode=LEGACY returns the old value to the last digit.

So I re-pinned the QN expectation to the new value, with that reasoning in the commit. The eval needs one more run on the new head.

I'll re-run the eval - it should pass now.

krickert · 2026-10-01T22:28:03Z

https://ci-builds.apache.org/job/OpenNLP/job/eval-tests-configurable/94/console

Eval build running.

rzo1 · 2026-10-02T04:14:39Z

Thanks for the reasoning, @krickert
Let's take it.

mawiesne

Thx @rzo1 and @krickert. Please improve the PR for code quality aspects.

Moreover, it seems several methods don't require static or could be private instead of package-private. Could not check em all. Please sharpen.

mawiesne · 2026-10-02T04:07:45Z

+    try (ObjectStream<ConlluSentence> stream = getStream("es-ud-sample.conllu")) {
+      ConlluSentence sent1 = stream.read();
+
+      Assertions.assertEquals(55, sent1.getWordLines().size());


Please extract sent1.getWordLines() into a local variable and reduce n duplications below. Check it for null before checking by positional index.

mawiesne · 2026-10-02T04:13:12Z

      if (sb.length() != 0) {
-        if (!alphaNumeric.matcher(token).find() || token.startsWith("'") || token.equalsIgnoreCase("n't")) {
+        if (!containsLetterOrDigit(token) || token.startsWith("'") || token.equalsIgnoreCase("n't")) {
          if ((token.equals("``") || token.equals("--") || token.equals("$") ||


Extract constants here for the symbols checked. Check elsewhere if loops are coded around such constants.

mawiesne · 2026-10-02T04:14:26Z

  }

+  /**
+   * Tests whether a token contains a letter or a decimal digit, by code point. A token without


Replace "Tests" with "Checks" here.

mawiesne · 2026-10-02T04:17:15Z

+   */
  private static void verifyChecksum(Path model, String expectedChecksum) throws IOException {
+    if (expectedChecksum == null) {
+      throw new IOException("The checksum file for " + model.getFileName() + " is blank");


That must be IllegalArgumentException, no IO... for parameter checks! Please change and update Javadoc accordingly. Please check elsewhere.

mawiesne · 2026-10-02T04:19:18Z

+    }
    final String actualChecksum = calculateSHA512(model);
    if (!actualChecksum.equalsIgnoreCase(expectedChecksum)) {
      throw new IOException("SHA512 checksum validation failed for " + model.getFileName() +


IllegalStateException here. Update Javadoc.

rzo1 self-assigned this Sep 18, 2026

rzo1 requested review from jzonthemtn, krickert and mawiesne September 18, 2026 10:10

This was referenced Sep 18, 2026

OPENNLP-1928: Regex removal (1/10): Replace trivial patterns with character scans #1311

Merged

OPENNLP-1928: Regex removal (1/8): Replace trivial patterns with character scans in StringUtil #1275

Closed

rzo1 marked this pull request as draft September 18, 2026 10:12

rzo1 assigned krickert Sep 18, 2026

rzo1 force-pushed the OPENNLP-1975-regex-removal-output-fixes branch from a6fd65a to 596cf58 Compare September 18, 2026 11:17

rzo1 force-pushed the OPENNLP-1928-regex-removal-scans branch from 1d1b66a to 33581d0 Compare September 18, 2026 11:17

rzo1 force-pushed the OPENNLP-1975-regex-removal-output-fixes branch 2 times, most recently from 4b26964 to 6297884 Compare September 18, 2026 13:29

Base automatically changed from OPENNLP-1928-regex-removal-scans to main September 18, 2026 17:04

rzo1 force-pushed the OPENNLP-1975-regex-removal-output-fixes branch from 6297884 to bbcce4a Compare September 18, 2026 17:07

rzo1 marked this pull request as ready for review September 20, 2026 15:55

krickert requested changes Sep 20, 2026

View reviewed changes

krickert force-pushed the OPENNLP-1975-regex-removal-output-fixes branch from 3e43b02 to 0a2c0f9 Compare September 21, 2026 00:53

rzo1 requested a review from krickert September 29, 2026 17:42

rzo1 force-pushed the OPENNLP-1975-regex-removal-output-fixes branch from 0a2c0f9 to 4ea6f1c Compare September 30, 2026 18:53

rzo1 added this to the 3.0.0 milestone Sep 30, 2026

krickert approved these changes Sep 30, 2026

View reviewed changes

jzonthemtn reviewed Oct 1, 2026

View reviewed changes

krickert added 2 commits October 1, 2026 10:12

OPENNLP-1975: Test the capital-period token class under the legacy mode

61d2410

Under opennlp.compat.mode=LEGACY a token is cp only for an ASCII capital or a German umlaut capital followed by a period, with at most one line terminator after it, as before 3.0.0.

krickert mentioned this pull request Oct 1, 2026

OPENNLP-1935: Regex removal (10/10): Forbid java.util.regex in production code #1348

Merged

rzo1 merged commit 55d1dd0 into main Oct 2, 2026
10 checks passed

rzo1 deleted the OPENNLP-1975-regex-removal-output-fixes branch October 2, 2026 04:14

mawiesne requested changes Oct 2, 2026

View reviewed changes

rzo1 mentioned this pull request Oct 2, 2026

OPENNLP-1975: Address follow-up review comments from #1314 #1351

Merged

mawiesne pushed a commit that referenced this pull request Oct 2, 2026

OPENNLP-1975: Address follow-up review comments from #1314 (#1351)

86bab9d

Conversation

rzo1 commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Behavior changes

Before merge

Uh oh!

krickert commented Sep 20, 2026

Uh oh!

krickert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krickert commented Sep 20, 2026

Uh oh!

krickert commented Sep 20, 2026

Uh oh!

krickert commented Sep 20, 2026

Uh oh!

rzo1 commented Sep 30, 2026

Uh oh!

krickert commented Sep 30, 2026

Uh oh!

rzo1 commented Oct 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzonthemtn commented Oct 1, 2026

Uh oh!

krickert commented Oct 1, 2026

Uh oh!

krickert commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krickert commented Oct 1, 2026

Uh oh!

rzo1 commented Oct 2, 2026

Uh oh!

Uh oh!

mawiesne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rzo1 commented Sep 18, 2026 •

edited

Loading

krickert commented Oct 1, 2026 •

edited

Loading