elastic icon indicating copy to clipboard operation
elastic copied to clipboard

Implement Geo Shape Query

Open heltonmarx opened this issue 6 years ago • 2 comments

Based on the elasticsearch and GeoJSON specification, this PR tries to solve the #939 supporting the follow types :

  • Circle
  • Envelope
  • Linestring
  • MultiLineString
  • MultiPoint
  • MultiPolygon
  • Point
  • Polygon

Any suggestion would be appreciated.

heltonmarx avatar Mar 16 '19 19:03 heltonmarx

A few initial comments on this PR:

  • Don't use documentation links with current or N.x (e.g. 6.x). These refer to the current version or the working version for 6.x. For the former, we refer to Elasticsearch version 12.0 in ~5 years. For the latter, we refer to the state the technical writers at Elasticsearch use for deployment before releasing the actual version of the docs. Use a full version number like 6.7 in the links as that won't change, and 6.7 is actually very good because that's probably the last version in the v6 time frame.
  • What is rawGeometry and why do we need this? Seems like a code smell to me. I'd prefer to have e.g. Circle be separate from Point, even if they have some things in common.
  • Do we really need TypePoint, TypeCircle etc.?
  • I don't like the fields of e.g. Circle being exported and still have a constructor-like function like NewCircle.
  • Please remove that "github.com/stretchr/testify/require" dependency. I'm only using "testing" from standard library and github.com/google/go-cmp/cmp, and I work actively against pulling in any additional dependencies.
  • The WithXXX functions are a nice idea at first sight. What about a separate design where you would use MarshalJSON on the individual shapes to add the missing type field to the serialized JSON. Wouldn't it be nice to write something like this from the client's perspective?
c := geo.Circle{Center: geo.PointFromLatLon(40.513, -70.119), Radius: 3*geo.Kilometer}

olivere avatar Mar 29 '19 15:03 olivere

Can we use this PR instead of #956, i.e. do you try to implement the same thing?

olivere avatar Mar 29 '19 15:03 olivere