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

import all potentially useful fields from CSV in to model #402

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

missinglink
Copy link
Member

imports some fields which are present in the CSV file but are not currently available on the model.

after I pushed this PR I realised that #320 covers some of the same territory.

additional fields imported:

  • UNIT -> setAddress( 'unit' );
  • CITY -> setMeta( 'city' );
  • DISTRICT -> setMeta( 'district' );
  • REGION -> setMeta( 'region' );

The metadata properties are useful downstream as 'hints' for the admin lookup service, this is particularly true in Australia where the CITY field in OA is better than the PIP locality.

I updated the tests and added a few more because the coverage wasn't great.

@orangejulius
Copy link
Member

unit is used in some queries already, so I think we do have to give this at least a little testing https://github.com/pelias/query/blob/master/layout/AddressesUsingIdsQuery.js#L44

That said I really like the idea of adding all this data and we should do it. However it might make sense to do it as part of a specific feature rather than in anticipation of some other PR using it.

Joxit added a commit that referenced this pull request Jan 20, 2021
missinglink pushed a commit that referenced this pull request Jan 21, 2021
* feat(import): add support for openaddresses geojsons format

* feat(import): map field used in #402

* chore(import): apply changes from code review
@NickStallman
Copy link

This branch has some simple conflicts compared to master.

Also should the default name also be modified to include the unit?
If you import 10 units for the same street number, they'd all have the same name if this isn't modified.
I'm not sure what the implications of that are either way.

        .setName( 'default', (record.NUMBER + ' ' + record.STREET) )

@missinglink
Copy link
Member Author

I just looked at rebasing this today and it seems it doesn't really do much.
Maybe it did at the time and most of it has been implemented already or maybe it never did.

Of the 4 changes, 3x of them use doc.setMeta(), this populates a part of the model which is only available during the import pipeline and is discarded before the record is sent to elasticsearch.

It seems the original intention was to provide hints to the admin lookup service, but as Julian mentions, that isn't implemented so there's no reason in adding this functionality until something actually uses it.

The final change doc.setAddress('unit', record.UNIT); is actually quite interesting, it seems we don't import the unit information from OA currently.

We could certainly start doing so, it would just require a little review of the data to see if we need to make any modifications/fixes to it and check it doesn't introduce any unexpected regressions.

diff --git a/lib/streams/documentStream.js b/lib/streams/documentStream.js
index d3ac2f0..7eb4df7 100644
--- a/lib/streams/documentStream.js
+++ b/lib/streams/documentStream.js
@@ -31,6 +31,20 @@ function createDocumentStream(id_prefix, stats) {
         if (record.POSTCODE) {
           doc.setAddress('zip', record.POSTCODE);
         }
+        if (record.UNIT) {
+          doc.setAddress('unit', record.UNIT);
+        }
+
+        // additional document metadata which may be useful downstream
+        if (record.CITY) {
+          doc.setMeta('city', record.CITY);
+        }
+        if (record.DISTRICT) {
+          doc.setMeta('district', record.DISTRICT);
+        }
+        if (record.REGION) {
+          doc.setMeta('region', record.REGION);
+        }

         // attempt to set the country code based on the directory prefix
         const match = id_prefix.match(COUNTRY_CODE_PATTERN);

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.

3 participants