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

Add optional left/right parameter to GeoJSON #8978

Closed
wants to merge 0 commits into from

Conversation

nknize
Copy link
Contributor

@nknize nknize commented Dec 16, 2014

This feature adds an optional orientation parameter to the GeoJSON document and geo_shape mapping enabling users to explicitly define how they want Elasticsearch to interpret vertex ordering. The default uses the right-hand rule (counterclockwise for outer ring, clockwise for inner ring) complying with OGC Simple Feature Access standards. The parameter can be explicitly specified for an entire index using the geo_shape mapping by adding "orientation":{"left"|"right"|"cw"|"ccw"|"clockwise"|"counterclockwise"} and/or overridden on each insert by adding the same parameter to the GeoJSON document.

closes #8764

@@ -246,7 +252,7 @@ defines the following vertex ordering:

For polygons that do not cross the dateline, vertex order will not matter in
Elasticsearch. For polygons that do cross the dateline, Elasticsearch requires
vertex orderinging comply with the OGC specification. Otherwise, an unintended polygon
vertex ordering comply with the OGC specification. Otherwise, an unintended polygon

Choose a reason for hiding this comment

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

vertex ordering TO comply

@clintongormley
Copy link

I can just comment on the intent (LGTM) and the docs (minor comments left) but you'll need to find somebody else to review the code :)

@clintongormley clintongormley removed their assignment Dec 17, 2014
@@ -265,6 +272,24 @@ OGC standards to eliminate ambiguity resulting in a polygon that crosses the dat
}
--------------------------------------------------

An `orientation` parameter can be defined when setting the geo_shape mapping (see <<geo-shape-mapping-options>>). This will define vertex
order for the index. It can also be overridden on each document. The following is an example for overriding
Copy link
Member

Choose a reason for hiding this comment

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

What is "index" here? Should this be "field" or "shape" or "coordinates"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Is this any clearer: "This will define vertex order for the coordinate list on the mapped geo_shape field"?

Copy link
Member

Choose a reason for hiding this comment

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

Yes much better.

@rjernst
Copy link
Member

rjernst commented Dec 17, 2014

@nknize I left some comments.

@nknize nknize added review and removed review labels Dec 17, 2014
@nknize nknize assigned rjernst and unassigned rjernst Dec 17, 2014
@nknize
Copy link
Contributor Author

nknize commented Dec 17, 2014

Good comments Ryan! Really appreciate it. I made some updates based on your feedback. Ready for another review!

@rjernst
Copy link
Member

rjernst commented Dec 22, 2014

I'm confused why Orientation still needs the various values, instead of just LEFT and RIGHT? And why have the string/boolean at all? Seems like you could just have a normal enum, and replace getValue() calls with orientation == Orientation.RIGHT?

@colings86
Copy link
Contributor

Good point Ryan. We can used the java doc for the enum values to describe
that the value is the same as clockwise in case the user finds left handed
and right handed confusing.

On Monday, 22 December 2014, Ryan Ernst notifications@github.com wrote:

I'm confused why Orientation still needs the various values, instead of
just LEFT and RIGHT? And why have the string/boolean at all? Seems like
you could just have a normal enum, and replace getValue() calls with orientation
== Orientation.RIGHT?


Reply to this email directly or view it on GitHub
#8978 (comment)
.

nknize added a commit that referenced this pull request Dec 23, 2014
PR #8978 included 4 unnecessary enumeration values ('cw', 'clockwise', 'ccw', 'counterclockwise'). Since the ShapeBuilder.parse method handles these as strings and maps them to LEFT and RIGHT enumerators, respectively, their enumeration counterpart is unnecessary. This minor change adds 4 static convenience variables (COUNTER_CLOCKWISE, CLOCKWISE, CCW, CW) for purposes of the API and removes the unnecessary values from the Orientation Enum.

closes #9035
nknize added a commit that referenced this pull request Dec 23, 2014
PR #8978 included 4 unnecessary enumeration values ('cw', 'clockwise', 'ccw', 'counterclockwise'). Since the ShapeBuilder.parse method handles these as strings and maps them to LEFT and RIGHT enumerators, respectively, their enumeration counterpart is unnecessary. This minor change adds 4 static convenience variables (COUNTER_CLOCKWISE, CLOCKWISE, CCW, CW) for purposes of the API and removes the unnecessary values from the Orientation Enum.

closes #9035
nknize added a commit that referenced this pull request Dec 23, 2014
PR #8978 included 4 unnecessary enumeration values ('cw', 'clockwise', 'ccw', 'counterclockwise'). Since the ShapeBuilder.parse method handles these as strings and maps them to LEFT and RIGHT enumerators, respectively, their enumeration counterpart is unnecessary. This minor change adds 4 static convenience variables (COUNTER_CLOCKWISE, CLOCKWISE, CCW, CW) for purposes of the API and removes the unnecessary values from the Orientation Enum.

closes #9035
@nknize
Copy link
Contributor Author

nknize commented Dec 29, 2014

merged from 77a7ef2

@nknize nknize closed this Dec 29, 2014
@nknize nknize deleted the feature/8764 branch December 29, 2014 21:58
@nknize nknize added v2.0.0-beta1 :Analytics/Geo Indexing, search aggregations of geo points and shapes >feature v1.4.3 v1.5.0 and removed review :Analytics/Geo Indexing, search aggregations of geo points and shapes >feature v1.4.3 v1.5.0 labels Jan 7, 2015
@clintongormley clintongormley changed the title [GEO] Add optional left/right parameter to GeoJSON Geo: Add optional left/right parameter to GeoJSON Feb 10, 2015
@clintongormley clintongormley changed the title Geo: Add optional left/right parameter to GeoJSON Add optional left/right parameter to GeoJSON Jun 7, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
PR elastic#8978 included 4 unnecessary enumeration values ('cw', 'clockwise', 'ccw', 'counterclockwise'). Since the ShapeBuilder.parse method handles these as strings and maps them to LEFT and RIGHT enumerators, respectively, their enumeration counterpart is unnecessary. This minor change adds 4 static convenience variables (COUNTER_CLOCKWISE, CLOCKWISE, CCW, CW) for purposes of the API and removes the unnecessary values from the Orientation Enum.

closes elastic#9035
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >enhancement v1.4.3 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GEO] Add optional left/right parameter to GeoJSON
4 participants