Fix bug filtering collinear points in polygon decomposition (#81155)

elastic · Dec 2, 2021 · 8b07747 · 8b07747
1 parent 32ca555
commit 8b07747
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 19 deletions.
diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPolygonDecomposer.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPolygonDecomposer.java
@@ -106,18 +106,9 @@ private static LinearRing filterRing(LinearRing linearRing) {
         int numPoints = linearRing.length();
         int count = 2;
         for (int i = 1; i < numPoints - 1; i++) {
-            if (linearRing.getLon(i - 1) == linearRing.getLon(i)) {
-                if (linearRing.getLat(i - 1) == linearRing.getLat(i)) {
-                    // same point
-                    continue;
-                }
-                if (linearRing.getLon(i - 1) == linearRing.getLon(i + 1)
-                    && linearRing.getLat(i - 1) > linearRing.getLat(i) != linearRing.getLat(i + 1) > linearRing.getLat(i)) {
-                    // coplanar
-                    continue;
-                }
+            if (skipPoint(linearRing, i) == false) {
+                count++;
             }
-            count++;
         }
         if (numPoints == count) {
             return linearRing;
@@ -129,19 +120,31 @@ private static LinearRing filterRing(LinearRing linearRing) {
         lons[0] = lons[count - 1] = linearRing.getLon(0);
         count = 0;
         for (int i = 1; i < numPoints - 1; i++) {
-            if (linearRing.getLon(i - 1) == linearRing.getLon(i)) {
-                if (linearRing.getLat(i - 1) == linearRing.getLat(i) || linearRing.getLon(i - 1) == linearRing.getLon(i + 1)) {
-                    // filter
-                    continue;
-                }
+            if (skipPoint(linearRing, i) == false) {
+                count++;
+                lats[count] = linearRing.getLat(i);
+                lons[count] = linearRing.getLon(i);
             }
-            count++;
-            lats[count] = linearRing.getLat(i);
-            lons[count] = linearRing.getLon(i);
         }
         return new LinearRing(lons, lats);
     }
 
+    private static boolean skipPoint(LinearRing linearRing, int i) {
+        if (linearRing.getLon(i - 1) == linearRing.getLon(i)) {
+            if (linearRing.getLat(i - 1) == linearRing.getLat(i)) {
+                // same point
+                return true;
+            }
+            if (linearRing.getLon(i - 1) == linearRing.getLon(i + 1)
+                && linearRing.getLat(i - 1) > linearRing.getLat(i) != linearRing.getLat(i + 1) > linearRing.getLat(i)) {
+                // collinear - we only remove points that go in the same direction. So latitudes [1,2,3] we would want
+                // to remove 1 but for [1,2,-1] we don't.
+                return true;
+            }
+        }
+        return false;
+    }
+
     private static void validateHole(LinearRing shell, LinearRing hole) {
         Set<Point> exterior = new HashSet<>();
         Set<Point> interior = new HashSet<>();

diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeometryIndexerTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeometryIndexerTests.java
@@ -193,6 +193,11 @@ public void testPolygonOrientation() throws IOException, ParseException {
             expected("POLYGON ((20 10, -20 10, -20 0, 20 0, 20 10)))"),
             actual(polygon(randomBoolean() ? null : randomBoolean(), 20, 0, 20, 10, -20, 10, -20, 0, 20, 0), randomBoolean())
         );
+
+        assertEquals(
+            expected("POLYGON ((180 29, 180 38, 180 56, 180 53, 178 47, 177 23, 180 29))"),
+            actual("POLYGON ((180 38,  180.0 56, 180.0 53, 178 47, 177 23, 180 29, 180 36, 180 37, 180 38))", randomBoolean())
+        );
     }
 
     public void testInvalidSelfCrossingPolygon() {