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

Fix #302: Read address attributes from nominatim #303

Merged
merged 10 commits into from
Jun 7, 2020
80 changes: 79 additions & 1 deletion es/mappings.json
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,85 @@
]
}
}
}
},
"district": {
"properties": {
"de": {
"type": "text",
"index": false,
"copy_to": [
"collector.de"
]
},
"default": {
"type": "text",
"index": false,
"copy_to": [
"collector.default"
]
},
"en": {
"type": "text",
"index": false,
"copy_to": [
"collector.en"
]
},
"fr": {
"type": "text",
"index": false,
"copy_to": [
"collector.fr"
]
},
"it": {
"type": "text",
"index": false,
"copy_to": [
"collector.it"
]
}
}
},
"locality": {
"properties": {
"de": {
"type": "text",
"index": false,
"copy_to": [
"collector.de"
]
},
"default": {
"type": "text",
"index": false,
"copy_to": [
"collector.default"
]
},
"en": {
"type": "text",
"index": false,
"copy_to": [
"collector.en"
]
},
"fr": {
"type": "text",
"index": false,
"copy_to": [
"collector.fr"
]
},
"it": {
"type": "text",
"index": false,
"copy_to": [
"collector.it"
]
}
}
}
}
}
}
2 changes: 2 additions & 0 deletions src/main/java/de/komoot/photon/Constants.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ public class Constants {
public static final String COUNTRY = "country";
public static final String COUNTRYCODE = "countrycode";
public static final String CITY = "city";
public static final String DISTRICT = "district";
public static final String LOCALITY = "locality";
public static final String STREET = "street";
public static final String STATE = "state";
public static final String TYPE = "type";
Expand Down
72 changes: 70 additions & 2 deletions src/main/java/de/komoot/photon/PhotonDoc.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
import com.vividsolutions.jts.geom.Point;
import lombok.Getter;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;

import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
Expand All @@ -17,6 +19,7 @@
*/
@Getter
@Setter
@Slf4j
public class PhotonDoc {
final private long placeId;
final private String osmType;
Expand All @@ -25,6 +28,7 @@ public class PhotonDoc {
final private String tagValue;
final private Map<String, String> name;
private String postcode;
final private Map<String, String> address;
final private Map<String, String> extratags;
final private Envelope bbox;
final private long parentPlaceId; // 0 if unset
Expand All @@ -34,14 +38,16 @@ public class PhotonDoc {
final private int rankSearch;

private Map<String, String> street;
private Map<String, String> locality;
private Map<String, String> district;
private Map<String, String> city;
private Set<Map<String, String>> context = new HashSet<Map<String, String>>();
private Map<String, String> country;
private Map<String, String> state;
private String houseNumber;
private Point centroid;

public PhotonDoc(long placeId, String osmType, long osmId, String tagKey, String tagValue, Map<String, String> name, String houseNumber, Map<String, String> extratags, Envelope bbox, long parentPlaceId, double importance, CountryCode countryCode, Point centroid, long linkedPlaceId, int rankSearch) {
public PhotonDoc(long placeId, String osmType, long osmId, String tagKey, String tagValue, Map<String, String> name, String houseNumber, Map<String, String> address, Map<String, String> extratags, Envelope bbox, long parentPlaceId, double importance, CountryCode countryCode, Point centroid, long linkedPlaceId, int rankSearch) {
String place = extratags != null ? extratags.get("place") : null;
if (place != null) {
// take more specific extra tag information
Expand All @@ -56,6 +62,7 @@ public PhotonDoc(long placeId, String osmType, long osmId, String tagKey, String
this.tagValue = tagValue;
this.name = name;
this.houseNumber = houseNumber;
this.address = address;
this.extratags = extratags;
this.bbox = bbox;
this.parentPlaceId = parentPlaceId;
Expand All @@ -75,6 +82,7 @@ public PhotonDoc(PhotonDoc other) {
this.name = other.name;
this.houseNumber = other.houseNumber;
this.postcode = other.postcode;
this.address = other.address;
this.extratags = other.extratags;
this.bbox = other.bbox;
this.parentPlaceId = other.parentPlaceId;
Expand All @@ -84,6 +92,8 @@ public PhotonDoc(PhotonDoc other) {
this.linkedPlaceId = other.linkedPlaceId;
this.rankSearch = other.rankSearch;
this.street = other.street;
this.locality = other.locality;
this.district = other.district;
this.city = other.city;
this.context = other.context;
this.country = other.country;
Expand All @@ -102,7 +112,7 @@ public String getUid() {
*/
public static PhotonDoc create(long placeId, String osmType, long osmId, Map<String, String> nameMap) {
return new PhotonDoc(placeId, osmType, osmId, "", "", nameMap,
"", null, null, 0, 0, null, null, 0, 0);
"", null, null, null, 0, 0, null, null, 0, 0);
}

public boolean isUsefulForIndex() {
Expand All @@ -116,4 +126,62 @@ public boolean isUsefulForIndex() {

return true;
}

/**
* Complete doc from nominatim address information.
*/
public void completeFromAddress() {
if (address == null) return;

String addressStreet = address.get("street");
if (addressStreet != null) {
if (this.street == null) {
this.street = new HashMap<>();
}
setOrReplace(addressStreet, this.street, "street");
}

String addressCity = address != null ? address.get("city") : null;
if (addressCity != null) {
if (this.city == null) {
this.city = new HashMap<>();
}
setOrReplace(addressCity, this.city, "city");
}

String addressDistrict = address != null ? address.get("suburb") : null;
if (addressDistrict != null) {
if (this.district == null) {
this.district = new HashMap<>();
}
setOrReplace(addressDistrict, this.district, "suburb");
}

String addressLocality = address != null ? address.get("neighbourhood") : null;
if (addressLocality != null) {
if (this.locality == null) {
this.locality = new HashMap<>();
}
setOrReplace(addressLocality, this.locality, "neighbourhood");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code duplication can be avoided if you move the lookup into setOrReplace as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed some duplication but not as much as I would have liked as the assignment to the instance fields (this.locality...) must happen outside setOrReplace or am I overlooking something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking along the lines of having a function which can be used like this:

this.locality = extractAddress(this.locality, address, "neighbourhood")

Nevermind for now. It is good as is.


String addressPostCode = address != null ? address.get("postcode") : null;
if (addressPostCode != null && !addressPostCode.equals(this.postcode)) {
if (log.isDebugEnabled()) {
log.debug("Replacing postcode "+this.postcode+" with "+ addressPostCode+ " for osmId #" + osmId);
}
this.postcode = addressPostCode;
}
}

private void setOrReplace(String name, Map<String, String> namesMap, String field) {
String existingName = namesMap.get("name");
if (!name.equals(existingName)) {
if (log.isDebugEnabled()) {
log.debug("Replacing "+ field +" name '"+existingName+"' with '"+ name+ "' for osmId #" + osmId);
}
namesMap.put("formerName", existingName);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more thinking of adding it to the context hashset. If you have a look at completePlace() function in NominatimConnector.java, you'll see that this is what happens when multiple city-like address parts show up.

namesMap.put("name", name);
}
}
}
2 changes: 2 additions & 0 deletions src/main/java/de/komoot/photon/Utils.java
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ public static XContentBuilder convert(PhotonDoc doc, String[] languages) throws
builder.field(Constants.COUNTRYCODE, countryCode.getAlpha2());
writeIntlNames(builder, doc.getState(), "state", languages);
writeIntlNames(builder, doc.getStreet(), "street", languages);
writeIntlNames(builder, doc.getLocality(), "locality", languages);
writeIntlNames(builder, doc.getDistrict(), "district", languages);
writeContext(builder, doc.getContext(), languages);
writeExtent(builder, doc.getBbox());

Expand Down
2 changes: 2 additions & 0 deletions src/main/java/de/komoot/photon/elasticsearch/Server.java
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ private JSONObject addLangsToMapping(JSONObject mappingsObject) {
propertiesObject = addToCollector("country", propertiesObject, copyToCollectorObject, lang);
propertiesObject = addToCollector("state", propertiesObject, copyToCollectorObject, lang);
propertiesObject = addToCollector("street", propertiesObject, copyToCollectorObject, lang);
propertiesObject = addToCollector("locality", propertiesObject, copyToCollectorObject, lang);
propertiesObject = addToCollector("locality", propertiesObject, copyToCollectorObject, lang);
propertiesObject = addToCollector("name", propertiesObject, nameToCollectorObject, lang);

// add language specific collector to default for name
Expand Down
19 changes: 17 additions & 2 deletions src/main/java/de/komoot/photon/nominatim/NominatimConnector.java
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ public NominatimResult mapRow(ResultSet rs, int rownum) throws SQLException {
"house_number",
Collections.<String, String>emptyMap(), // no name
(String) null,
Collections.<String, String>emptyMap(), // no address
Collections.<String, String>emptyMap(), // no extratags
(Envelope) null,
rs.getLong("parent_place_id"),
Expand Down Expand Up @@ -186,6 +187,7 @@ public NominatimResult mapRow(ResultSet rs, int rowNum) throws SQLException {
rs.getString("type"),
DBUtils.getMap(rs, "name"),
(String) null,
DBUtils.getMap(rs, "address"),
DBUtils.getMap(rs, "extratags"),
envelope,
rs.getLong("parent_place_id"),
Expand All @@ -205,7 +207,7 @@ public NominatimResult mapRow(ResultSet rs, int rowNum) throws SQLException {
return result;
}
};
private final String selectColsPlaceX = "place_id, osm_type, osm_id, class, type, name, housenumber, postcode, extratags, ST_Envelope(geometry) AS bbox, parent_place_id, linked_place_id, rank_search, importance, country_code, centroid";
private final String selectColsPlaceX = "place_id, osm_type, osm_id, class, type, name, housenumber, postcode, address, extratags, ST_Envelope(geometry) AS bbox, parent_place_id, linked_place_id, rank_search, importance, country_code, centroid";
private final String selectColsOsmline = "place_id, osm_id, parent_place_id, startnumber, endnumber, interpolationtype, postcode, country_code, linegeo";
private final String selectColsAddress = "p.place_id, p.osm_type, p.osm_id, p.name, p.class, p.type, p.rank_address, p.admin_level, p.postcode, p.extratags->'place' as place";
private Importer importer;
Expand Down Expand Up @@ -297,7 +299,7 @@ public AddressRow mapRow(ResultSet rs, int rowNum) throws SQLException {
return terms;
}

private static final PhotonDoc FINAL_DOCUMENT = new PhotonDoc(0, null, 0, null, null, null, null, null, null, 0, 0, null, null, 0, 0);
private static final PhotonDoc FINAL_DOCUMENT = new PhotonDoc(0, null, 0, null, null, null, null, null, null, null, 0, 0, null, null, 0, 0);

private class ImportThread implements Runnable {
private final BlockingQueue<PhotonDoc> documents;
Expand Down Expand Up @@ -487,6 +489,16 @@ private void completePlace(PhotonDoc doc) {
continue;
}

if (address.isLocality() && doc.getLocality() == null) {
doc.setLocality(address.getName());
continue;
}

if (address.isDistrict() && doc.getDistrict() == null) {
doc.setDistrict(address.getName());
continue;
}

if (address.isState() && doc.getState() == null) {
doc.setState(address.getName());
continue;
Expand All @@ -497,5 +509,8 @@ private void completePlace(PhotonDoc doc) {
doc.getContext().add(address.getName());
}
}
// finally, overwrite gathered information with higher prio
// address info from nominatim which should have precedence
doc.completeFromAddress();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ public boolean isStreet() {
return 26 <= rankAddress && rankAddress < 28;
}

public boolean isLocality() {
return 22 <= rankAddress && rankAddress < 26;
}

public boolean isDistrict() {
return 17 <= rankAddress && rankAddress < 22;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the other one, you should now use the rank address and get better results for that:

suburb: [17, 21]
neighbourhood: [22, 25]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that was misleading. I meant inclusive ranges here. So it should be:

return 22 <= rankAddress && rankAddress < 26;

and

return 17 <= rankAddress && rankAddress < 22;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed that one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistically you should be returning the condition directly, that is
...
return "place".equals(osmKey) && "suburb".equals(osmValue);
...

Further I would suggest, as a rule, always adding javadoc to new methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, GitHub doesn't update the view of the lines you have commented on. It actually already does what you suggest, @simonpoole.

And yes, I can add the javadoc.

public boolean isCity() {
return 13 <= rankAddress && rankAddress <= 16;
}
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/de/komoot/photon/utils/ConvertToJson.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,16 @@
@Slf4j
public class ConvertToJson {
private final static String[] KEYS_LANG_UNSPEC = {Constants.OSM_ID, Constants.OSM_VALUE, Constants.OSM_KEY, Constants.POSTCODE, Constants.HOUSENUMBER, Constants.COUNTRYCODE, Constants.OSM_TYPE};
private final static String[] KEYS_LANG_SPEC = {Constants.NAME, Constants.COUNTRY, Constants.CITY, Constants.STREET, Constants.STATE};
private final static String[] KEYS_LANG_SPEC = {Constants.NAME, Constants.COUNTRY, Constants.CITY, Constants.DISTRICT, Constants.LOCALITY, Constants.STREET, Constants.STATE};

private final String lang;

public ConvertToJson(String lang) {
this.lang = lang;
}

public List<JSONObject> convert(SearchResponse searchResponse) {
SearchHit[] hits = searchResponse.getHits().hits();
SearchHit[] hits = searchResponse.getHits().getHits();
final List<JSONObject> list = Lists.newArrayListWithExpectedSize(hits.length);
for (SearchHit hit : hits) {
final Map<String, Object> source = hit.getSource();
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/de/komoot/photon/ESBaseTester.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class ESBaseTester {
private PhotonDoc createDoc(double lon, double lat, int id, int osmId, String key, String value) {
ImmutableMap<String, String> nameMap = ImmutableMap.of("name", "berlin");
Point location = FACTORY.createPoint(new Coordinate(lon, lat));
return new PhotonDoc(id, "way", osmId, key, value, nameMap, null, null, null, 0, 0.5, null, location, 0, 0);
return new PhotonDoc(id, "way", osmId, key, value, nameMap, null, null, null, null, 0, 0.5, null, location, 0, 0);
}

@Before
Expand Down
39 changes: 39 additions & 0 deletions src/test/java/de/komoot/photon/PhotonDocTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package de.komoot.photon;

import java.util.HashMap;

import org.hamcrest.core.IsEqual;
import org.junit.Assert;
import org.junit.Test;

public class PhotonDocTest {

@Test
public void testCompleteAddressOverwritesStreet() {
HashMap<String, String> address = new HashMap<>();
address.put("street", "test street");
PhotonDoc doc = createPhotonDocWithAddress(address);

HashMap<String, String> streetNames = new HashMap<>();
streetNames.put("name", "parent place street");
doc.setStreet(streetNames);

doc.completeFromAddress();
Assert.assertThat(doc.getStreet().get("name"), IsEqual.equalTo("test street"));
}

@Test
public void testCompleteAddressCreatesStreetIfNonExistantBefore() {
HashMap<String, String> address = new HashMap<>();
address.put("street", "test street");
PhotonDoc doc = createPhotonDocWithAddress(address);

doc.completeFromAddress();
Assert.assertThat(doc.getStreet().get("name"), IsEqual.equalTo("test street"));
}

private PhotonDoc createPhotonDocWithAddress(HashMap<String, String> address) {
return new PhotonDoc(1, "W", 2, "highway", "residential", null, "4", address, null, null, 0, 30, null, null, 0, 30);
}

}