Refactors GeoDistanceQueryBuilder/-Parser #13496
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
cbuescher
force-pushed
the
feature/query-refactoring-geodistance
branch
2 times, most recently
from
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?
There was a problem hiding this comment.
I think all of this went away in the latest GeoBoundingBox PR - see e.g. there for replacement functionality: https://github.com/elastic/elasticsearch/pull/11969/files#diff-c34f179cb205333bd004736b1a8a71caR46
|
Left a few comments. Thanks for adopting this PR. |
|
@MaineC addressed your comments, let me know if you think its okay now. |
There was a problem hiding this comment.
this can't be null anymore?
|
left minor comments LGTM and doesn't need another review IMO |
Splits parsing and Lucene query generation. Switches from storing lat/lon separately to using GeoPoint instead. Relates to elastic#10217
cbuescher
force-pushed
the
feature/query-refactoring-geodistance
branch
from
128ce19 to
353fab9
Compare
|
Merged with feature branch with 8fb1aa9. |
clintongormley
added
:Search/Search
6 participants
Splits parsing and Lucene query generation. Switches from storing lat/lon
separately to using GeoPoint instead.
Relates to #10217