Refactor of GeoShapeQuery #13466
Refactor of GeoShapeQuery #13466
Conversation
| INTERSECTS("intersects"), | ||
| DISJOINT("disjoint"), | ||
| WITHIN("within"); | ||
| INTERSECTS((byte) 0, "intersects"), |
There was a problem hiding this comment.
can we just use the ordinal() for serialization?
There was a problem hiding this comment.
Sure, should I add a simple unit test to make sure the order (and ordinals) of these enum values don't change?
|
I did a first round of review... looks awesome already |
|
@s1monw I pushed a commit which should address most of your comments and replied to the others |
|
@s1monw actually hold off reviewing again for now, looks like there are test failures I need to sort out |
|
@s1monw ok good to go now :) |
| @@ -37,6 +42,37 @@ | |||
| this.relationName = relationName; | |||
| } | |||
|
|
|||
| @Override | |||
| public ShapeRelation readFrom(StreamInput in) throws IOException { | |||
There was a problem hiding this comment.
for my future reference (we had a similar discussion just today on some other PR), why don't we simply rely on the enum ordinal() here like we already do elsewhere?
There was a problem hiding this comment.
folks are generally concnered that the ordinal might cahnge if you change the order of the enum. I usually write explicit tests for it but with switch / case it's declerative and that should protect from it?
There was a problem hiding this comment.
but it feels like we are back with our own ordinal then, more LOC too with two switches compared to the previous Operator impl :) can't we just handle bw comp once we actually change the enum? of course the main issue is remembering about this, and tests are a good way to remember...
There was a problem hiding this comment.
++ for ordinal and tests then
|
LGTM thanks man! |
clintongormley
added
:Search/Search
Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.
Relates to #10217
PR goes against the query-refactoring branch