Handle case where the hole vertex is south of the containing polygon(s) #27685
Conversation
Normally the hole is assigned to the component of the first edge to the south of one of its vertices, but if the chosen hole vertex is south of everything then the binary search returns -1 yielding an ArrayIndexOutOfBoundsException. Instead, assign the vertex to the component of the first edge to its north. Subsequent validation catches the fact that the hole is outside its component. Fixes elastic#25933
DaveCTurner
added
:Analytics/Geo
| pb.hole(new LineStringBuilder(new CoordinatesBuilder().coordinate(4, 2).coordinate(3, 1).coordinate(4, 1).close())); | ||
| pb.build(); | ||
| } catch (InvalidShapeException e) { | ||
| assertEquals("Hole lies outside shell at or near point (4.0, 1.0, NaN)", e.getMessage()); |
There was a problem hiding this comment.
nit: there is the neat expectThrows() utility in LuceneTestCase that makes test for exceptions a little more succinct. MIght help here (and in the other test) as well
There was a problem hiding this comment.
D'oh, I forgot about that. I pushed dc88dac
cbuescher
left a comment
cbuescher
left a comment
There was a problem hiding this comment.
@DaveCTurner I had a look at the PR and left a small comment, other than that I'm not too familiar with the validation of polygons wt. holes and cannot comment with much certainty on the proposed fix. I think @nknize should have a look at this as well.
|
@nknize does this look ok to you? |
| throw new InvalidShapeException("Invalid shape: Hole is not within polygon"); | ||
| } | ||
| final int index = -((sharedVertex) ? 0 : pos+2); | ||
| final int index = sharedVertex || pos == -1 ? 0 : -(pos+2); |
There was a problem hiding this comment.
@DaveCTurner thanks for walking me trough this f2f, it took some time for me to understand but now I think I understand this whole method a lot better. This looks right to me now after some longer thought, I think we should add some comments to this part after that explains the three(?) cases that might be true at this point and what that means for the index of the edge we assign this component to.
cbuescher
left a comment
cbuescher
left a comment
There was a problem hiding this comment.
@DaveCTurner thanks a lot for the additional comments. More verbose but definitely more readable IMHO.
…s) (elastic#27685) Normally the hole is assigned to the component of the first edge to the south of one of its vertices, but if the chosen hole vertex is south of everything then the binary search returns -1 yielding an ArrayIndexOutOfBoundsException. Instead, assign the vertex to the component of the first edge to its north. Subsequent validation catches the fact that the hole is outside its component. Fixes elastic#25933
* es/6.x: (170 commits) Allow TrimFilter to be used in custom normalizers (#27758) recovery from snapshot should fill gaps (#27850) Remove unused class PreBuiltTokenFilters (#27839) Reject scroll query if size is 0 (#22552) (#27842) Mutes ‘Rollover no condition matched’ YAML test Make randomNonNegativeLong() draw from a uniform distribution (#27856) Adapt rest test after backport. Relates #27833 Handle case where the hole vertex is south of the containing polygon(s) (#27685) Move range field mapper back to core Fix publication of elasticsearch-cli to Maven Do not use system properties when building the HttpAsyncClient (#27829) Optimize version map for append-only indexing (#27752) Add NioGroup for use in different transports (#27737) adapt field collapsing skip test version. relates #27833 Add version support for inner hits in field collapsing (#27822) (#27833) Clarify that number of threads is set by packages Register HTTP read timeout setting Fixes Checkstyle Remove `operationThreaded` from Java API (#27836) Fixes failing BytesSizeValues tests ...
3 participants
Normally the hole is assigned to the component of the first edge to the south
of one of its vertices, but if the chosen hole vertex is south of everything
then the binary search returns -1 yielding an
ArrayIndexOutOfBoundsException.Instead, assign the vertex to the component of the first edge to its north.
Subsequent validation catches the fact that the hole is outside its component.
Fixes #25933