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: extract dateline handling logic from ShapeBuilders #44187

Merged
merged 6 commits into from
Jul 16, 2019

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Jul 10, 2019

Extracts dateline decomposition logic from ShapeBuilder into a separate
utility class that is used on the indexing side. The search side
will be handled as part of another PR at this time we will remove
the decomposition logic from ShapeBuilders as well. This PR also doesn't
change any existing logic including bugs.

Relates to #40908

Extracts dateline decomposition logic from ShapeBuilder into a separate
utility class that is used on the indexing side. The search side
will be handled as part of another PR at this time we will remove
the decomposition logic from ShapeBuilders as well. This PR also doesn't
change any existing logic including bugs.
@imotov imotov added :Analytics/Geo Indexing, search aggregations of geo points and shapes >refactoring v8.0.0 v7.4.0 labels Jul 10, 2019
@imotov imotov requested review from talevy and nknize July 10, 2019 19:01
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@nknize
Copy link
Contributor

nknize commented Jul 11, 2019

So it looks like this always assumes spherical geometries and splits across the 180 degree longitude boundary. To support XY cartesian geometries I think we should either:

  1. tease out the base geometry building logic into a base class, and move the GIS decomposition logic into a derived class (e.g., SphericalGeometryIndexer) so we can extend to a CartesianGeometryIndexer in the geo xpack module.
  2. update this class constructor to accept an isGeo parameter and only call decompose if set to true. This way we can build cartesian Geometry in core and implement the XY geometry building logic in the toLucenePolygon method inside GeometryFieldMapper of the xpack geo module

I like 2 because I think it makes the building logic more friendly for the REST API?

@imotov
Copy link
Contributor Author

imotov commented Jul 11, 2019

@elasticmachine run elasticsearch-ci/default-distro

@nknize I was thinking about going the first route actually. I would like to make the Indexing logic easily swap-able, in order to fix the issues that I have stumbled upon this refactoring (such as #43916, #43837, #43826, #43775) in a backward-compatible manner.

Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

LGTM I can't wait for the follow-up PR that removes all the other stuff!

@nknize
Copy link
Contributor

nknize commented Jul 15, 2019

@imotov Just a quick follow up from our conversation: are you still planning on moving GeometryIndexer out of o.e.common.geo since it's only used in GeoShapeFieldMapper? I assumed we would do it in this PR. I think it's fine to move it as package private to o.e.index.mapper since it's never used outside of the Mapping?

@imotov
Copy link
Contributor Author

imotov commented Jul 15, 2019

think it's fine to move it as package private to o.e.index.mapper since it's never used outside of the Mapping?

I don't think I can do that. It will probably be needed in QueryBuilders.

Copy link
Contributor

@nknize nknize left a comment

Choose a reason for hiding this comment

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

It will probably be needed in QueryBuilders.
Okay, let's do it in a separate PR then when we refactor the queries.


ShapeType shapeType = ShapeType.forName(type);
ShapeType shapeType;
if ("bbox".equals(type.toLowerCase(Locale.ROOT))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just use equalsIgnoreCase instead of transforming the type string toLowerCase?

@imotov imotov merged commit 33ad792 into elastic:master Jul 16, 2019
imotov added a commit to imotov/elasticsearch that referenced this pull request Jul 16, 2019
Changes in elastic#44187 introduced some optimization in the way shapes are
generated. These changes were not captured in GeoWKTShapeParserTests.
imotov added a commit that referenced this pull request Jul 17, 2019
Changes in #44187 introduced some optimization in the way shapes are
generated. These changes were not captured in GeoWKTShapeParserTests.

Relates #44187
imotov added a commit that referenced this pull request Jul 17, 2019
Extracts dateline decomposition logic from ShapeBuilder into a separate
utility class that is used on the indexing side. The search side
will be handled as part of another PR at this time we will remove
the decomposition logic from ShapeBuilders as well. This PR also doesn't
change any existing logic including bugs.

Relates to #40908
imotov added a commit that referenced this pull request Jul 17, 2019
Changes in #44187 introduced some optimization in the way shapes are
generated. These changes were not captured in GeoWKTShapeParserTests.

Relates #44187
@imotov imotov deleted the geo-move-coordiantes-to-utils branch May 1, 2020 22:11
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 >refactoring v7.4.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants