WIP: GeoDistanceQueryBuilder/-Parser refactoring #12283
Conversation
|
Wrt. to parsing consecutive boolean values out of json - this might become obsolete anyway as soon as #12300 is in. |
There was a problem hiding this comment.
Maybe null-check here, otherwise we might get NPEs later.
|
@MaineC did a first quick round of review. Not sure what to do regarding the question if we need a valid indexService. If this is related to the "optimizeBbox" option, I saw that theres also a "none" option in the resulting Lucene query, maybe that would help? Don't properly understand what that option does to be honest. |
|
Thanks for your comments - will incorporate them into the PR tomorrow.
... it is - but only partially: Even if setting this option to the value that worked for the GeoBoundingBoxQueryBuilder tests it's still impossible to run the doQuery method. The problem is in the following line: which runs into an AssertionError: java.lang.AssertionError I've had the same issue with the GeoBoundingBoxQuery test, except that there was at least one code path that didn't run into the problem above. Maybe this is one of those queries that really need at least a single node pulled up to be tested?
s/seems to be solved/will be solved as soon as the PR I reference above is in, so far it's still open, no? |
MaineC
force-pushed
the
feature/geo-distance-refactoring
branch
from
e396219 to
efd4de1
Compare
This add equals, hashcode, read/write methods, validation, separates toQuery and JSON parsing and adds serialization and query generation tests. Deprecates two types of initializing the bounding box: In our documentation we speak about specifying top/left and bottom/right corner of a bounding box. Here we also allow for top/right and bottom/left. This adds not only to the amount of code but also testing needed w/o too much benefit for the user other than more chances to confuse top/right/bottom/left/latitude/longitude IMHO. Missing: The toQuery method with type set to "indexed" is not tested at the moment. Cleanup changes unrelated to base refactoring: * Switched from type String to enum for types in GeoBoundingBoxQueryBuilder. * Switched to using type GeoPoint for storing the bounding box coordinates instead of array of double values. Relates to elastic#10217 for the query refactoring part. Relates to elastic#12016 for how missing mappings are handled. Adds a utility class for generating random geo data. Adds some missing documentation. Extend test to MEMORY type config
Splits parsing and Lucene query generation. Switches from storing lat/lon separately to using GeoPoint instead. Creates first test stub. Relates to elastic#10217 Add serialisation and basic testing. Adds test query generation, readFrom, writeTo, equals, hashCode. Issues: * testFromXContent fails as the current doXContent does not serialise normalizeLat/normalizeLon in a way fromXContent can parse correctly. * testToQuery fails as query generation seems to rely on an actual node being pulled up (much like we already had in the GeoBoundingBox case, only that this time I see no code path that doesn't need the node) Add basic validation
MaineC
force-pushed
the
feature/geo-distance-refactoring
branch
from
efd4de1 to
9c6cc4a
Compare
|
Needs another rebase, also needs to have unit test fixed. Caveat: As validation and some tests will rely on what was already done for GeoBoundingBox the latter changes were merged into the branch this PR is based on. This should be merged after the bounding box PR was merged. |
|
Will be continued in a separate PR by @cbuescher |
clintongormley
added
:Search/Search
3 participants
This is pretty much work in progress (e.g. misses validation). It comes with two questions/issues I ran into and need help with/ feedback for:
elasticsearch/core/src/main/java/org/elasticsearch/index/query/GeoDistanceQueryParser.java
Line 128 in 9fa69bd