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

Deprecate GeoDistance enums and remove geo distance script helpers #19783

Merged
merged 1 commit into from Aug 6, 2016

Conversation

Projects
None yet
3 participants
@nknize
Copy link
Member

commented Aug 3, 2016

GeoDistance is implemented using a crazy enum that causes issues with the scripting modules. This PR moves all distance calculations to arcDistance and planeDistance static methods in GeoUtils. It also removes unnecessary distance helper methods from ScriptDocValues.GeoPoints. Docs and tests are updated to reflect changes.

*/
@Deprecated

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 3, 2016

Member

Why are we deprecating and not removing these? We know they don't work in scripts, so what uses could their be other than in our own code?

This comment has been minimized.

Copy link
@nknize

nknize Aug 4, 2016

Author Member

None. I'll remove the deprecation.

This comment has been minimized.

Copy link
@nknize

nknize Aug 4, 2016

Author Member

Forgot to mention the GeoDistance class is coupled with the GeoDistance query parser. That's why I started w/ deprecation. I plan to completely remove the GeoDistance class but would rather do it in a few small PRs than one large one. Any issue keeping it in this one and I'll remove it in the next PR?

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 4, 2016

Member

Sure, Ok.

@rjernst

View changes

core/src/main/java/org/elasticsearch/common/geo/GeoUtils.java Outdated
@@ -478,6 +478,21 @@ public static double maxRadialDistanceMeters(final double centerLat, final doubl
return SloppyMath.haversinMeters(centerLat, centerLon, centerLat, (MAX_LON + centerLon) % 360);
}

/** Return the distance (in meters) between 2 lat,lon geo points using the haversine method implemented by lucene */
public static double arcDistance(double lat1, double lon1, double lat2, double lon2, DistanceUnit unit) {

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 3, 2016

Member

I don't think we need the flexibility to take distance unit here. That is an unnecessary extra step. If a user wants to convert from meters, they can. But they don't need our internal utility to do it. Having it also disagrees with the javadoc, which states meters are returned.

This comment has been minimized.

Copy link
@nknize

nknize Aug 4, 2016

Author Member

+1. I had originally removed the DistanceUnit argument (which is why the javadoc disagrees). I added it back for java API convenience but have no strong feelings either way. I can remove it.

GeoPoint point = getValue();
return GeoDistance.FACTOR.calculate(point.lat(), point.lon(), lat, lon, DistanceUnit.DEFAULT) + 2;
}

public double arcDistance(double lat, double lon) {

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 3, 2016

Member

can we just remove these methods too, and have users use the static methods in GeoUtils?

This comment has been minimized.

Copy link
@nknize

nknize Aug 4, 2016

Author Member

we can. Is there a particular motivation for removing doc['loc'].arcDistance(tgtLat, tgtLon) in favor of org.elasticsearch.common.geo.GeoUtils.arcDistance(doc['loc'].lat, doc['loc'].lon, tgtLat, tgtLon)? Seems like the former is more intuitive? Missing values would also have to be explicitly handled in scripts if we remove the withDefault methods. Seems like removing them just adds burden for writing geo scripts?

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 4, 2016

Member

My motivation is both making it so there is one obvious way to calculate distance (I was reminded recently of this beatiful mantra from the Zen of Python: There should be one— and preferably only one —obvious way to do it.). I also think not having instance methods will allow us to play more with the underlying field access so we dont need an intermediate object, GeoPoint).

This comment has been minimized.

Copy link
@nknize

nknize Aug 4, 2016

Author Member

I also think not having instance methods will allow us to play more with the underlying field access so we dont need an intermediate object, GeoPoint).

+1 I'm working toward this over the course of a few "cleanup" PRs. But at the moment even doc['loc'] allocates a GeoPoint so cutting over to the static doesn't fix this issue.

One obvious way to calculate distance

Definitely agree. IMHO the .arcDistance is more obvious than the verbose static, and the withDefault methods also make handling missing values more obvious than having to put if (doc['loc'] == null) in the script? Maybe we also tackle this one in another PR?

@rjernst

This comment has been minimized.

Copy link
Member

commented Aug 3, 2016

I left a couple comments. This is a good start, but I think we should cut it down further to have one way to calculate distance (GeoUtils methods) and leave edge cases (wanting to convert from meters to something else) as an exercise for the user (finding a constant and multiplying).

@nknize

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2016

Thx for the feedback @rjernst. I went ahead and removed the DistanceUnit argument. I'll tackle the enum removal in the next PR and look at removing the GeoPoint allocations in a follow up PR.

@nknize nknize removed the review label Aug 5, 2016

@nknize nknize force-pushed the nknize:deprecate/geodistance_enums branch Aug 5, 2016

Deprecate GeoDistance enumerators and remove geo distance script helpers
GeoDistance is implemented using a crazy enum that causes issues with the scripting modules. This commit moves all distance calculations to arcDistance and planeDistance static methods in GeoUtils. It also removes unnecessary distance helper methods from ScriptDocValues.GeoPoints.

@nknize nknize force-pushed the nknize:deprecate/geodistance_enums branch to 2d590af Aug 5, 2016

@nknize nknize merged commit 2d590af into elastic:master Aug 6, 2016

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
@clintongormley

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

@nknize given that this is a breaking change, please can we have a note in the migration docs?

@nknize

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2016

Thx @clintongormley! I updated the migration docs to reflect the breaking change to geo scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.