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] Refactor GeoShapeQueryBuilder to derive from AbstractGeometryQueryBuilder #44780

Merged
merged 7 commits into from
Jul 24, 2019

Conversation

nknize
Copy link
Contributor

@nknize nknize commented Jul 24, 2019

Refactors GeoShapeQueryBuilder to derive from a new AbstractShapeQueryBuilder that provides common parsing and build logic for spatial shapes. This will allow development of custom shape queries by extending AbstractShapeQueryBuilder, preventing the duplication of common shape query logic.

note: #44715 further deprecates ShapeBuilder to make the QueryBuilder more spatial agnostic and will be included upon merge.

Relates to #40908

Refactors GeoShapeQueryBuilder to derive from a new AbstractShapeQueryBuilder that provides common parsing and build logic for spatial shapes. This will allow development of custom shape queries by extending AbstractShapeQueryBuilder preventing duplication of common shape query logic.
@nknize nknize added :Analytics/Geo Indexing, search aggregations of geo points and shapes >refactoring v8.0.0 v7.4.0 labels Jul 24, 2019
@nknize nknize requested a review from imotov July 24, 2019 01:11
@nknize
Copy link
Contributor Author

nknize commented Jul 24, 2019

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2

@imotov
Copy link
Contributor

imotov commented Jul 24, 2019

It looks good in general but let's get #44715 in first and merge it into this one. I think it should dramatically simplify GeoShapeQueryBuilder ideally leaving only legacy-specific logic there. I think the goal should be to move as much non-legacy logic fromGeoShapeQueryBuilder into GeometryIndexer as possible, otherwise it will force us to add the corresponding XYShape-specific logic to XYShapeQueryBuilder which is part of HL Rest client.

@nknize
Copy link
Contributor Author

nknize commented Jul 24, 2019

I agree on getting #44715 in first (I'm already merging it with this dev branch as part of the PR review).

We're also going to need to be careful about naming because the field type that exposes Lucene's LatLonShape is called geo_shape and the field type that exposes Lucene's XYShape is called geometry so we risk getting confused by terminology where lat,lon geometries are called geo and x,y geometries are called geometry. GeometryIndexer is a good example, I think we'll need to carefully document that class to make it clear that it's intended for lat,lon geometries (or rename it to something like GeodeticIndexer differentiating between Geodetic lat, lon and Geocentric XYZ, or tangential plane XY).

@imotov
Copy link
Contributor

imotov commented Jul 24, 2019

I merged #44715 into master. Thanks for the review!

@nknize
Copy link
Contributor Author

nknize commented Jul 24, 2019

np; and I just merged those changes from master w/ this PR. Let's see how CI responds and I think this will be good to go. Per our team discuss this morning I renamed AbstractShapeQueryBuilder to AbstractGeometryQueryBuilder to try and keep the naming conventions clearer. Now we have GeoShapeQueryBuilder (and soon ShapeQueryBuilder) that will derive from this new AbstractGeometryQueryBuilder. I also like how AbstractGeometryQueryBuilder now has no dependencies on jts or spatial4j, only GeoShapeQueryBuilder does for legacy purposes. It will make it easier to remove those dependencies once we get there.

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

private static Shape buildS4J(Geometry geometry) {
return geometryToShapeBuilder(geometry).buildS4J();
}

private Query getVectorQuery(QueryShardContext context, Geometry queryShape) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the next iteration I am going to move this into AbstractGeometryIndexer and make it obtainable from QueryShardContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to also @deprecate it in 8.0 (master) in advance of its removal in 9.0?

Copy link
Contributor Author

@nknize nknize Jul 24, 2019

Choose a reason for hiding this comment

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

Oh... oops.. Sorry, you were referring to getVectorQuery. I think that's a great idea.

@nknize nknize changed the title [Geo] Refactor GeoShapeQueryBuilder to derive from AbstractShapeQueryBuilder [Geo] Refactor GeoShapeQueryBuilder to derive from AbstractGeometryQueryBuilder Jul 24, 2019
@nknize nknize merged commit 0482894 into elastic:master Jul 24, 2019
nknize added a commit that referenced this pull request Jul 25, 2019
…eryBuilder (#44780)

Refactors GeoShapeQueryBuilder to derive from a new AbstractGeometryQueryBuilder that provides common parsing and build logic for spatial geometries. This will allow development of custom geometry queries by extending AbstractGeometryQueryBuilder preventing duplication of common spatial query logic.
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.

3 participants