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

Remove geohash_cell and geo_distance_range queries #21825

Closed
dadoonet opened this issue Nov 28, 2016 · 16 comments
Closed

Remove geohash_cell and geo_distance_range queries #21825

dadoonet opened this issue Nov 28, 2016 · 16 comments
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >docs General docs changes v5.4.4 v6.0.3

Comments

@dadoonet
Copy link
Member

Related to #21670

We are still documenting geo_distance_range in our docs:

And also Java Client users can still see the related builders without any deprecation notice:

In 5.0, 5.1 and 5.x branches we should mark related methods and classes as deprecated, same for our documentation (ref guide and java guide)
In 6.0 we should remove those methods.

In 5.0 series, we should use the deprecation logger in case anyone is using that on 2.x indices.
For 5.x indices, it fails nicely with messages like:

  • [failed to parse [geohash_cell] query. geo_point field no longer supports geohash_cell queries]
  • [geo_distance_range] queries are no longer supported for geo_point field types. Use geo_distance sort or aggregations
@dadoonet dadoonet added :Analytics/Geo Indexing, search aggregations of geo points and shapes :Core/Infra/Transport API Transport client API >docs General docs changes labels Nov 28, 2016
dadoonet added a commit to dadoonet/elasticsearch that referenced this issue Nov 28, 2016
Some of the methods have been removed or deprecated.

Also related to elastic#21825.
@clintongormley
Copy link
Contributor

@nknize i thought that the geo_distance_range query was coming back?

@jpountz
Copy link
Contributor

jpountz commented Nov 29, 2016

From a recent conversation with @nknize, it will not come back.

dadoonet added a commit that referenced this issue Nov 30, 2016
Some of the methods have been removed or deprecated.

Also related to #21825.

Backport of #21831 in 5.x branch.
dadoonet added a commit that referenced this issue Nov 30, 2016
Some of the methods have been removed or deprecated.

Also related to #21825.

Backport of #21831 in 5.1 branch.
dadoonet added a commit that referenced this issue Nov 30, 2016
Some of the methods have been removed or deprecated.

Also related to #21825.

Backport of #21831 in 5.0 branch.
@javanna
Copy link
Member

javanna commented Nov 30, 2016

I was looking into adding deprecation warnings for geo_distance_range and geohash_cell queries in 5.x, but I got confused. Did I get it right that those queries can't be used already against 5.x indices as they throw error? But it seems like they were never deprecated and we kinda removed the support for them already in 5.0 (unless they are executed against 2.x indices, in which case they work)? Not sure what the best way to document this is.

@javanna javanna removed the :Core/Infra/Transport API Transport client API label Nov 30, 2016
@jpountz
Copy link
Contributor

jpountz commented Dec 1, 2016

I agree we should have made the transition more smooth. Your understanding that they work on 2.x indices and fail on 5.x indices sounds correct to me. Geo-hash cell feels very esoteric to me so I am not too concerned, but for geo_distance_range, maybe we should try to bring it back on the 5.x series, even if that means having limitations such as only working on single-valued fields? We should also mark both queries as deprecated in the docs and Java APIs.

@javanna
Copy link
Member

javanna commented Dec 1, 2016

We should also mark both queries as deprecated in the docs and Java APIs.

and print out deprecation warnings when they are used ;)

@nknize
Copy link
Contributor

nknize commented Dec 1, 2016

Its correct that geo_distance_range and geohash_cell work on 2.x and fail on 5.x due to the migration from GeoPointField to LatLonPoint (which doesn't support a distance range query). I can bring back both as deprecated queries with relative ease. I agree its something that should have been caught in either alpha or beta releases. I suppose the lack of feedback indicates little to no use of each? Nevertheless, the removal should have followed the deprecation process to begin with, so I'll take the blame for that oversight.

@jpountz
Copy link
Contributor

jpountz commented Dec 1, 2016

I just pinged @clintongormley to have his opinion about this issue and he said he would not add the queries back until someone makes a case for them.

So let's just fix our docs to reflect the current state of things in 5.x?

@tobias74
Copy link

What is the alternative in 5.2 for geohash_cell? Or was it just removed without a new implementation?

@jpountz
Copy link
Contributor

jpountz commented Mar 15, 2017

You coud run a bounding box for the given geo hash?

@tobias74
Copy link

tobias74 commented Mar 23, 2017

A bounding box needs two points, but I just have that one geo_hash of the aggregation. The geohash in itself is not really a point, but rather a "geo-box". So I think it should be easy to use it in a bounding-box-query, but I cannot provide the same geohash for both points in the query

@bigdrum
Copy link

bigdrum commented Apr 16, 2017

What's the alternative of geo_distance_range? The document "Distance aggregations or sorting should be used instead" is kind of vague.

Is using a bool must_not query to exclude a geo_distance from another a good solution?

@clintongormley
Copy link
Contributor

Is using a bool must_not query to exclude a geo_distance from another a good solution?

yes

@gmoskovicz
Copy link
Contributor

@nknize @clintongormley

Regarding

A bounding box needs two points, but I just have that one geo_hash of the aggregation. The geohash in itself is not really a point, but rather a "geo-box". So I think it should be easy to use it in a bounding-box-query, but I cannot provide the same geohash for both points in the query

I think that there is a need of some decoding here. Maybe we should add that the query was removed and include also that for people previously running a geohash_cell query they need now two points (so some decoding outside ES is needed)?

@clintongormley
Copy link
Contributor

@gmoskovicz see #25154

@gmoskovicz
Copy link
Contributor

Thanks @clintongormley for pointing me to this, and @jpountz for creating it.

@lcawl lcawl added v6.0.1 and removed v6.0.0 labels Nov 13, 2017
@lcawl lcawl added v6.0.2 and removed v6.0.1 labels Dec 6, 2017
@jaymode jaymode added v6.0.3 and removed v6.0.2 labels Dec 13, 2017
@imotov
Copy link
Contributor

imotov commented Mar 23, 2018

It looks like these methods were removed from the documentation and the query builder. Closing.

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 >docs General docs changes v5.4.4 v6.0.3
Projects
None yet
Development

No branches or pull requests