-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Refactor GeoShape tests to GeoShape and GeoPoint #50737
Refactor GeoShape tests to GeoShape and GeoPoint #50737
Conversation
in Class org.elasticsearch.search.geo.GeoPointShapeQueryTests
…tion raised, see org.elasticsearch.index.query.QueryShardContext.toQuery
in GeoPointShapeQueryTests
…ShapeQueryTests.java
Pinging @elastic/es-analytics-geo (:Analytics/Geo) |
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this PR wants to pave the way to support geo_shape queries with geo_point fields. Should be the tests on points part of the abstract class?
server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java
Show resolved
Hide resolved
That is looking good. Could you resolve conflicts some can see if test passes. |
GeoShapeQueryTests refactored adding superclass and sibling to accomodate new test coverage as part of adaptation of geo_shape query to work on geo_point fields
@elasticmachine run elasticsearch-ci/2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Refactor of GeoShape tests to separate GeoShape and GeoPoint as preliminary step prior to implementing #48928
Added new methods to avoid duplicate code for mapping creation
All existing tests passed post refactor in org.elasticsearch.search.geo.*
New placeholder test in GeoPointShapeQueryTests is non-failing pending 48928
Note commit 24b9535 has been reset by d7f2218 this will be reincorporated as part of 48928