Cut over geo_point field and queries to new LatLonPoint type #20315

nknize · 2016-09-02T22:38:01Z

This PR cuts over geo_point fields to use Lucene's new point-based LatLonPoint type for indexes created in 5.0. Indexes created prior to 5.0 continue to use their respective encoding type. Below is a description of the changes made to support the new encoding type:

New indexes use a new LatLonPointFieldMapper which provides a parse method for the new type
The new LatLonPoint parse method removes support for deprecated lat_lon and geohash parameters
Backcompat testing for deprecated lat_lon and geohash parameters is added to all unit and integration tests
LatLonPointFieldMapper provides DocValues support (enabled by default) which uses Lucene's new LatLonDocValuesField type
New LatLonPoint field data classes are added for aggregation support (wraps LatLonPoint's Numeric Doc Values)
MultiFields use the geohash as the string value instead of the lat,lon string. This makes it easier to perform geo queries on the geohash instead of a useless lat,lon comma delimited string.

Removed Features:

With the removal of geohash indexing, GeoHashCellQuery support is removed for all new indexes (still supported on existing indexes)
LatLonPoint does not support a Distance Range query because it is super inefficient. Instead, geo_distance_range queries should be accomplished using either the geo_distance aggregation, sorting by descending distance on a geo_distance query, or a boolean must not using the excluded distance (distance_range query was syntactic sugar for this).

closes #20314

clintongormley · 2016-09-07T15:16:39Z

@colings86 @rjernst @mikemccand please could you review this - @jpountz is away

mikemccand · 2016-09-07T18:27:48Z

...c/main/java/org/elasticsearch/index/fielddata/plain/AbstractLatLonPointDVIndexFieldData.java

+import java.io.IOException;
+
+/**
+ * Created by nknize on 8/23/16.


Maybe change these boilerplates to a real javadoc?

mikemccand · 2016-09-07T23:17:25Z

Thanks @nknize, I looked at the Lucene usage here and left a few small comments.

Are you planning on exposing LatLonPoint.newPolygonQuery and .newDistanceSort too?

LatLonPoint also offers very fast nearest neighbor search, but we should probably do that separately.

nknize · 2016-09-08T14:57:26Z

Thanks for the review @mikemccand! I'll address the feedback and update the PR.

Are you planning on exposing LatLonPoint.newPolygonQuery and .newDistanceSort too?

newPolygonQuery does need to be added to this PR, and I'll look at adding newDistanceSort but I don't think that one's a blocker?

I'm also planning to add nearest neighbor in another PR,.

mikemccand · 2016-09-08T17:14:55Z

newPolygonQuery does need to be added to this PR, and I'll look at adding newDistanceSort but I don't think that one's a blocker?

++, these both can be done separately.

mikemccand · 2016-09-13T09:20:01Z

The Lucene parts LGTM, thanks @nknize!

I do think .newDistanceSort is important since the optimized implementation in Lucene is extremely fast, but this can be done separately.

s1monw · 2016-09-13T10:11:58Z

core/src/main/java/org/elasticsearch/index/mapper/BaseGeoPointFieldMapper.java

@@ -143,7 +143,16 @@ public abstract Y build(BuilderContext context, String simpleName, MappedFieldTy
                                FieldMapper geoHashMapper, MultiFields multiFields, Explicit<Boolean> ignoreMalformed, CopyTo copyTo);

        public Y build(Mapper.BuilderContext context) {
-            GeoPointFieldType geoPointFieldType = (GeoPointFieldType)fieldType;
+            // version 5.0 cuts over to LatLonPoint and no longer indexes geohash, or lat/lon separately
+            if (context.indexCreatedVersion().before(Version.V_5_0_0_alpha6)) {


this must be updated to onOrBefore

s1monw · 2016-09-13T10:16:30Z

this look great, @nknize I notice that this removes tons of docs but doesn't add new docs how is this going to work? do we have to add docs later?

s1monw · 2016-09-13T10:17:13Z

test this please

nknize · 2016-09-13T14:41:10Z

Thanks @s1monw! I talked to @clintongormley about updating docs in a separate PR to make it easier to review.

This commit cuts over geo_point fields to use Lucene's new point-based LatLonPoint type for indexes created in 5.0. Indexes created prior to 5.0 continue to use their respective encoding type. Below is a description of the changes made to support the new encoding type: * New indexes use a new LatLonPointFieldMapper which provides a parse method for the new type * The new LatLonPoint parse method removes support for lat_lon and geohash parameters * Backcompat testing for deprecated lat_lon and geohash parameters is added to all unit and integration tests * LatLonPointFieldMapper provides DocValues support (enabled by default) which uses Lucene's new LatLonDocValuesField type * New LatLonPoint field data classes are added for aggregation support (wraps LatLonPoint's Numeric Doc Values) * MultiFields use the geohash as the string value instead of the lat,lon string making it easier to perform geo string queries on the geohash instead of a lat,lon comma delimited string. Removed Features: * With the removal of geohash indexing, GeoHashCellQuery support is removed for all new indexes (still supported on existing indexes) * LatLonPoint does not support a Distance Range query because it is super inefficient. Instead, the geo_distance_range query should be accomplished using either the geo_distance aggregation, sorting by descending distance on a geo_distance query, or a boolean must not of the excluded distance (which is what the distance_range query did anyway). TODO: * fix/finish yaml changes for plugin and rest integration tests * update documentation

This commit removes documentation for: * geohash cell query * lat_lon parameter * geohash parameter * geohash_precision parameter * geohash_prefix parameter It also updates failing tests that reference these parameters for backcompat.

mikemccand · 2016-09-29T17:49:18Z

@nknize I think the newPolygonQueryhere is inefficient in the multi-polygon case? It looks like (correct me if this is wrong!!) if e.g. a user queries on the Russia polygon (many top level polygons, each with many holes) that ES will make a very large BooleanQuery as a result, instead of passing that multi-polygon to LatLonPoint (which is quite a bit more efficient)?

Should I open a new issue to fix this?

nknize · 2016-10-06T20:36:42Z

Nice catch @mikemccand I agree the multi-poly should be passed to LatLonPoint. Go ahead and open the issue and I'll make the change. Thx!

mikemccand · 2016-10-06T21:51:24Z

Thanks @nknize ... I opened #20789.

nknize added review WIP :Analytics/Geo Indexing, search aggregations of geo points and shapes :Search Foundations/Mapping Index mappings, including merging and defining field types v5.0.0-beta1 labels Sep 2, 2016

nknize assigned jpountz Sep 2, 2016

mikemccand reviewed Sep 7, 2016
View reviewed changes

nknize force-pushed the latlonpoint_mapper branch 6 times, most recently from ab47328 to ba0b77d Compare September 9, 2016 20:54

nknize removed the WIP label Sep 9, 2016

nknize force-pushed the latlonpoint_mapper branch from 48e21ac to ccc4c1f Compare September 12, 2016 20:59

s1monw reviewed Sep 13, 2016
View reviewed changes

nknize force-pushed the latlonpoint_mapper branch from ccc4c1f to 3537cbb Compare September 13, 2016 14:37

nknize force-pushed the latlonpoint_mapper branch from ed5225d to 5af9eef Compare September 13, 2016 17:17

Update docs for LatLonPoint cut over

1a60e1c

This commit removes documentation for: * geohash cell query * lat_lon parameter * geohash parameter * geohash_precision parameter * geohash_prefix parameter It also updates failing tests that reference these parameters for backcompat.

nknize force-pushed the latlonpoint_mapper branch from 5af9eef to 1a60e1c Compare September 13, 2016 17:18

nknize merged commit 1a60e1c into elastic:master Sep 13, 2016

clintongormley added v5.0.0-beta1 >feature labels Sep 14, 2016

mikemccand mentioned this pull request Oct 6, 2016

Polygon query for geo_point field should pass the multi-polygon to LatLonPoint #20789

Closed

clintongormley mentioned this pull request Oct 8, 2016

Remove lat_lon? #19792

Closed

javanna mentioned this pull request Sep 25, 2017

Java API contains unsupported geohash_cell query builder #25319

Closed

cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Oct 2, 2023

Add annotation for elastic#20315

885a722

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cut over geo_point field and queries to new LatLonPoint type #20315

Cut over geo_point field and queries to new LatLonPoint type #20315

nknize commented Sep 2, 2016 •

edited

Loading

clintongormley commented Sep 7, 2016

mikemccand Sep 7, 2016

mikemccand commented Sep 7, 2016

nknize commented Sep 8, 2016

mikemccand commented Sep 8, 2016

mikemccand commented Sep 13, 2016

s1monw Sep 13, 2016

s1monw commented Sep 13, 2016

s1monw commented Sep 13, 2016

nknize commented Sep 13, 2016

mikemccand commented Sep 29, 2016

nknize commented Oct 6, 2016

mikemccand commented Oct 6, 2016

Cut over geo_point field and queries to new LatLonPoint type #20315

Cut over geo_point field and queries to new LatLonPoint type #20315

Conversation

nknize commented Sep 2, 2016 • edited Loading

clintongormley commented Sep 7, 2016

mikemccand Sep 7, 2016

Choose a reason for hiding this comment

mikemccand commented Sep 7, 2016

nknize commented Sep 8, 2016

mikemccand commented Sep 8, 2016

mikemccand commented Sep 13, 2016

s1monw Sep 13, 2016

Choose a reason for hiding this comment

s1monw commented Sep 13, 2016

s1monw commented Sep 13, 2016

nknize commented Sep 13, 2016

mikemccand commented Sep 29, 2016

nknize commented Oct 6, 2016

mikemccand commented Oct 6, 2016

nknize commented Sep 2, 2016 •

edited

Loading