Geo: Add coerce support to libs/geo WKT parser

Adds support for coercing not closed polygons and ignoring Z value to libs/geo WKT parser. Closes elastic#43173
imotov · Jun 17, 2019 · 20e1bcf · 20e1bcf
1 parent 4fd7a22
commit 20e1bcf
Show file tree

Hide file tree

Showing 8 changed files with 112 additions and 33 deletions.
diff --git a/libs/geo/src/main/java/org/elasticsearch/geo/geometry/GeometryUtils.java b/libs/geo/src/main/java/org/elasticsearch/geo/geometry/GeometryUtils.java
@@ -22,7 +22,7 @@
 /**
  * Geometry-related utility methods
  */
-final class GeometryUtils {
+public final class GeometryUtils {
     /**
      * Minimum longitude value.
      */
@@ -67,4 +67,12 @@ static void checkLongitude(double longitude) {
         }
     }
 
+    public static double checkAltitude(final boolean ignoreZValue, double zValue) {
+        if (ignoreZValue == false) {
+            throw new IllegalArgumentException("found Z value [" + zValue + "] but [ignore_z_value] "
+                + "parameter is [" + ignoreZValue + "]");
+        }
+        return zValue;
+    }
+
 }
diff --git a/libs/geo/src/main/java/org/elasticsearch/geo/utils/WellKnownText.java b/libs/geo/src/main/java/org/elasticsearch/geo/utils/WellKnownText.java
@@ -22,6 +22,7 @@
 import org.elasticsearch.geo.geometry.Circle;
 import org.elasticsearch.geo.geometry.Geometry;
 import org.elasticsearch.geo.geometry.GeometryCollection;
+import org.elasticsearch.geo.geometry.GeometryUtils;
 import org.elasticsearch.geo.geometry.GeometryVisitor;
 import org.elasticsearch.geo.geometry.Line;
 import org.elasticsearch.geo.geometry.LinearRing;
