Geo: improve handling of out of bounds points in linestrings

server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java

@@ -308,7 +308,7 @@ public static void normalizePoint(double[] lonLat, boolean normLon, boolean norm
         assert lonLat != null && lonLat.length == 2;
 
         normLat = normLat && (lonLat[1] > 90 || lonLat[1] < -90);
-        normLon = normLon && (lonLat[0] > 180 || lonLat[0] < -180);
+        normLon = normLon && (lonLat[0] > 180 || lonLat[0] < -180 || normLat);
 
         if (normLat) {
             lonLat[1] = centeredModulus(lonLat[1], 360);

server/src/main/java/org/elasticsearch/index/mapper/GeoShapeIndexer.java

@@ -225,15 +225,16 @@ protected static double intersection(double p1x, double p2x, double dateline) {
      * Splits the specified line by datelines and adds them to the supplied lines array
      */
     private List<Line> decomposeGeometry(Line line, List<Line> lines) {
-        for (Line part : decompose(line)) {
-            double[] lats = new double[part.length()];
-            double[] lons = new double[part.length()];
-            for (int i = 0; i < part.length(); i++) {
-                lats[i] = normalizeLat(part.getY(i));
-                lons[i] = normalizeLonMinus180Inclusive(part.getX(i));
-            }
-            lines.add(new Line(lons, lats));
+        double[] lons = new double[line.length()];
+        double[] lats = new double[lons.length];
+
+        for (int i = 0; i < lons.length; i++) {
+            double[] lonLat = new double[] {line.getX(i), line.getY(i)};
+            normalizePoint(lonLat,false, true);
+            lons[i] = lonLat[0];
+            lats[i] = lonLat[1];
         }
+        lines.addAll(decompose(lons, lats));
         return lines;
     }
 
@@ -253,12 +254,11 @@ private List<Line> decomposeGeometry(Line line, List<Line> lines) {
     /**
      * Decompose a linestring given as array of coordinates by anti-meridian.
      *
-     * @param line     linestring that should be decomposed
+     * @param lons     longitudes of the linestring that should be decomposed
+     * @param lats     latitudes of the linestring that should be decomposed
      * @return array of linestrings given as coordinate arrays
      */
-    private List<Line> decompose(Line line) {
-        double[] lons = line.getX();
-        double[] lats = line.getY();
+    private List<Line> decompose(double[] lons, double[] lats) {
         int offset = 0;
         ArrayList<Line> parts = new ArrayList<>();
 

server/src/test/java/org/elasticsearch/common/geo/GeometryIndexerTests.java

@@ -38,8 +38,10 @@
 
 import java.io.IOException;
 import java.text.ParseException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.List;
 
 import static org.hamcrest.Matchers.instanceOf;
 
@@ -155,32 +157,59 @@ public void testLine() {
      */
     public double length(Line line) {
         double distance = 0;
+        double[] prev = new double[]{line.getLon(0), line.getLat(0)};
+        GeoUtils.normalizePoint(prev, false, true);
         for (int i = 1; i < line.length(); i++) {
-            distance += Math.sqrt((line.getLat(i) - line.getLat(i - 1)) * (line.getLat(i) - line.getLat(i - 1)) +
-                (line.getLon(i) - line.getLon(i - 1)) * (line.getLon(i) - line.getLon(i - 1)));
+            double[] cur = new double[]{line.getLon(i), line.getLat(i)};
+            GeoUtils.normalizePoint(cur, false, true);
+            distance += Math.sqrt((cur[0] - prev[0]) * (cur[0] - prev[0]) + (cur[1] - prev[1]) * (cur[1] - prev[1]));
+            prev = cur;
         }
         return distance;
     }
 
     /**
-     * A simple tests that generates a random lines crossing anti-merdian and checks that the decomposed segments of this line
+     * Removes the points on the antimeridian that are introduced during linestring decomposition
+     */
+    public static MultiPoint remove180s(MultiPoint points) {
+        List<Point> list = new ArrayList<>();
+        points.forEach(point -> {
+            if (Math.abs(point.getLon()) - 180.0 > 0.000001) {
+                list.add(point);
+            }
+        });
+        if (list.isEmpty()) {
+            return MultiPoint.EMPTY;
+        }
+        return new MultiPoint(list);
+    }
+
+    /**
+     * A randomized test that generates a random lines crossing anti-merdian and checks that the decomposed segments of this line
      * have the same total length (measured using Euclidean distances between neighboring points) as the original line.
+     *
+     * It also extracts all points from these lines, performs normalization of these points and then compares that the resulting
+     * points of line normalization match the points of points normalization with the exception of points that were created on the
+     * antimeridian as the result of line decomposition.
      */
     public void testRandomLine() {
         int size = randomIntBetween(2, 20);
         int shift = randomIntBetween(-2, 2);
         double[] originalLats = new double[size];
         double[] originalLons = new double[size];
 
+        // Generate a random line that goes over poles and stretches beyond -180 and +180
         for (int i = 0; i < size; i++) {
-            originalLats[i] = GeometryTestUtils.randomLat();
+            // from time to time go over poles
+            originalLats[i] = randomInt(4) == 0 ? GeometryTestUtils.randomLat() : GeometryTestUtils.randomLon();
             originalLons[i] = GeometryTestUtils.randomLon() + shift * 360;
             if (randomInt(3) == 0) {
                 shift += randomFrom(-2, -1, 1, 2);
             }
         }
         Line original = new Line(originalLons, originalLats);
 
+        // Check that the length of original and decomposed lines is the same
         Geometry decomposed = indexer.prepareForIndexing(original);
         double decomposedLength = 0;
         if (decomposed instanceof Line) {
@@ -192,8 +221,19 @@ public void testRandomLine() {
                 decomposedLength += length(lines.get(i));
             }
         }
-
         assertEquals("Different Lengths between " + original + " and " + decomposed, length(original), decomposedLength, 0.001);
+
+        // Check that normalized linestring generates the same points as the normalized multipoint based on the same set of points
+        MultiPoint decomposedViaLines = remove180s(GeometryTestUtils.toMultiPoint(decomposed));
+        MultiPoint originalPoints = GeometryTestUtils.toMultiPoint(original);
+        MultiPoint decomposedViaPoint = remove180s(GeometryTestUtils.toMultiPoint(indexer.prepareForIndexing(originalPoints)));
+        assertEquals(decomposedViaPoint.size(), decomposedViaLines.size());
+        for (int i=0; i<decomposedViaPoint.size(); i++) {
+            assertEquals("Difference between decomposing lines " + decomposedViaLines + " and points " + decomposedViaPoint +
+                " at the position " + i, decomposedViaPoint.get(i).getLat(), decomposedViaLines.get(i).getLat(), 0.0001);
+            assertEquals("Difference between decomposing lines " + decomposedViaLines + " and points " + decomposedViaPoint +
+                " at the position " + i, decomposedViaPoint.get(i).getLon(), decomposedViaLines.get(i).getLon(), 0.0001);
+        }
     }
 
     public void testMultiLine() {
@@ -231,6 +271,9 @@ public void testPoint() {
 
         point = new Point(180, 180);
         assertEquals(new Point(0, 0), indexer.prepareForIndexing(point));
+
+        point = new Point(-180, -180);
+        assertEquals(new Point(0, 0), indexer.prepareForIndexing(point));
     }
 
     public void testMultiPoint() {

server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java

@@ -386,6 +386,10 @@ public void testNormalizePointEdgeCases() {
         assertNormalizedPoint(new GeoPoint(180.0, 360.0), new GeoPoint(0.0, 180.0));
         assertNormalizedPoint(new GeoPoint(-90.0, -180.0), new GeoPoint(-90.0, -180.0));
         assertNormalizedPoint(new GeoPoint(90.0, 180.0), new GeoPoint(90.0, 180.0));
+        assertNormalizedPoint(new GeoPoint(100.0, 180.0), new GeoPoint(80.0, 0.0));
+        assertNormalizedPoint(new GeoPoint(100.0, -180.0), new GeoPoint(80.0, 0.0));
+        assertNormalizedPoint(new GeoPoint(-100.0, 180.0), new GeoPoint(-80.0, 0.0));
+        assertNormalizedPoint(new GeoPoint(-100.0, -180.0), new GeoPoint(-80.0, 0.0));
     }
 
     public void testParseGeoPoint() throws IOException {

test/framework/src/main/java/org/elasticsearch/geo/GeometryTestUtils.java

@@ -23,6 +23,7 @@
 import org.elasticsearch.geometry.Circle;
 import org.elasticsearch.geometry.Geometry;
 import org.elasticsearch.geometry.GeometryCollection;
+import org.elasticsearch.geometry.GeometryVisitor;
 import org.elasticsearch.geometry.Line;
 import org.elasticsearch.geometry.LinearRing;
 import org.elasticsearch.geometry.MultiLine;
@@ -34,6 +35,8 @@
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.function.Function;
 
@@ -186,4 +189,73 @@ protected static Geometry randomGeometry(int level, boolean hasAlt) {
         );
         return geometry.apply(hasAlt);
     }
+
+    /**
+     * Extracts all vertices of the supplied geometry
+     */
+    public static MultiPoint toMultiPoint(Geometry geometry) {
+        return geometry.visit(new GeometryVisitor<>() {
+            @Override
+            public MultiPoint visit(Circle circle) throws RuntimeException {
+                throw new UnsupportedOperationException("not supporting circles yet");
+            }
+
+            @Override
+            public MultiPoint visit(GeometryCollection<?> collection) throws RuntimeException {
+                List<Point> points = new ArrayList<>();
+                collection.forEach(geometry -> toMultiPoint(geometry).forEach(points::add));
+                return new MultiPoint(points);
+            }
+
+            @Override
+            public MultiPoint visit(Line line) throws RuntimeException {
+                List<Point> points = new ArrayList<>();
+                for (int i = 0; i < line.length(); i++) {
+                    points.add(new Point(line.getX(i), line.getY(i), line.getZ(i)));
+                }
+                return new MultiPoint(points);
+            }
+
+            @Override
+            public MultiPoint visit(LinearRing ring) throws RuntimeException {
+                return visit((Line) ring);
+            }
+
+            @Override
+            public MultiPoint visit(MultiLine multiLine) throws RuntimeException {
+                return visit((GeometryCollection<?>) multiLine);
+            }
+
+            @Override
+            public MultiPoint visit(MultiPoint multiPoint) throws RuntimeException {
+                return multiPoint;
+            }
+
+            @Override
+            public MultiPoint visit(MultiPolygon multiPolygon) throws RuntimeException {
+                return visit((GeometryCollection<?>) multiPolygon);
+            }
+
+            @Override
+            public MultiPoint visit(Point point) throws RuntimeException {
+                return new MultiPoint(Collections.singletonList(point));
+            }
+
+            @Override
+            public MultiPoint visit(Polygon polygon) throws RuntimeException {
+                List<Geometry> multiPoints = new ArrayList<>();
+                multiPoints.add(toMultiPoint(polygon.getPolygon()));
+                for (int i = 0; i < polygon.getNumberOfHoles(); i++) {
+                    multiPoints.add(toMultiPoint(polygon.getHole(i)));
+                }
+                return toMultiPoint(new GeometryCollection<>(multiPoints));
+            }
+
+            @Override
+            public MultiPoint visit(Rectangle rectangle) throws RuntimeException {
+                return new MultiPoint(Arrays.asList(new Point(rectangle.getMinX(), rectangle.getMinY(), rectangle.getMinZ()),
+                    new Point(rectangle.getMaxX(), rectangle.getMaxY(), rectangle.getMaxZ())));
+            }
+        });
+    }
 }