Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new hasPid query option so geocoding results can be restricted to matches that have parcel identifiers (PIDs), and tightens PID handling during site-loader preparation to skip blanks and avoid emitting empty PID mappings.
Changes:
- Add
hasPidparameter toGeocodeQueryand apply PID-based filtering inGeocoder.geocode(). - Add datastore helper to determine whether a match has PIDs.
- Improve PID ingestion/output in
SiteLoaderPrep(skip blank PIDs, avoid writing empty PID strings).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ols-geocoder-process/src/main/java/ca/bc/gov/ols/siteloaderprep/SiteLoaderPrep.java | Filters out blank PIDs and avoids writing empty PID mappings; adds PID dedupe behavior. |
| ols-geocoder-core/src/main/java/ca/bc/gov/ols/geocoder/GeocoderDataStore.java | Introduces PID-presence check for geocode matches. |
| ols-geocoder-core/src/main/java/ca/bc/gov/ols/geocoder/Geocoder.java | Filters initial matches by PID when requested and suppresses fallback when hasPid=true. |
| ols-geocoder-core/src/main/java/ca/bc/gov/ols/geocoder/api/GeocodeQuery.java | Adds hasPid flag and adjusts preliminary result sizing to compensate for filtering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1682
to
+1683
| ISite site = getRawSiteByUuid(UUID.fromString(address.getSiteID())); | ||
| return site != null && site.getPids() != null && !site.getPids().trim().isEmpty(); |
Comment on lines
+396
to
+400
| for(GeocodeMatch match : matches) { | ||
| match.resolve(datastore); | ||
| if(datastore.hasPids(match)) { | ||
| filteredMatches.add(match); | ||
| } |
Comment on lines
+452
to
+454
| if(hasPid) { | ||
| return Math.max(getMaxResults() + 1, 100); | ||
| } |
| matches = handler.getMatches(); | ||
| } | ||
|
|
||
| matches = filterMatchesByPid(matches, query); |
Comment on lines
+254
to
+256
| if(!pids.contains(pid)) { | ||
| pids.add(pid); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement hasPid parameter when enabled Geocoder only return matches that have pid. Also has minor update in SiteloaderPrep.