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

Expose points_only option through geo_shape field mapper #12893

Merged
merged 1 commit into from Sep 8, 2015

Conversation

@nknize
Copy link
Member

commented Aug 14, 2015

This PR adds a points_only option (defaults to false) to the GeoShapeFieldMapper that exposes the pointsOnly optimization in lucene's RecursivePrefixTreeStrategy. This optimization short-circuits DFS traversal when it is known that the field contains points only. To enforce this option the GeoShapeFieldMapper has been updated to throw a MapperParsingException for any Shape that is not of type Point when the option is set to true. Randomized testing is added and docs are updated. ShapeCollection has also been updated to eventually support MultiPoint. Limitations of S4j are currently the only blocker.

closes #12856

@dakrone

View changes

core/src/test/java/org/elasticsearch/search/geo/GeoShapeIntegrationIT.java Outdated
ensureGreen();

// test exception handling by attempting to insert a random polygon
ShapeBuilder shape = RandomShapeGenerator.createShape(random());

This comment has been minimized.

Copy link
@dakrone

dakrone Sep 1, 2015

Member

Just so I understand, the random shape could or could not be a ploygon, so if it is a polygon we want to make sure an exception is thrown (and then finish the test), otherwise we want to use it in a query. Correct?

This comment has been minimized.

Copy link
@nknize

nknize Sep 4, 2015

Author Member

Good point. The comment should be more clear (and probably moved inside the catch block). RandomShapeGenerator.createShape generates a random GeoJson compliant object, so that could be one of [Point, MultiPoint, Linestring, MultiLinestring, Polygon, Multipolygon, GeometryCollection]. If the geo_shape mapping is defined with points_only : true then anything other than a Point should throw an exception. That's what we're testing here in the catch block.

@@ -211,7 +219,8 @@ private static ShapeBuilder createShape(Random r, Point nearPoint, Rectangle wit
try {
pgb.build();
} catch (InvalidShapeException e) {
return null;
// jts bug rarely results in an invalid shape, if it does happen we try again instead of returning null
return createShape(r, nearPoint, within, st, validate);

This comment has been minimized.

Copy link
@dakrone

dakrone Sep 1, 2015

Member

How common is the bug? Are we going to exceed max stack depth here recursing?

This comment has been minimized.

Copy link
@nknize

nknize Sep 4, 2015

Author Member

Not at all common. Typically (~90% of the time) the first recursion creates something other than a polygon (e.g., linestring, multilinestring) thus completes without issue.

@dakrone

This comment has been minimized.

Copy link
Member

commented Sep 1, 2015

Left two very minor comments, other than that looks good to me!

@nknize nknize force-pushed the nknize:enhancement/12856 branch Sep 8, 2015

add points_only option to GeoShapeFieldMapper for optimizing indexing…
… performance on geo_shape indexes designed to store only points. Includes updated documentation and exception handling for ensuring index integrity on points only data.

@nknize nknize force-pushed the nknize:enhancement/12856 branch to e4e71d8 Sep 8, 2015

@nknize nknize merged commit e4e71d8 into elastic:master Sep 8, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@clintongormley clintongormley added v2.0.0-beta2 and removed v2.0.0 labels Sep 14, 2015

@clintongormley clintongormley added v2.0.0-rc1 and removed v2.0.0 labels Oct 7, 2015

@clintongormley clintongormley removed the v2.1.0 label Nov 22, 2015

@nknize nknize deleted the nknize:enhancement/12856 branch May 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.