Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Country codes #425

Merged
merged 2 commits into from
Jun 30, 2019
Merged

Country codes #425

merged 2 commits into from
Jun 30, 2019

Conversation

Gerungofulus
Copy link
Contributor

This PR builds on #404. It is different in the sense that it doesn't not add country codes to the default collector. It partially implements #183, as mentioned by the original contributor @Azbesciak. I originally set out to add a country code query param as suggested by @lonvia in #418, but I am unsure about the best way to implement it. Photon already implements filters by tags by using the query parameters osm_tag. I think filtering addresses should be possible by filtering for an osm tag. Unfortunately I was not able to filter properly by country codes using this method. The other solution would be to simply add a new query parameter country_code. What do you think?

Azbesciak and others added 2 commits June 15, 2019 10:44
To keep convention field is named 'countrycode' like a 'housenumber'. It is also uppercase (ISO-3166 alpha 2).
@Gerungofulus
Copy link
Contributor Author

Okay I just realised that osm_tag does not behave in the sense that it could filter any key value pair as it would be possible working directly on osm postgre data. We should probably rename that filter to avoid confusion in the future. What do you think?

@karussell
Copy link
Collaborator

Looks good to me!

And yes, the filtering should go into a different PR. What exactly did you find confusing about osm_tag?

@Gerungofulus
Copy link
Contributor Author

Gerungofulus commented Jun 16, 2019

Looks good to me!

And yes, the filtering should go into a different PR. What exactly did you find confusing about osm_tag?

Well as far as I remember, a given node or way in OSM is an object with an ID in the corresponding table in (nodes/ways). Those objects can have an indefinite amount of key-value pairs called tags. The naming scheme of photon implies that only a certain selection of these kv-pairs is indeed a tag (and an object may only correspond to one of these tags?). So that's why I find the naming osm_tag relatively confusing, as kv pairs with the key being addr:street would not be considered tags while they indeed are considered tags at other sources such as this.

@karussell
Copy link
Collaborator

The naming scheme of photon implies that only a certain selection of these kv-pairs is indeed a tag

I think this is only done because indexing them can be expensive (in terms of index size). Indexing all tags is not easy for world wide setup, so we index only an "important" subset

@Gerungofulus
Copy link
Contributor Author

The naming scheme of photon implies that only a certain selection of these kv-pairs is indeed a tag

I think this is only done because indexing them can be expensive (in terms of index size). Indexing all tags is not easy for world wide setup, so we index only an "important" subset

Yeah that's what I was thinking. It's just that I suggest renaming the query parameter or at least listing which tags are compatible, because this list which is linked in the Readme, does contain country codes, but adding this key to the query params via osm_tag does not seem to do much.

@karussell
Copy link
Collaborator

Yes, we should probably throw an exception for tags that we currently do not support and list all supported (similarly what we do with the query params)

@lonvia lonvia merged commit dca95df into komoot:master Jun 30, 2019
@lonvia
Copy link
Collaborator

lonvia commented Jun 30, 2019

Country code filtering really should be a separate parameter. It is independent of tag filtering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants