From e547e113e1fe611f1df0ba7f596091853b3bf481 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Wed, 2 Apr 2014 23:30:17 +0200 Subject: [PATCH] Geo context suggester: Require precision in mapping The default precision was way too exact and could lead people to think that geo context suggestions are not working. This patch now requires you to set the precision in the mapping, as elasticsearch itself can never tell exactly, what the required precision for the users suggestions are. Closes #5621 --- .../suggesters/context-suggest.asciidoc | 3 +- rest-api-spec/test/suggest/20_context.yaml | 5 ++ .../CompletionSuggestionBuilder.java | 2 - .../context/GeolocationContextMapping.java | 8 +++ .../suggest/ContextSuggestSearchTests.java | 58 ++++++++++++++++++- .../GeoLocationContextMappingTest.java | 7 ++- 6 files changed, 76 insertions(+), 7 deletions(-) diff --git a/docs/reference/search/suggesters/context-suggest.asciidoc b/docs/reference/search/suggesters/context-suggest.asciidoc index 60427cbb7c7..1830118af47 100644 --- a/docs/reference/search/suggesters/context-suggest.asciidoc +++ b/docs/reference/search/suggesters/context-suggest.asciidoc @@ -174,13 +174,12 @@ geohash of a certain precision, which provides the context. [float] ==== Geo location Mapping -The mapping for a geo context accepts four settings: +The mapping for a geo context accepts four settings, only of which `precision` is required: [horizontal] `precision`:: This defines the precision of the geohash and can be specified as `5m`, `10km`, or as a raw geohash precision: `1`..`12`. It's also possible to setup multiple precisions by defining a list of precisions: `["5m", "10km"]` - (default is a geohash level of 12) `neighbors`:: Geohashes are rectangles, so a geolocation, which in reality is only 1 metre away from the specified point, may fall into the neighbouring rectangle. Set `neighbours` to `true` to include the neighbouring geohashes in the context. diff --git a/rest-api-spec/test/suggest/20_context.yaml b/rest-api-spec/test/suggest/20_context.yaml index 8577bd50cd2..cabd8a39552 100644 --- a/rest-api-spec/test/suggest/20_context.yaml +++ b/rest-api-spec/test/suggest/20_context.yaml @@ -33,6 +33,7 @@ setup: "context": "location": "type" : "geo" + "precision" : "5km" --- "Simple context suggestion should work": @@ -201,8 +202,12 @@ setup: - do: indices.refresh: {} + - do: + indices.get_mapping: {} + - do: suggest: + index: test body: result: text: "hote" diff --git a/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java b/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java index d757e82ad0a..4631aaabcb5 100644 --- a/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java +++ b/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java @@ -18,8 +18,6 @@ */ package org.elasticsearch.search.suggest.completion; -import org.apache.lucene.search.suggest.analyzing.XFuzzySuggester; -import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.search.suggest.SuggestBuilder; diff --git a/src/main/java/org/elasticsearch/search/suggest/context/GeolocationContextMapping.java b/src/main/java/org/elasticsearch/search/suggest/context/GeolocationContextMapping.java index 7ddbef0f2b2..1016d7aa6a2 100644 --- a/src/main/java/org/elasticsearch/search/suggest/context/GeolocationContextMapping.java +++ b/src/main/java/org/elasticsearch/search/suggest/context/GeolocationContextMapping.java @@ -114,6 +114,10 @@ public class GeolocationContextMapping extends ContextMapping { * config */ protected static GeolocationContextMapping load(String name, Map config) { + if (!config.containsKey(FIELD_PRECISION)) { + throw new ElasticsearchParseException("field [precision] is missing"); + } + final GeolocationContextMapping.Builder builder = new GeolocationContextMapping.Builder(name); if (config != null) { @@ -381,6 +385,10 @@ public class GeolocationContextMapping extends ContextMapping { } } + if (precision == null || precision.length == 0) { + precision = this.precision; + } + return new GeoQuery(name, point.geohash(), precision); } else { return new GeoQuery(name, GeoUtils.parseGeoPoint(parser).getGeohash(), precision); diff --git a/src/test/java/org/elasticsearch/search/suggest/ContextSuggestSearchTests.java b/src/test/java/org/elasticsearch/search/suggest/ContextSuggestSearchTests.java index 021a9d8cb8d..66bc4568ede 100644 --- a/src/test/java/org/elasticsearch/search/suggest/ContextSuggestSearchTests.java +++ b/src/test/java/org/elasticsearch/search/suggest/ContextSuggestSearchTests.java @@ -27,7 +27,9 @@ import org.elasticsearch.action.suggest.SuggestResponse; import org.elasticsearch.common.geo.GeoHashUtils; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.unit.Fuzziness; -import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.*; +import org.elasticsearch.index.mapper.MapperParsingException; +import org.elasticsearch.search.aggregations.support.format.ValueFormatter; import org.elasticsearch.search.suggest.Suggest.Suggestion; import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry; import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry.Option; @@ -479,6 +481,7 @@ public class ContextSuggestSearchTests extends ElasticsearchIntegrationTest { .field("type", "completion") .startObject("context").startObject("location") .field("type", "geo") + .field("precision", "500m") .startObject("default").field("lat", berlinAlexanderplatz.lat()).field("lon", berlinAlexanderplatz.lon()).endObject() .endObject().endObject() .endObject().endObject().endObject() @@ -529,6 +532,7 @@ public class ContextSuggestSearchTests extends ElasticsearchIntegrationTest { .field("type", "completion") .startObject("context").startObject("location") .field("type", "geo") + .field("precision", "1m") .endObject().endObject() .endObject().endObject().endObject() .endObject(); @@ -670,6 +674,7 @@ public class ContextSuggestSearchTests extends ElasticsearchIntegrationTest { .field("type", "completion") .startObject("context").startObject("location") .field("type", "geo") + .field("precision", "5m") .field("path", "loc") .endObject().endObject() .endObject().endObject().endObject() @@ -687,6 +692,57 @@ public class ContextSuggestSearchTests extends ElasticsearchIntegrationTest { assertSuggestion(suggestResponse.getSuggest(), 0, "suggestion", "Berlin Alexanderplatz"); } + @Test(expected = MapperParsingException.class) + public void testThatPrecisionIsRequired() throws Exception { + XContentBuilder xContentBuilder = jsonBuilder().startObject() + .startObject("item").startObject("properties").startObject("suggest") + .field("type", "completion") + .startObject("context").startObject("location") + .field("type", "geo") + .field("path", "loc") + .endObject().endObject() + .endObject().endObject().endObject() + .endObject(); + + assertAcked(prepareCreate(INDEX).addMapping("item", xContentBuilder)); + } + + @Test + public void testThatLatLonParsingFromSourceWorks() throws Exception { + XContentBuilder xContentBuilder = jsonBuilder().startObject() + .startObject("mappings").startObject("test").startObject("properties").startObject("suggest_geo") + .field("type", "completion") + .startObject("context").startObject("location") + .field("type", "geo") + .field("precision", "1km") + .endObject().endObject() + .endObject().endObject().endObject() + .endObject().endObject(); + + assertAcked(prepareCreate("test").setSource(xContentBuilder.bytes())); + + double latitude = 52.22; + double longitude = 4.53; + String geohash = GeoHashUtils.encode(latitude, longitude); + + XContentBuilder doc1 = jsonBuilder().startObject().startObject("suggest_geo").field("input", "Hotel Marriot in Amsterdam").startObject("context").startObject("location").field("lat", latitude).field("lon", longitude).endObject().endObject().endObject().endObject(); + index("test", "test", "1", doc1); + XContentBuilder doc2 = jsonBuilder().startObject().startObject("suggest_geo").field("input", "Hotel Marriot in Berlin").startObject("context").startObject("location").field("lat", 53.31).field("lon", 13.24).endObject().endObject().endObject().endObject(); + index("test", "test", "2", doc2); + refresh(); + + XContentBuilder source = jsonBuilder().startObject().startObject("suggestion").field("text", "h").startObject("completion").field("field", "suggest_geo").startObject("context").field("location", geohash).endObject().endObject().endObject().endObject(); + SuggestRequest suggestRequest = new SuggestRequest(INDEX).suggest(source.bytes()); + SuggestResponse suggestResponse = client().suggest(suggestRequest).get(); + assertSuggestion(suggestResponse.getSuggest(), 0, "suggestion", "Hotel Marriot in Amsterdam"); + + // this is exact the same request, but using lat/lon instead of geohash + source = jsonBuilder().startObject().startObject("suggestion").field("text", "h").startObject("completion").field("field", "suggest_geo").startObject("context").startObject("location").field("lat", latitude).field("lon", longitude).endObject().endObject().endObject().endObject().endObject(); + suggestRequest = new SuggestRequest(INDEX).suggest(source.bytes()); + suggestResponse = client().suggest(suggestRequest).get(); + assertSuggestion(suggestResponse.getSuggest(), 0, "suggestion", "Hotel Marriot in Amsterdam"); + } + public void assertGeoSuggestionsInRange(String location, String suggest, double precision) throws IOException { String suggestionName = randomAsciiOfLength(10); CompletionSuggestionBuilder context = new CompletionSuggestionBuilder(suggestionName).field(FIELD).text(suggest).size(10) diff --git a/src/test/java/org/elasticsearch/search/suggest/context/GeoLocationContextMappingTest.java b/src/test/java/org/elasticsearch/search/suggest/context/GeoLocationContextMappingTest.java index b15865ad639..b95722ffac9 100644 --- a/src/test/java/org/elasticsearch/search/suggest/context/GeoLocationContextMappingTest.java +++ b/src/test/java/org/elasticsearch/search/suggest/context/GeoLocationContextMappingTest.java @@ -18,13 +18,14 @@ */ package org.elasticsearch.search.suggest.context; -import com.carrotsearch.ant.tasks.junit4.dependencies.com.google.common.collect.Maps; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.ElasticsearchTestCase; import org.junit.Test; +import java.util.HashMap; + import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; /** @@ -38,7 +39,9 @@ public class GeoLocationContextMappingTest extends ElasticsearchTestCase { XContentParser parser = XContentHelper.createParser(builder.bytes()); parser.nextToken(); - GeolocationContextMapping mapping = GeolocationContextMapping.load("foo", Maps.newHashMap()); + HashMap config = new HashMap<>(); + config.put("precision", 12); + GeolocationContextMapping mapping = GeolocationContextMapping.load("foo", config); mapping.parseQuery("foo", parser); }