@@ -52,12 +53,20 @@ public class WellKnownText {
     public static final String COMMA = ",";
     public static final String NAN = "NaN";
 
-    private static final String NUMBER = "<NUMBER>";
-    private static final String EOF = "END-OF-STREAM";
-    private static final String EOL = "END-OF-LINE";
+    private final String NUMBER = "<NUMBER>";
+    private final String EOF = "END-OF-STREAM";
+    private final String EOL = "END-OF-LINE";
 
-    public WellKnownText() {
+    private final boolean coerce;
+    private final boolean ignoreZValue;
 
+    public WellKnownText(boolean coerce, boolean ignoreZValue) {
+        this.coerce = coerce;
+        this.ignoreZValue = ignoreZValue;
+    }
+
+    public WellKnownText() {
+        this(true, true);
     }
 
     public String toWKT(Geometry geometry) {
@@ -247,7 +256,7 @@ public Geometry fromWKT(String wkt) throws IOException, ParseException {
     /**
      * parse geometry from the stream tokenizer
      */
-    private static Geometry parseGeometry(StreamTokenizer stream) throws IOException, ParseException {
+    private Geometry parseGeometry(StreamTokenizer stream) throws IOException, ParseException {
         final String type = nextWord(stream).toLowerCase(Locale.ROOT);
         switch (type) {
             case "point":
@@ -272,7 +281,7 @@ private static Geometry parseGeometry(StreamTokenizer stream) throws IOException
         throw new IllegalArgumentException("Unknown geometry type: " + type);
     }
 
-    private static GeometryCollection<Geometry> parseGeometryCollection(StreamTokenizer stream) throws IOException, ParseException {
+    private GeometryCollection<Geometry> parseGeometryCollection(StreamTokenizer stream) throws IOException, ParseException {
         if (nextEmptyOrOpen(stream).equals(EMPTY)) {
             return GeometryCollection.EMPTY;
         }
@@ -284,43 +293,43 @@ private static GeometryCollection<Geometry> parseGeometryCollection(StreamTokeni
         return new GeometryCollection<>(shapes);
     }
 
-    private static Point parsePoint(StreamTokenizer stream) throws IOException, ParseException {
+    private Point parsePoint(StreamTokenizer stream) throws IOException, ParseException {
         if (nextEmptyOrOpen(stream).equals(EMPTY)) {
             return Point.EMPTY;
         }
         double lon = nextNumber(stream);
         double lat = nextNumber(stream);
         Point pt;
         if (isNumberNext(stream)) {
-            pt = new Point(lat, lon, nextNumber(stream));
+            pt = new Point(lat, lon, GeometryUtils.checkAltitude(ignoreZValue, nextNumber(stream)));
         } else {
             pt = new Point(lat, lon);
         }
         nextCloser(stream);
         return pt;
     }
 
-    private static void parseCoordinates(StreamTokenizer stream, ArrayList<Double> lats, ArrayList<Double> lons, ArrayList<Double> alts)
+    private void parseCoordinates(StreamTokenizer stream, ArrayList<Double> lats, ArrayList<Double> lons, ArrayList<Double> alts)
         throws IOException, ParseException {
         parseCoordinate(stream, lats, lons, alts);
         while (nextCloserOrComma(stream).equals(COMMA)) {
             parseCoordinate(stream, lats, lons, alts);
         }
     }
 
-    private static void parseCoordinate(StreamTokenizer stream, ArrayList<Double> lats, ArrayList<Double> lons, ArrayList<Double> alts)
+    private void parseCoordinate(StreamTokenizer stream, ArrayList<Double> lats, ArrayList<Double> lons, ArrayList<Double> alts)
         throws IOException, ParseException {
         lons.add(nextNumber(stream));
         lats.add(nextNumber(stream));
         if (isNumberNext(stream)) {
-            alts.add(nextNumber(stream));
+            alts.add(GeometryUtils.checkAltitude(ignoreZValue, nextNumber(stream)));
         }
         if (alts.isEmpty() == false && alts.size() != lons.size()) {
             throw new ParseException("coordinate dimensions do not match: " + tokenString(stream), stream.lineno());
         }
     }
 
-    private static MultiPoint parseMultiPoint(StreamTokenizer stream) throws IOException, ParseException {
+    private MultiPoint parseMultiPoint(StreamTokenizer stream) throws IOException, ParseException {
         String token = nextEmptyOrOpen(stream);
         if (token.equals(EMPTY)) {
             return MultiPoint.EMPTY;
@@ -340,7 +349,7 @@ private static MultiPoint parseMultiPoint(StreamTokenizer stream) throws IOExcep
         return new MultiPoint(Collections.unmodifiableList(points));
     }
 
-    private static Line parseLine(StreamTokenizer stream) throws IOException, ParseException {
+    private Line parseLine(StreamTokenizer stream) throws IOException, ParseException {
         String token = nextEmptyOrOpen(stream);
         if (token.equals(EMPTY)) {
             return Line.EMPTY;
@@ -356,7 +365,7 @@ private static Line parseLine(StreamTokenizer stream) throws IOException, ParseE
         }
     }
 
-    private static MultiLine parseMultiLine(StreamTokenizer stream) throws IOException, ParseException {
+    private MultiLine parseMultiLine(StreamTokenizer stream) throws IOException, ParseException {
         String token = nextEmptyOrOpen(stream);
         if (token.equals(EMPTY)) {
             return MultiLine.EMPTY;
@@ -369,20 +378,21 @@ private static MultiLine parseMultiLine(StreamTokenizer stream) throws IOExcepti
         return new MultiLine(Collections.unmodifiableList(lines));
     }
 
-    private static LinearRing parsePolygonHole(StreamTokenizer stream) throws IOException, ParseException {
+    private LinearRing parsePolygonHole(StreamTokenizer stream) throws IOException, ParseException {
         nextOpener(stream);
         ArrayList<Double> lats = new ArrayList<>();
         ArrayList<Double> lons = new ArrayList<>();
         ArrayList<Double> alts = new ArrayList<>();
         parseCoordinates(stream, lats, lons, alts);
+        closeLinearRingIfCoerced(lats, lons, alts);
         if (alts.isEmpty()) {
             return new LinearRing(toArray(lats), toArray(lons));
         } else {
             return new LinearRing(toArray(lats), toArray(lons), toArray(alts));
         }
     }
 
-    private static Polygon parsePolygon(StreamTokenizer stream) throws IOException, ParseException {
+    private Polygon parsePolygon(StreamTokenizer stream) throws IOException, ParseException {
         if (nextEmptyOrOpen(stream).equals(EMPTY)) {
             return Polygon.EMPTY;
         }
@@ -395,6 +405,7 @@ private static Polygon parsePolygon(StreamTokenizer stream) throws IOException,
         while (nextCloserOrComma(stream).equals(COMMA)) {
             holes.add(parsePolygonHole(stream));
         }
+        closeLinearRingIfCoerced(lats, lons, alts);
         LinearRing shell;
         if (alts.isEmpty()) {
             shell = new LinearRing(toArray(lats), toArray(lons));
@@ -408,7 +419,21 @@ private static Polygon parsePolygon(StreamTokenizer stream) throws IOException,
         }
     }
 
-    private static MultiPolygon parseMultiPolygon(StreamTokenizer stream) throws IOException, ParseException {
+    private void closeLinearRingIfCoerced(ArrayList<Double> lats, ArrayList<Double> lons, ArrayList<Double> alts) {
+        if (coerce && lats.isEmpty() == false && lons.isEmpty() == false) {
+            int last = lats.size() - 1;
+            if (!lats.get(0).equals(lats.get(last)) || !lons.get(0).equals(lons.get(last)) ||
+                (alts.isEmpty() == false && !alts.get(0).equals(alts.get(last)))) {
+                lons.add(lons.get(0));
+                lats.add(lats.get(0));
+                if (alts.isEmpty() == false) {
+                    alts.add(alts.get(0));
+                }
+            }
+        }
+    }
+
+    private MultiPolygon parseMultiPolygon(StreamTokenizer stream) throws IOException, ParseException {
         String token = nextEmptyOrOpen(stream);
         if (token.equals(EMPTY)) {
             return MultiPolygon.EMPTY;
@@ -421,7 +446,7 @@ private static MultiPolygon parseMultiPolygon(StreamTokenizer stream) throws IOE
         return new MultiPolygon(Collections.unmodifiableList(polygons));
     }
 
-    private static Rectangle parseBBox(StreamTokenizer stream) throws IOException, ParseException {
+    private Rectangle parseBBox(StreamTokenizer stream) throws IOException, ParseException {
         if (nextEmptyOrOpen(stream).equals(EMPTY)) {
             return Rectangle.EMPTY;
         }
@@ -438,7 +463,7 @@ private static Rectangle parseBBox(StreamTokenizer stream) throws IOException, P
     }
 
 
-    private static Circle parseCircle(StreamTokenizer stream) throws IOException, ParseException {
+    private Circle parseCircle(StreamTokenizer stream) throws IOException, ParseException {
         if (nextEmptyOrOpen(stream).equals(EMPTY)) {
             return Circle.EMPTY;
         }
@@ -457,7 +482,7 @@ private static Circle parseCircle(StreamTokenizer stream) throws IOException, Pa
     /**
      * next word in the stream
      */
-    private static String nextWord(StreamTokenizer stream) throws ParseException, IOException {
+    private String nextWord(StreamTokenizer stream) throws ParseException, IOException {
         switch (stream.nextToken()) {
             case StreamTokenizer.TT_WORD:
                 final String word = stream.sval;
@@ -472,7 +497,7 @@ private static String nextWord(StreamTokenizer stream) throws ParseException, IO
         throw new ParseException("expected word but found: " + tokenString(stream), stream.lineno());
     }
 
-    private static double nextNumber(StreamTokenizer stream) throws IOException, ParseException {
+    private double nextNumber(StreamTokenizer stream) throws IOException, ParseException {
         if (stream.nextToken() == StreamTokenizer.TT_WORD) {
             if (stream.sval.equalsIgnoreCase(NAN)) {
                 return Double.NaN;
@@ -487,7 +512,7 @@ private static double nextNumber(StreamTokenizer stream) throws IOException, Par
         throw new ParseException("expected number but found: " + tokenString(stream), stream.lineno());
     }
 
-    private static String tokenString(StreamTokenizer stream) {
+    private String tokenString(StreamTokenizer stream) {
         switch (stream.ttype) {
             case StreamTokenizer.TT_WORD:
                 return stream.sval;
@@ -501,13 +526,13 @@ private static String tokenString(StreamTokenizer stream) {
         return "'" + (char) stream.ttype + "'";
     }
 
-    private static boolean isNumberNext(StreamTokenizer stream) throws IOException {
+    private boolean isNumberNext(StreamTokenizer stream) throws IOException {
         final int type = stream.nextToken();
         stream.pushBack();
         return type == StreamTokenizer.TT_WORD;
     }
 
-    private static String nextEmptyOrOpen(StreamTokenizer stream) throws IOException, ParseException {
+    private String nextEmptyOrOpen(StreamTokenizer stream) throws IOException, ParseException {
         final String next = nextWord(stream);
         if (next.equals(EMPTY) || next.equals(LPAREN)) {
             return next;
@@ -516,28 +541,28 @@ private static String nextEmptyOrOpen(StreamTokenizer stream) throws IOException
             + " but found: " + tokenString(stream), stream.lineno());
     }
 
-    private static String nextCloser(StreamTokenizer stream) throws IOException, ParseException {
+    private String nextCloser(StreamTokenizer stream) throws IOException, ParseException {
         if (nextWord(stream).equals(RPAREN)) {
             return RPAREN;
         }
         throw new ParseException("expected " + RPAREN + " but found: " + tokenString(stream), stream.lineno());
     }
 
-    private static String nextComma(StreamTokenizer stream) throws IOException, ParseException {
+    private String nextComma(StreamTokenizer stream) throws IOException, ParseException {
         if (nextWord(stream).equals(COMMA) == true) {
             return COMMA;
         }
         throw new ParseException("expected " + COMMA + " but found: " + tokenString(stream), stream.lineno());
     }
 
-    private static String nextOpener(StreamTokenizer stream) throws IOException, ParseException {
+    private String nextOpener(StreamTokenizer stream) throws IOException, ParseException {
         if (nextWord(stream).equals(LPAREN)) {
             return LPAREN;
         }
         throw new ParseException("expected " + LPAREN + " but found: " + tokenString(stream), stream.lineno());
     }
 
-    private static String nextCloserOrComma(StreamTokenizer stream) throws IOException, ParseException {
+    private String nextCloserOrComma(StreamTokenizer stream) throws IOException, ParseException {
         String token = nextWord(stream);
         if (token.equals(COMMA) || token.equals(RPAREN)) {
             return token;
@@ -546,7 +571,7 @@ private static String nextCloserOrComma(StreamTokenizer stream) throws IOExcepti
             + " but found: " + tokenString(stream), stream.lineno());
     }
 
-    public static String getWKTName(Geometry geometry) {
+    private static String getWKTName(Geometry geometry) {
         return geometry.visit(new GeometryVisitor<String, RuntimeException>() {
             @Override
             public String visit(Circle circle) {
@@ -600,7 +625,7 @@ public String visit(Rectangle rectangle) {
         });
     }
 
-    private static double[] toArray(ArrayList<Double> doubles) {
+    private double[] toArray(ArrayList<Double> doubles) {
         return doubles.stream().mapToDouble(i -> i).toArray();
     }
 

diff --git a/libs/geo/src/test/java/org/elasticsearch/geo/geometry/LineTests.java b/libs/geo/src/test/java/org/elasticsearch/geo/geometry/LineTests.java
@@ -54,4 +54,10 @@ public void testInitValidation() {
         ex = expectThrows(IllegalArgumentException.class, () -> new Line(new double[]{1, 100, 3, 1}, new double[]{3, 4, 5, 3}));
         assertEquals("invalid latitude 100.0; must be between -90.0 and 90.0", ex.getMessage());
     }
+
+    public void testWKTValidation() {
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
+            () -> new WellKnownText(randomBoolean(), false).fromWKT("linestring (3 1 6, 4 2 5)"));
+        assertEquals("found Z value [6.0] but [ignore_z_value] parameter is [false]", ex.getMessage());
+    }
 }
diff --git a/libs/geo/src/test/java/org/elasticsearch/geo/geometry/PointTests.java b/libs/geo/src/test/java/org/elasticsearch/geo/geometry/PointTests.java
@@ -49,4 +49,10 @@ public void testInitValidation() {
         ex = expectThrows(IllegalArgumentException.class, () -> new Point(10, 500));
         assertEquals("invalid longitude 500.0; must be between -180.0 and 180.0", ex.getMessage());
     }
+
+    public void testWKTValidation() {
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
+            () -> new WellKnownText(randomBoolean(), false).fromWKT("point (20.0 10.0 100.0)"));
+        assertEquals("found Z value [100.0] but [ignore_z_value] parameter is [false]", ex.getMessage());
+    }
 }
diff --git a/libs/geo/src/test/java/org/elasticsearch/geo/geometry/PolygonTests.java b/libs/geo/src/test/java/org/elasticsearch/geo/geometry/PolygonTests.java
@@ -43,6 +43,15 @@ public void testBasicSerialization() throws IOException, ParseException {
         assertEquals(new Polygon(new LinearRing(new double[]{1, 2, 3, 1}, new double[]{3, 4, 5, 3}, new double[]{5, 4, 3, 5})),
             wkt.fromWKT("polygon ((3 1 5, 4 2 4, 5 3 3, 3 1 5))"));
 
+        // Auto closing in coerce mode
+        assertEquals(new Polygon(new LinearRing(new double[]{1, 2, 3, 1}, new double[]{3, 4, 5, 3})),
+            wkt.fromWKT("polygon ((3 1, 4 2, 5 3))"));
+        assertEquals(new Polygon(new LinearRing(new double[]{1, 2, 3, 1}, new double[]{3, 4, 5, 3}, new double[]{5, 4, 3, 5})),
+            wkt.fromWKT("polygon ((3 1 5, 4 2 4, 5 3 3))"));
+        assertEquals(new Polygon(new LinearRing(new double[]{1, 2, 3, 1}, new double[]{3, 4, 5, 3}),
+            Collections.singletonList(new LinearRing(new double[]{1.5, 1.5, 1.0, 1.5}, new double[]{0.5, 2.5, 2.0, 0.5}))),
+            wkt.fromWKT("polygon ((3 1, 4 2, 5 3, 3 1), (0.5 1.5, 2.5 1.5, 2.0 1.0))"));
+
         assertEquals("polygon EMPTY", wkt.toWKT(Polygon.EMPTY));
         assertEquals(Polygon.EMPTY, wkt.fromWKT("polygon EMPTY)"));
     }
@@ -61,4 +70,20 @@ public void testInitValidation() {
                 Collections.singletonList(new LinearRing(new double[]{1, 2, 3, 1}, new double[]{3, 4, 5, 3}))));
         assertEquals("holes must have the same number of dimensions as the polygon", ex.getMessage());
     }
+
+    public void testWKTValidation() {
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
+            () -> new WellKnownText(false, true).fromWKT("polygon ((3 1 5, 4 2 4, 5 3 3))"));
+        assertEquals("first and last points of the linear ring must be the same (it must close itself): " +
+            "lats[0]=1.0 lats[2]=3.0 lons[0]=3.0 lons[2]=5.0 alts[0]=5.0 alts[2]=3.0", ex.getMessage());
+
+        ex = expectThrows(IllegalArgumentException.class,
+            () -> new WellKnownText(randomBoolean(), false).fromWKT("polygon ((3 1 5, 4 2 4, 5 3 3, 3 1 5))"));
+        assertEquals("found Z value [5.0] but [ignore_z_value] parameter is [false]", ex.getMessage());
+
+        ex = expectThrows(IllegalArgumentException.class,
+            () -> new WellKnownText(false, randomBoolean()).fromWKT("polygon ((3 1, 4 2, 5 3, 3 1), (0.5 1.5, 2.5 1.5, 2.0 1.0))"));
+        assertEquals("first and last points of the linear ring must be the same (it must close itself): " +
+            "lats[0]=1.5 lats[2]=1.0 lons[0]=0.5 lons[2]=2.0", ex.getMessage());
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeometryParser.java b/server/src/main/java/org/elasticsearch/common/geo/GeometryParser.java
@@ -37,7 +37,7 @@ public final class GeometryParser {
 
     public GeometryParser(boolean rightOrientation, boolean coerce, boolean ignoreZValue) {
         geoJsonParser = new GeoJson(rightOrientation, coerce, ignoreZValue);
-        wellKnownTextParser = new WellKnownText();
+        wellKnownTextParser = new WellKnownText(coerce, ignoreZValue);
     }
 
     /**

diff --git a/...ql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoShape.java b/...ql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoShape.java
@@ -51,7 +51,7 @@ public class GeoShape implements ToXContentFragment, NamedWriteable {
 
     private static final GeometryParser GEOMETRY_PARSER = new GeometryParser(true, true, true);
 
-    private static final WellKnownText WKT_PARSER = new WellKnownText();
+    private static final WellKnownText WKT_PARSER = new WellKnownText(true, true);
 
     public GeoShape(double lon, double lat) {
         shape = new Point(lat, lon);

diff --git a/.../org/elasticsearch/xpack/sql/expression/function/scalar/geo/StWkttosqlProcessorTests.java b/.../org/elasticsearch/xpack/sql/expression/function/scalar/geo/StWkttosqlProcessorTests.java
@@ -39,4 +39,13 @@ public void testTypeCheck() {
         siae = expectThrows(SqlIllegalArgumentException.class, () -> procPoint.process("point (10 10"));
         assertEquals("Cannot parse [point (10 10] as a geo_shape value", siae.getMessage());
     }
+
+    public void testCoerce() {
+        StWkttosqlProcessor proc = new StWkttosqlProcessor();
+        assertNull(proc.process(null));
+        Object result = proc.process("POLYGON ((3 1 5, 4 2 4, 5 3 3))");
+        assertThat(result, instanceOf(GeoShape.class));
+        GeoShape geoShape = (GeoShape) result;
+        assertEquals("polygon ((3.0 1.0 5.0, 4.0 2.0 4.0, 5.0 3.0 3.0, 3.0 1.0 5.0))", geoShape.toString());
+    }
 }