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

Implement Geo Shape Query #1058

Open
wants to merge 2 commits into
base: release-branch.v6
Choose a base branch
from

Conversation

heltonmarx
Copy link

@heltonmarx heltonmarx commented Mar 16, 2019

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.

Based on the elasticsearch and GeoJSON specification, this PR try to
solve the olivere#939 supporting the follow types :
 * Circle
 * Envelope
 * Linestring
 * MultiLineString
 * MultiPoint
 * MultiPolygon
 * Point
 * Polygon

Any suggestion would be appreciated.

Change-Id: If7e6f59a3833da665e31e7ec179951780d82f9c2
Change-Id: I72ce5d9647ce5c55924cb563db49de4253ae792c
Coordinates []float64 `json:"coordinates"`
}

// NewCircle creates and initializes a new Circle.

Choose a reason for hiding this comment

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

is there a method to verify the arguments?
for example: the order of coordinates, Lon and Lat or Lat and Lon.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, no, there's not, I will implement it.

Copy link
Owner

Choose a reason for hiding this comment

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

There are some functions like this in geo_point.go.

Copy link
Owner

Choose a reason for hiding this comment

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

They are roughly based on GeoPoint.java.

@olivere
Copy link
Owner

olivere commented Mar 29, 2019

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
Copy link
Owner

olivere commented Mar 29, 2019

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

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