rzo1 commented on PR #1154:
URL: https://github.com/apache/opennlp/pull/1154#issuecomment-4938604387
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:
**Correctness / API design**
-
`opennlp-extensions/opennlp-geo/src/main/resources/opennlp/geo/naturalearth-populated-places.txt`:
Upstream Natural Earth mojibake is carried verbatim into the bundled table,
e.g. indexed names like `AmundseniScott South Pole Station`, `EdDamer`, `Sdid
Bouzid` and roughly 20 containment values like `ThMi Bmnh`, `MUdenine`,
`Kasssrine`. Corrupted indexed names are silently unfindable by their correct
spelling, the header's derivation record ("those containment elements are
omitted") is inaccurate for these rows, and the audit test (ASCII/shape checks
only) cannot detect letter-level corruption.
- `opennlp-api/src/main/java/opennlp/tools/geo/Geocoder.java`: The new
Geocoder/Gazetteer seam duplicates the documented use case of the existing
non-deprecated `opennlp.tools.entitylinker` API (whose Javadoc cites gazetteer
location lookup as its primary example) with no cross-reference, deprecation,
or guidance on which contract new consumers should target.
- `opennlp-api/src/main/java/opennlp/tools/geo/Gazetteer.java`: The
lookup/byId/byRegion/resolve methods document no failure mode for I/O or
backend errors even though the Javadoc explicitly anticipates database-backed
and remote-service implementations. Each implementation will invent its own
undocumented unchecked exception, callers cannot handle backend failure
portably, and a checked exception channel cannot be added compatibly after
release (the existing EntityLinker declares IOException).
- `opennlp-api/src/main/java/opennlp/tools/geo/GazetteerEntry.java`:
Interop-critical string vocabularies (featureClass `CITY`/`ADMIN`/`POI`,
attribute keys `fips`/`geoid`/`zcta`/`wikidata`/`geonames`/`whosonfirst`,
source marker `UNSPECIFIED`) exist only as Javadoc conventions, yet
`CandidateRanking` hard-codes the featureClass literals, so a third-party
Gazetteer with different spellings (e.g. `City`, `PPL`) silently loses the
feature-class tie-breaking prior in `PopulationPriorGeocoder`. These should be
public constants or an enum in `opennlp.tools.geo`.
- `opennlp-api/src/main/java/opennlp/tools/geo/Gazetteer.java` (also
`GazetteerEntry.java`, `AttributeValue.java`, `BundledDataAuditTest.java`):
Published API Javadoc repeatedly references an "emoji annotation layer" and a
test comment names a class `EmojiAnnotationJoin`, but neither exists anywhere
in the codebase. These references should be dropped or phrased as
forward-looking.
- `opennlp-extensions/opennlp-geo/pom.xml`: The jar will bundle the Natural
Earth-derived data with only the generic Apache-2.0 `META-INF/LICENSE` from
maven-remote-resources; the new Natural Earth public-domain section exists only
in the root LICENSE. The Maven Central jar is itself an ASF distribution
bundling third-party data, so its LICENSE should note the public-domain
material when the promised LICENSE wiring lands.
**Minor**
-
`opennlp-extensions/opennlp-geo/src/main/java/opennlp/geo/PopulationPriorGeocoder.java`:
`confidence()` can overflow long arithmetic (`first + second`, `first -
second`) for populations near `Long.MAX_VALUE`, producing a confidence outside
[0, 1] that makes the `GeoResolution` constructor throw. Requires an
adversarial custom Gazetteer; computing the ratio in double or clamping closes
it.
- `opennlp-api/src/main/java/opennlp/tools/geo/GazetteerEntry.java`:
`population` uses 0 as the sentinel for "unknown", conflating missing data with
a genuinely zero population; this feeds directly into the confidence formula
and cannot be changed compatibly after release, so the choice should be made
deliberately now.
-
`opennlp-extensions/opennlp-geo/src/main/java/opennlp/geo/BundledGazetteer.java`:
The only public entry point is a permanent JVM-wide static singleton
(`getInstance()`) with a package-private constructor and parser, so users
cannot build a gazetteer over their own rows in the documented table format or
release the 7,342-row index. A public constructor/factory taking entries would
give the same convenience without locking in the singleton.
-
`opennlp-extensions/opennlp-geo/src/main/java/opennlp/geo/BundledGazetteer.java`:
`indexName` silently skips a name whose fold key is empty, so a
punctuation-only name would load but be unreachable by lookup, contradicting
the module's fail-loud stance; either reject such rows or add an audit
assertion that every bundled name yields a non-empty fold key.
-
`opennlp-extensions/opennlp-geo/src/main/java/opennlp/geo/PopulationPriorGeocoder.java`:
`resolve()` copies and re-sorts the candidate list for every mention even
though `BundledGazetteer.lookup` already returns lists pre-sorted with the same
comparator; skipping the copy/sort for size-1 lists or a known-trusted
gazetteer would take this out of the per-mention hot path.
-
`opennlp-extensions/opennlp-geo/src/test/java/opennlp/geo/BundledGazetteerTest.java`:
Several documented fail-loud parser behaviors have no test, e.g. duplicate
attribute keys, empty pipe-list elements from leading/trailing/doubled pipes,
and out-of-range longitude (latitude has a test, longitude does not).
-
`opennlp-extensions/opennlp-geo/src/test/java/opennlp/geo/BundledDataAuditTest.java`:
The class Javadoc claims the data file header's claims are enforced, but the
exact "7342 rows" claim is only checked as a >= 7000 floor, so a regeneration
with a stale header passes; assert the exact count or parse it from the header.
-
`opennlp-extensions/opennlp-geo/src/main/resources/opennlp/geo/naturalearth-populated-places.txt`:
The file carries the standard ASF Apache-2.0 header even though it is
substantively a derivation of a third-party public-domain dataset; per ASF
source-header policy the ASF header should likely be dropped in favor of the
existing derivation-record comment plus the LICENSE pointer.
-
`opennlp-extensions/opennlp-geo/src/main/resources/opennlp/geo/naturalearth-populated-places.txt`:
The ~950 KB generated table is committed without the extraction/transformation
script, so refreshing to a newer Natural Earth release means re-implementing
the documented rules by hand; please attach or commit the generation tooling.
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]
