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

Allow to parse lat/lon as strings and coerce them #5626

Merged

Conversation

spinscale
Copy link
Contributor

In order to be more failsafe parsing GeoPoints can support
lat/lon as strings and coerce them. Added support and test for this.

@clintongormley
Copy link

Also needs to be fixed here:

[test][1], node[-PhRAYloSDezUgMBB--99w], [P], s[STARTED]: Failed to execute [[[]], suggestSource[{"result":{"text":"hote","completion":{"context":{"location":{"lat":"52.22","lon":"4.53"}},"field":"suggest_geo"}}}]]
org.elasticsearch.ElasticsearchException: failed to execute suggest
at org.elasticsearch.action.suggest.TransportSuggestAction.shardOperation(TransportSuggestAction.java:171)
at org.elasticsearch.action.suggest.TransportSuggestAction.shardOperation(TransportSuggestAction.java:61)
at org.elasticsearch.action.support.broadcast.TransportBroadcastOperationAction$AsyncBroadcastAction$2.run(TransportBroadcastOperationAction.java:226)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:722)
Caused by: org.elasticsearch.ElasticsearchParseException: latitude must be a number
at org.elasticsearch.search.suggest.context.GeolocationContextMapping.parseQuery(GeolocationContextMapping.java:331)
at org.elasticsearch.search.suggest.context.GeolocationContextMapping.parseQuery(GeolocationContextMapping.java:65)
at org.elasticsearch.search.suggest.context.ContextMapping$ContextQuery.parseQueries(ContextMapping.java:296)
at org.elasticsearch.search.suggest.completion.CompletionSuggestParser.parse(CompletionSuggestParser.java:112)
at org.elasticsearch.search.suggest.SuggestParseElement.parseInternal(SuggestParseElement.java:90)
at org.elasticsearch.action.suggest.TransportSuggestAction.shardOperation(TransportSuggestAction.java:165)
... 5 more

@spinscale
Copy link
Contributor Author

updated, can you check again?

if(parser.nextToken() == Token.VALUE_NUMBER) {
lat = parser.doubleValue();
parser.nextToken();
if(parser.currentToken() == Token.VALUE_NUMBER || parser.currentToken() == Token.VALUE_STRING) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you want to use a swich here?

@clintongormley
Copy link

Tests now passing ++

@spinscale
Copy link
Contributor Author

I think we could also remove the duplicated code here. The reason it exists is to support these two formats in the query

POST services/_suggest
{
    "suggest" : {
        "text" : "m",
        "completion" : {
            "field" : "suggest_field",
            "size": 10,
            "context": {
                "location": {
                    "value": {
                        "lat": 0,
                        "lon": 0
                    },
                    "precision": "1km"
                }
            }
        }
    }
}
POST services/_suggest
{
    "suggest" : {
        "text" : "m",
        "completion" : {
            "field" : "suggest_field",
            "size": 10,
            "context": {
                "location": {
                        "lat": 0,
                        "lon": 0,
                        "precision": "1km"
                }
            }
        }
    }
}

If we only want to support the first format, we could remove the duplicate geo point parsing... the second one is shorter and more handy through

@spinscale
Copy link
Contributor Author

updated the PR to use a switch statement

@s1monw
Copy link
Contributor

s1monw commented Apr 1, 2014

LGTM

In order to be more failsafe parsing GeoPoints can support
lat/lon as strings and coerce them. Added support and test for this.
@spinscale spinscale merged commit 8b8cd26 into elastic:master Apr 1, 2014
@s1monw
Copy link
Contributor

s1monw commented Apr 11, 2014

seems like we have to backport this to 1.0 branch as well @spinscale should we?

@spinscale
Copy link
Contributor Author

+1, will do

spinscale added a commit that referenced this pull request Apr 14, 2014
In order to be more failsafe parsing GeoPoints can support
lat/lon as strings and coerce them. Added support and test for this.

This is a part of the patch of 8b8cd26 from PR #5626
spinscale added a commit that referenced this pull request Apr 14, 2014
In order to be more failsafe parsing GeoPoints can support
lat/lon as strings and coerce them. Added support and test for this.

This is a part of the patch of 8b8cd26 from PR #5626
njam added a commit to njam/CM that referenced this pull request Apr 30, 2014
@clintongormley clintongormley added the :Analytics/Geo Indexing, search aggregations of geo points and shapes label Jun 7, 2015
@clintongormley clintongormley changed the title Geo: Allow to parse lat/lon as strings and coerce them Allow to parse lat/lon as strings and coerce them Jun 7, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
In order to be more failsafe parsing GeoPoints can support
lat/lon as strings and coerce them. Added support and test for this.

This is a part of the patch of 8b8cd26 from PR elastic#5626
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
In order to be more failsafe parsing GeoPoints can support
lat/lon as strings and coerce them. Added support and test for this.

This is a part of the patch of 8b8cd26 from PR elastic#5626
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.0.3 v1.1.1 v1.2.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants