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

Geo: Switch Geometry classes from lat, lon, alt to x, y, z #45048

Closed
imotov opened this issue Jul 31, 2019 · 3 comments · Fixed by #45332
Closed

Geo: Switch Geometry classes from lat, lon, alt to x, y, z #45048

imotov opened this issue Jul 31, 2019 · 3 comments · Fixed by #45332
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes :Analytics/SQL SQL querying >breaking-java

Comments

@imotov
Copy link
Contributor

imotov commented Jul 31, 2019

Proposal

  • Change all references to lat, lon and alt with x, y, and z
  • Change the order of the parameters in constructors fromlat, lon, alt to x, y, z
  • Move Geometry hierarchy from org.elasticsearch.geo.geometry to org.elasticsearch.geometry

Rationale

I think I made a mistake of going with lat, lon notation instead of x, y notation when I first introduced the Geometry hierarchy and the window of opportunity to rectify this mistake with a limited impact on end users is quickly closing. I have been recently doing a lot of refactoring work with Geometry classes and was constantly struggling with the order and naming of parameters for the following reasons:

  • the current order and naming defers from most of the other libraries and standard that we are using in elasticsearch including JTS, WKT, GeoJSON and naming convention used in geosql functions.
  • we are in process of extending use of Geometry classes in the context of XYShapes and different projections, which makes generic x and y more appropriate than lat and lon.
  • the hierarchy is based on Geometry (not Geography) in the first place, so member naming based on lat and lon doesn't seem to be consistent with the class naming.

Reducing the impact

If we make the transition before 7.4 the only users who would be affected are JDBC users that are using geosql features. The feature is currently marked as beta and JDBC driver is setup in such a way that it doesn't provide any compatibility between version, so migration will mean recompiling with a new version of the driver. Since we are change the order of the parameters in constructor but not the type we will bring users attention to this issue by also changing the package name for Geometry classes. However, there is no way to avoid changing the code for JDBC geosql users. On the positive side, switching to x, y, z naming convention will make interface it more consistent for goesql users as well. Considering that the feature is really, I don't think that a lot of users will be affected by this change.

In 7.4 the Geometry hierarchy will become the part of QueryBuilder interface, so any changes will have much bigger impact.

@imotov imotov added :Analytics/Geo Indexing, search aggregations of geo points and shapes >breaking-java :Analytics/SQL SQL querying team-discuss labels Jul 31, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

imotov added a commit to imotov/elasticsearch that referenced this issue Aug 8, 2019
Changes the order of parameters in Geometries from lat, lon to lon, lat
and moves all Geometry classes are moved to the
org.elasticsearch.geomtery package.

Closes elastic#45048
@imotov
Copy link
Contributor Author

imotov commented Aug 8, 2019

We discussed this with both SQL and Geo teams and decided to proceed with this change. I opened #45332.

imotov added a commit that referenced this issue Aug 9, 2019
Changes the order of parameters in Geometries from lat, lon to lon, lat
and moves all Geometry classes are moved to the
org.elasticsearch.geomtery package.

Closes #45048
imotov added a commit to imotov/elasticsearch that referenced this issue Aug 15, 2019
Changes the order of parameters in Geometries from lat, lon to lon, lat
and moves all Geometry classes are moved to the
org.elasticsearch.geomtery package.

Backport of elastic#45332

Closes elastic#45048
imotov added a commit that referenced this issue Aug 16, 2019
Changes the order of parameters in Geometries from lat, lon to lon, lat
and moves all Geometry classes are moved to the
org.elasticsearch.geomtery package.

Backport of #45332

Closes #45048
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 :Analytics/SQL SQL querying >breaking-java
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants