Remove .geohash suffix from GeoDistanceQuery and GeoDistanceRangeQuery

Occasionally the .geohash suffix in Geo{Distance|DistanceRange}Query would conflict with a mapping that defines a sub-field by the same name. This occurs often with nested and multi-fields a mapping defines a geo_point sub-field using the field name "geohash". Since the QueryParser already handles parsing geohash encoded geopoints without requiring the ".geohash" suffix, the suffix parsing can be removed altogether. This commit removes the .geohash suffix parsing, adds explicit test coverage for the nested query use-case, and adds random distance queries to the nested query test suite.
elastic · Mar 9, 2016 · d09ee3f · d09ee3f
1 parent e411cbb
commit d09ee3f
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 15 deletions.
diff --git a/core/src/main/java/org/elasticsearch/index/mapper/geo/BaseGeoPointFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/geo/BaseGeoPointFieldMapper.java
@@ -65,7 +65,6 @@ public static class Names {
         public static final String LON = "lon";
         public static final String LON_SUFFIX = "." + LON;
         public static final String GEOHASH = "geohash";
-        public static final String GEOHASH_SUFFIX = "." + GEOHASH;
         public static final String IGNORE_MALFORMED = "ignore_malformed";
     }
 

diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryParser.java
@@ -120,9 +120,6 @@ public GeoDistanceQueryBuilder fromXContent(QueryParseContext parseContext) thro
                 } else if (currentFieldName.endsWith(GeoPointFieldMapper.Names.LON_SUFFIX)) {
                     point.resetLon(parser.doubleValue());
                     fieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.LON_SUFFIX.length());
-                } else if (currentFieldName.endsWith(GeoPointFieldMapper.Names.GEOHASH_SUFFIX)) {
-                    point.resetFromGeoHash(parser.text());
-                    fieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.GEOHASH_SUFFIX.length());
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, AbstractQueryBuilder.NAME_FIELD)) {
                     queryName = parser.text();
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, AbstractQueryBuilder.BOOST_FIELD)) {

diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoDistanceRangeQueryParser.java b/core/src/main/java/org/elasticsearch/index/query/GeoDistanceRangeQueryParser.java
@@ -196,15 +196,6 @@ public GeoDistanceRangeQueryBuilder fromXContent(QueryParseContext parseContext)
                         point = new GeoPoint();
                     }
                     point.resetLon(parser.doubleValue());
-                } else if (currentFieldName.endsWith(GeoPointFieldMapper.Names.GEOHASH_SUFFIX)) {
-                    String maybeFieldName = currentFieldName.substring(0, currentFieldName.length() - GeoPointFieldMapper.Names.GEOHASH_SUFFIX.length());
-                    if (fieldName == null || fieldName.equals(maybeFieldName)) {
-                        fieldName = maybeFieldName;
-                    } else {
-                        throw new ParsingException(parser.getTokenLocation(), "[" + GeoDistanceRangeQueryBuilder.NAME +
-                                "] field name already set to [" + fieldName + "] but found [" + currentFieldName + "]");
-                    }
-                    point = GeoPoint.fromGeohash(parser.text());
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, NAME_FIELD)) {
                     queryName = parser.text();
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, BOOST_FIELD)) {

diff --git a/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java b/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java
@@ -140,6 +140,7 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
     protected static final String DATE_FIELD_NAME = "mapped_date";
     protected static final String OBJECT_FIELD_NAME = "mapped_object";
     protected static final String GEO_POINT_FIELD_NAME = "mapped_geo_point";
+    protected static final String GEO_POINT_FIELD_MAPPING = "type=geo_point,lat_lon=true,geohash=true,geohash_prefix=true";
     protected static final String GEO_SHAPE_FIELD_NAME = "mapped_geo_shape";
     protected static final String[] MAPPED_FIELD_NAMES = new String[] { STRING_FIELD_NAME, INT_FIELD_NAME, DOUBLE_FIELD_NAME,
             BOOLEAN_FIELD_NAME, DATE_FIELD_NAME, OBJECT_FIELD_NAME, GEO_POINT_FIELD_NAME, GEO_SHAPE_FIELD_NAME };
@@ -300,7 +301,7 @@ public void onRemoval(ShardId shardId, Accountable accountable) {
                     BOOLEAN_FIELD_NAME, "type=boolean",
                     DATE_FIELD_NAME, "type=date",
                     OBJECT_FIELD_NAME, "type=object",
-                    GEO_POINT_FIELD_NAME, "type=geo_point,lat_lon=true,geohash=true,geohash_prefix=true",
+                    GEO_POINT_FIELD_NAME, GEO_POINT_FIELD_MAPPING,
                     GEO_SHAPE_FIELD_NAME, "type=geo_shape"
             ).string()), MapperService.MergeReason.MAPPING_UPDATE, false);
             // also add mappings for two inner field in the object field

