Refactor of GeoPolygonQuery #13426
Refactor of GeoPolygonQuery #13426
Conversation
| @Override | ||
| protected Query doToQuery(QueryShardContext context) throws IOException { | ||
|
|
||
| if (!shell.get(shell.size() - 1).equals(shell.get(0))) { |
There was a problem hiding this comment.
I'd slightly prefer to do this before or into a copy of the shape? I think only the latter is possible though, not sure. Also, maybe it is not a big deal to auto close and change the original shape...
There was a problem hiding this comment.
I don't think it's going to be possible to do this before the toQuery method. I can copy the shape but these shapes can be big and complex so I'd only want to do that if we need to. I guess one side effect of doing this here is that two QueryBuilders which are identical before the toQuery method is called (pass hashCode and Equals checks) would then not be equal after calling toQuery on only one of them. So actually maybe we should make a copy here? I'm torn
There was a problem hiding this comment.
I am thinking it is not worth it. Hopefully it won't cause problems to our test infra as we don't check the object after calling toQuery...... should we have done it in the first place? :)
|
left a few minor comments, looks good already though |
|
@javanna I pushed a commit to address your comments |
|
|
||
| private Boolean ignoreMalformed; | ||
| public GeoPolygonQueryBuilder(String fieldName) { | ||
| this.fieldName = fieldName; |
There was a problem hiding this comment.
can we just call this(fieldName, new ArrayList()); instead?
|
left some minors - looks great |
|
@s1monw thanks for the review. I've push a new commit that should address all your comments |
colings86
changed the title
|
LGTM |
| // percolation queries we only ignore_malformed on 2.x created indexes | ||
| if (!indexCreatedBeforeV2_0 && !ignoreMalformed) { | ||
| for (GeoPoint point : shell) { | ||
| if (point.lat() > 90.0 || point.lat() < -90.0) { |
There was a problem hiding this comment.
Maybe add a note to switch to https://github.com/elastic/elasticsearch/pull/11969/files#diff-6236367b2788d500f308334adc6d4d7fR70 once that PR is in?
There was a problem hiding this comment.
I'll make a new change to use this once that PR has been pushed
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
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