Refactors GeoDistanceQueryBuilder/-Parser#13496
Refactors GeoDistanceQueryBuilder/-Parser#13496cbuescher wants to merge 1 commit intoelastic:feature/query-refactoringfrom
Conversation
There was a problem hiding this comment.
Can we remove the lat() and lon() methods? We don't want people setting lat but not lon so it don't makes sense (IMO) to allow them to be set separately
32746b1 to
511ba0e
Compare
|
@colings86 thanks for the comments, added small changes according to this and also minor changes to the GeoDistanceRange/PolygonQ.B. using new utility methods and writable GeoPoint from this PR. Glad if you want to take another look at that. |
There was a problem hiding this comment.
We should throw an IOException with a nice message if the read int is greater than values().length. See ShapeRelation for example of what I mean. We also need unit tests to make sure the ordinals don't change. See ShapeRelationTests for an example of that.
There was a problem hiding this comment.
Great, unit test already exists, I will combine the existing two classes in the following commit.
|
@cbuescher Left some more comments but I think it's close |
|
@colings86 adressed your comments, other than the helper method in ESTestCase which I'd prefer to keep there I think I adressed your comments. Would also like for @MaineC to have a final look since this PR contains many of her own changes. |
There was a problem hiding this comment.
As we are currently trying to get rid of guava as a dependency - switch from using Preconditions to an explicit check?
Splits parsing and Lucene query generation. Switches from storing lat/lon
separately to using GeoPoint instead.
Relates to #10217