[henriquejsza]

Thank you for contributing to Apache OpenNLP.

For all changes:

• Is there a GitHub issue associated with this PR? Is it referenced in the PR description (e.g. "Fixes Regenerated NOTICE file after dependency changes #1234")?
• Has your PR been rebased against the latest commit within the target branch (typically main)?
• Is your initial contribution a single, squashed commit?

For code changes:

• Have you ensured that the full suite of tests is executed via mvn clean install at the root opennlp folder?
• Have you written or updated unit tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0? (no new dependencies)
• If applicable, have you updated the LICENSE file, including the main LICENSE file in opennlp folder? (not applicable)
• If applicable, have you updated the NOTICE file, including the main NOTICE file found in opennlp folder? (not applicable)

Fixes #1342

DependencyContextGenerator joined the word-tag and position values of a
combined feature with / and | without escaping them. A value holding one
of these characters could make a combined feature of one configuration read
the same as that of another, for example s1t=X|Y, s0t=Z and s1t=X, s0t=Y|Z
both produced s1t,s0t=X|Y|Z.

This escapes the escape character \, /, and | in every value that is
joined into a combined feature, so each combined feature is read back
unambiguously. Single-position features carry one value, so a separator there
cannot split the feature and the value is left as is. Values without any of
these characters are returned unchanged.

The dependency parser is new in 3.0 and no models have shipped yet, so there is
no released model to keep compatible. Universal POS tags never contain / or
|, so the trained-model feature strings only change for tokens that contain
one of these characters; the pinned UniversalDependencyParserEval scores
should be re-run on a newly trained model.

[krickert]

@henriquejsza thank you for this PR. I have time to look it over this weekend (within 48 hours). Thank you for your contribution and appreciate the time you put into this!

I'll also get you an answer about the model training.

[krickert]

Just a few minor nits

I got around to reviewing this - it's a clean fix and the tests read well.

Two small nits:

• escape could be a plain private method like the other helpers in the class, no need for static.
• The Javadoc says the feature "is read back unambiguously", but nothing reads it back. Maybe "stays unambiguous".

[krickert]

On the model question: the dependency parser is new in 3.0 and we haven't released a model for it, so nothing published needs retraining.

The only thing affected is UniversalDependencyParserEval, which trains the parser from the UD treebanks during the run and checks UAS and LAS against fixed numbers. Those corpora only live on our Jenkins node, so you can't run it locally.

@mawiesne - once we get someone a second review I'll start the eval job change and post the numbers here. If they moved, I'll share the numbers for @henriquejsza to update the expected scores.

[rzo1]

LGTM once krickert's nits are in. Separate issue: a token that is literally *NULL* or *ROOT* still collides with the marker values.

[rzo1]

Nothing reads this back. "stays unambiguous"?

[rzo1]

Doesn't need to be static, the other helpers aren't.