From 20e1bcf288f0e3bffa29269785df104f7ed39901 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Fri, 14 Jun 2019 12:23:40 -0400 Subject: [PATCH] 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 #43173 --- .../geo/geometry/GeometryUtils.java | 10 ++- .../geo/utils/WellKnownText.java | 85 ++++++++++++------- .../elasticsearch/geo/geometry/LineTests.java | 6 ++ .../geo/geometry/PointTests.java | 6 ++ .../geo/geometry/PolygonTests.java | 25 ++++++ .../common/geo/GeometryParser.java | 2 +- .../function/scalar/geo/GeoShape.java | 2 +- .../scalar/geo/StWkttosqlProcessorTests.java | 9 ++ 8 files changed, 112 insertions(+), 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 index 9a7d4b99d3e53..c7bfa16b16a8d 100644 --- 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 index c489c26e8bca4..f781c8bd0ffa0 100644 --- 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 = ""; - private static final String EOF = "END-OF-STREAM"; - private static final String EOL = "END-OF-LINE"; + private final String 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 parseGeometryCollection(StreamTokenizer stream) throws IOException, ParseException { + private GeometryCollection parseGeometryCollection(StreamTokenizer stream) throws IOException, ParseException { if (nextEmptyOrOpen(stream).equals(EMPTY)) { return GeometryCollection.EMPTY; } @@ -284,7 +293,7 @@ private static GeometryCollection 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; } @@ -292,7 +301,7 @@ private static Point parsePoint(StreamTokenizer stream) throws IOException, Pars 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); } @@ -300,7 +309,7 @@ private static Point parsePoint(StreamTokenizer stream) throws IOException, Pars return pt; } - private static void parseCoordinates(StreamTokenizer stream, ArrayList lats, ArrayList lons, ArrayList alts) + private void parseCoordinates(StreamTokenizer stream, ArrayList lats, ArrayList lons, ArrayList alts) throws IOException, ParseException { parseCoordinate(stream, lats, lons, alts); while (nextCloserOrComma(stream).equals(COMMA)) { @@ -308,19 +317,19 @@ private static void parseCoordinates(StreamTokenizer stream, ArrayList l } } - private static void parseCoordinate(StreamTokenizer stream, ArrayList lats, ArrayList lons, ArrayList alts) + private void parseCoordinate(StreamTokenizer stream, ArrayList lats, ArrayList lons, ArrayList 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,12 +378,13 @@ 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 lats = new ArrayList<>(); ArrayList lons = new ArrayList<>(); ArrayList alts = new ArrayList<>(); parseCoordinates(stream, lats, lons, alts); + closeLinearRingIfCoerced(lats, lons, alts); if (alts.isEmpty()) { return new LinearRing(toArray(lats), toArray(lons)); } else { @@ -382,7 +392,7 @@ private static LinearRing parsePolygonHole(StreamTokenizer stream) throws IOExce } } - 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 lats, ArrayList lons, ArrayList 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() { @Override public String visit(Circle circle) { @@ -600,7 +625,7 @@ public String visit(Rectangle rectangle) { }); } - private static double[] toArray(ArrayList doubles) { + private double[] toArray(ArrayList 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 index 48e6cb8ea11c9..332616db17f5b 100644 --- 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 index 5bb776603da15..715c0bd7c085c 100644 --- 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 index ec80dee7940c4..16d414a81404b 100644 --- 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 index 6308deb084ea1..fe06c3a9c33d2 100644 --- 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/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoShape.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoShape.java index da948cb740306..f9a025ea4f09a 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoShape.java +++ b/x-pack/plugin/sql/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/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/StWkttosqlProcessorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/StWkttosqlProcessorTests.java index fc7b33ae905d7..818897dce343e 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/StWkttosqlProcessorTests.java +++ b/x-pack/plugin/sql/src/test/java/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()); + } }