Support filtering against dependency_a when using boundary.country parameter
#1622
Conversation
This file contains 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
orangejulius
added a commit
to pelias/acceptance-tests
that referenced
this pull request
Apr 28, 2022
This adds tests cases for the `boundary.country` parameter on the `search`, and `structured` API endpoints. Previously, we only tested `autocomplete`. In addition, it adds tests for records that only have `parent.dependency_a` values, not `parent.country_a`, which require pelias/api#1622 to pass.
missinglink
approved these changes
Apr 28, 2022
orangejulius
force-pushed
the
boundary_country_dependency
branch
from
48b8827
to
fa72383
Compare
orangejulius
added a commit
to pelias/wof-admin-lookup
that referenced
this pull request
May 10, 2022
…ere possible After pelias/api#1622, which extends the `boundary.country` API parameter to include the `depencency` placetype, we noticed that venue and address records often have a 2-character country code when they are part of a dependency. The `boundary.country` parameter currently checks only for 3-character codes, and so the best solution is to ensure the relevant property on all records in Elasticsearch is a 3-character country code whenever possible. This change expands on the logic for selecting an abbreviation for an admin record to be used for point in polygon lookup. Logic specific to countries is expanded to include dependencies, and some additional possible fields that might contain 3-character codes are checked. Because the abbreviaton logic is now a bit more substantial, it's extracted into its own function. It's also been made a bit less redundant and hopefully more clear.
orangejulius
added a commit
to pelias/wof-admin-lookup
that referenced
this pull request
May 11, 2022
…ere possible After pelias/api#1622, which extends the `boundary.country` API parameter to include the `depencency` placetype, we noticed that venue and address records often have a 2-character country code when they are part of a dependency. The `boundary.country` parameter currently checks only for 3-character codes, and so the best solution is to ensure the relevant property on all records in Elasticsearch is a 3-character country code whenever possible. This change expands on the logic for selecting an abbreviation for an admin record to be used for point in polygon lookup. Logic specific to countries is expanded to include dependencies, and some additional possible fields that might contain 3-character codes are checked. Because the abbreviaton logic is now a bit more substantial, it's extracted into its own function. It's also been made a bit less redundant and hopefully more clear.
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.
The
boundary.countryparameter is supported on all major Pelias API endpoints and adds a hard filter based on a given 2 or 3 character country code.The behavior is pretty straightforward, but the underlying data model for country-like parents is not quite so simple.
While most records have a
parent.country_aproperty with the country code of a parent, a few haveparent.dependency_a. This includes places like US territories outside the 50 states, the French Overseas Territories, and various others that have their own country codes but may not be a completely sovereign country of their own.In practice however, this distinction isn't super useful to most people. See the
country_codeparameter we added in #1541 for another case where it's helpful to provide an interface that glosses over some of the details.Functionality
This PR makes the
boundary.countryparam look for a matching country code in either theparent.dependency_aproperty orparent.country_a, using an Elasticsearchmulti_matchquery. Previously onlyparent.country_awas considered.There's really nothing tricky to it, the
multi_matchquery allows either field to contain the desired country code.As a side note, I realized that our autocomplete queries were using the
parent.country_a.ngramfield due to our work in #1264. While this should make little practical difference it's technically not needed, as theboundary.countryparam doesn't need to consider partial inputs. As a side effect this PR fixes that ever so slight departure from the ideal.A note on implementation
As it stands, this PR includes a departure from the previous implementation, which used a
boundary.countryspecific view in the pelias-query library.Instead, the generic
multi_matchview is used, and all the Pelias API specific logic is contained here. It also means there's no need to release a companion PR topelias-query. In the past we've found the dance of developing PRs to API andpelias-queryin parallel to be a bit of extra work, and this pattern would eliminate that need. I'd love to see us moving towards having all the Pelias-specific query logic here in the API, whilepelias-querycontains only or mostly Elasticsearch-specific stuff.That said it does mean that the way this filter parameter works is now different from most of the others, so it's worth discussing if that's ok with us.
Still to come
The
/v1/reverseendpoint technically supportsboundary.countryas well (though the use case is fairly limited). This will come in a subsequent PR as the "coarse" part of the reverse endpoint doesn't supportboundary.countryat all. It might make sense to have further discussion there, including the possibility of removing theboundary.countryparam from the reverse endpoint.