diff --git a/core/src/test/java/org/elasticsearch/index/query/GeoDistanceRangeQueryTests.java b/core/src/test/java/org/elasticsearch/index/query/GeoDistanceRangeQueryTests.java
@@ -24,10 +24,12 @@
 import org.apache.lucene.spatial.util.GeoDistanceUtils;
 import org.apache.lucene.util.NumericUtils;
 import org.elasticsearch.Version;
+import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.geo.GeoDistance;
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoUtils;
 import org.elasticsearch.common.unit.DistanceUnit;
+import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.search.geo.GeoDistanceRangeQuery;
 import org.elasticsearch.test.geo.RandomGeoGenerator;
 
@@ -296,6 +298,36 @@ public void testInvalidDistanceUnit() {
         }
     }
 
+    public void testNestedRangeQuery() throws IOException {
+        // create a nested geo_point type with a subfield named "geohash" (explicit testing for ISSUE #15179)
+        MapperService mapperService = queryShardContext().getMapperService();
+        String nestedMapping =
+            "{\"nested_doc\" : {\"properties\" : {" +
+            "\"locations\": {\"properties\": {" +
+            "\"geohash\": {\"type\": \"geo_point\"}}," +
+            "\"type\": \"nested\"}" +
+            "}}}";
+        mapperService.merge("nested_doc", new CompressedXContent(nestedMapping), MapperService.MergeReason.MAPPING_UPDATE, false);
+
+        // create a range query on the nested locations.geohash sub-field
+        String queryJson =
+            "{\n" +
+            "  \"nested\": {\n" +
+            "    \"path\": \"locations\",\n" +
+            "    \"query\": {\n" +
+            "      \"geo_distance_range\": {\n" +
+            "        \"from\": \"0.0km\",\n" +
+            "        \"to\" : \"200.0km\",\n" +
+            "        \"locations.geohash\": \"s7ws01wyd7ws\"\n" +
+            "      }\n" +
+            "    }\n" +
+            "  }\n" +
+            "}\n";
+        NestedQueryBuilder builder = (NestedQueryBuilder) parseQuery(queryJson);
+        QueryShardContext context = createShardContext();
+        builder.toQuery(context);
+    }
+
     public void testFromJson() throws IOException {
         String json =
                 "{\n" +

diff --git a/core/src/test/java/org/elasticsearch/index/query/GeohashCellQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/GeohashCellQueryBuilderTests.java
@@ -63,7 +63,7 @@ protected void doAssertLuceneQuery(Builder queryBuilder, Query query, QueryShard
             assertThat(query, instanceOf(TermQuery.class));
             TermQuery termQuery = (TermQuery) query;
             Term term = termQuery.getTerm();
-            assertThat(term.field(), equalTo(queryBuilder.fieldName() + GeoPointFieldMapper.Names.GEOHASH_SUFFIX));
+            assertThat(term.field(), equalTo(queryBuilder.fieldName() + "." + GeoPointFieldMapper.Names.GEOHASH));
             String geohash = queryBuilder.geohash();
             if (queryBuilder.precision() != null) {
                 int len = Math.min(queryBuilder.precision(), geohash.length());

diff --git a/core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java
@@ -52,6 +52,7 @@ public void setUp() throws Exception {
                 BOOLEAN_FIELD_NAME, "type=boolean",
                 DATE_FIELD_NAME, "type=date",
                 OBJECT_FIELD_NAME, "type=object",
+                GEO_POINT_FIELD_NAME, GEO_POINT_FIELD_MAPPING,
                 "nested1", "type=nested"
         ).string()), MapperService.MergeReason.MAPPING_UPDATE, false);
     }