search

pelias / model

Pelias data models
6 stars 17 forks source link

rename 'polygon' field to 'shape' to match pelias/schema #134

Closed missinglink closed 4 years ago

missinglink commented 4 years ago

The setPolygon() and getPolygon() methods are replaced by setShape() & getShape().

This ensures the field and method names are consistent with Pelias Schema.

As discussed in https://github.com/pelias/schema/pull/466 the field name should be shape, not polygon.

It's astounding that it's gone 3 years without anyone noticing that it didn't actually work, most likely because it's not very often used.

orangejulius commented 4 years ago

Yeah, to be clear, outside of little bits of testing out functionality for importing geometries (as part of https://github.com/pelias/whosonfirst/issues/19, https://github.com/pelias/api/issues/1121, etc), this method has never been used.

In the past, we've had a lot of problems with importing geometries into Elasticsearch (poor performance, rejection of large geometries like that of New Zealand by Elasticsearch), but at least theoretically we should allow it. Either that or we should delete the shape field from the schema all together and tell people to use the Spatial service.

missinglink commented 4 years ago

Agh ok, so I'm also not against removing the field instead.

orangejulius commented 4 years ago

Can't believe we had this bug for 3+ years :)

I'm skeptical Elasticsearch geo_shape fields will ever be performant enough to use, but lets merge this for now and try it out. If not we'll remove the field and go 100% all in on the Spatial service :)