Refactor of GeoShapeQuery#13466
Refactor of GeoShapeQuery#13466colings86 wants to merge 0 commit intoelastic:feature/query-refactoringfrom colings86:refactor/geo-shape-query
Conversation
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 :) |
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! |
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