Skip to content

Commit 844ee68

Browse files
craigtavernercraigtaverner
authored
Added geometry validation for GEO types to exit early on invalid latitudes (elastic#128259)
It turns out in elastic#128234 that Lucene pushdown on ST_DISTANCE with invalid points (latitude out of range) will cause all documents to be returned, since DISJOINT on an invalid circle is true for all documents. We could either add an extra check for spatial pushdown that the geometries are valid, or add validation at geometry creation. This second option is much easier to implement, and a more comprehensive approach, as it prevents invalid geometries in many more places, hopefully reducing the likelihood of subtle and obscure bugs like elastic#128234 happening in future.
1 parent d815e28 commit 844ee68

File tree

7 files changed

+109
-15
lines changed

7 files changed

+109
-15
lines changed

‎docs/changelog/128259.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 128259
2+
summary: Added geometry validation for GEO types to exit early on invalid latitudes
3+
area: Geo
4+
type: bug
5+
issues:
6+
- 128234

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/SpatialCoordinateTypes.java‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import org.apache.lucene.util.BytesRef;
1313
import org.elasticsearch.geometry.Geometry;
1414
import org.elasticsearch.geometry.Point;
15+
import org.elasticsearch.geometry.utils.GeographyValidator;
1516
import org.elasticsearch.geometry.utils.GeometryValidator;
1617
import org.elasticsearch.geometry.utils.WellKnownBinary;
1718
import org.elasticsearch.geometry.utils.WellKnownText;
@@ -32,6 +33,11 @@ public long pointAsLong(double x, double y) {
3233
int longitudeEncoded = encodeLongitude(x);
3334
return (((long) latitudeEncoded) << 32) | (longitudeEncoded & 0xFFFFFFFFL);
3435
}
36+
37+
public GeometryValidator validator() {
38+
// We validate the lat/lon values, and ignore any z values
39+
return GeographyValidator.instance(true);
40+
}
3541
},
3642
CARTESIAN {
3743

@@ -67,6 +73,10 @@ public long pointAsLong(double x, double y) {
6773
}
6874
};
6975

76+
protected GeometryValidator validator() {
77+
return GeometryValidator.NOOP;
78+
}
79+
7080
public abstract Point longAsPoint(long encoded);
7181

7282
public abstract long pointAsLong(double x, double y);
@@ -77,7 +87,7 @@ public long wkbAsLong(BytesRef wkb) {
7787
}
7888

7989
public Point wkbAsPoint(BytesRef wkb) {
80-
Geometry geometry = WellKnownBinary.fromWKB(GeometryValidator.NOOP, false, wkb.bytes, wkb.offset, wkb.length);
90+
Geometry geometry = WellKnownBinary.fromWKB(validator(), false, wkb.bytes, wkb.offset, wkb.length);
8191
if (geometry instanceof Point point) {
8292
return point;
8393
} else {
@@ -101,26 +111,18 @@ public BytesRef wktToWkb(String wkt) {
101111
// TODO: we should be able to transform WKT to WKB without building the geometry
102112
// we should as well use different validator for cartesian and geo?
103113
try {
104-
Geometry geometry = WellKnownText.fromWKT(GeometryValidator.NOOP, false, wkt);
114+
Geometry geometry = WellKnownText.fromWKT(validator(), false, wkt);
105115
return new BytesRef(WellKnownBinary.toWKB(geometry, ByteOrder.LITTLE_ENDIAN));
106116
} catch (Exception e) {
107117
throw new IllegalArgumentException("Failed to parse WKT: " + e.getMessage(), e);
108118
}
109119
}
110120

111-
public Geometry wktToGeometry(String wkt) {
112-
try {
113-
return WellKnownText.fromWKT(GeometryValidator.NOOP, false, wkt);
114-
} catch (Exception e) {
115-
throw new IllegalArgumentException("Failed to parse WKT: " + e.getMessage(), e);
116-
}
117-
}
118-
119121
public String wkbToWkt(BytesRef wkb) {
120122
return WellKnownText.fromWKB(wkb.bytes, wkb.offset, wkb.length);
121123
}
122124

123125
public Geometry wkbToGeometry(BytesRef wkb) {
124-
return WellKnownBinary.fromWKB(GeometryValidator.NOOP, false, wkb.bytes, wkb.offset, wkb.length);
126+
return WellKnownBinary.fromWKB(validator(), false, wkb.bytes, wkb.offset, wkb.length);
125127
}
126128
}

‎x-pack/plugin/esql/qa/testFixtures/src/main/resources/spatial.csv-spec‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1788,6 +1788,78 @@ count:long
17881788
12
17891789
;
17901790

1791+
airportsDistanceLessThanInvalidPoint
1792+
required_capability: st_distance
1793+
required_capability: geo_validation
1794+
1795+
FROM airports
1796+
| EVAL distance = ST_DISTANCE(location, TO_GEOPOINT("POINT(-28.6359 153.5906)"))
1797+
| KEEP distance, location, name, scalerank
1798+
| WHERE distance < 500000 AND scalerank < 6
1799+
| STATS count = COUNT()
1800+
;
1801+
1802+
warning:Line 2:41: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
1803+
warning:Line 2:41: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1804+
1805+
count:l
1806+
0
1807+
;
1808+
1809+
airportsDistanceLessThanInvalidPoint2
1810+
required_capability: st_distance
1811+
required_capability: geo_validation
1812+
1813+
FROM airports
1814+
| EVAL distance = ST_DISTANCE(TO_GEOPOINT("POINT(-28.6359 153.5906)"), location)
1815+
| KEEP distance, location, name, scalerank
1816+
| WHERE distance < 500000 AND scalerank < 6
1817+
| STATS count = COUNT()
1818+
;
1819+
1820+
warning:Line 2:31: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
1821+
warning:Line 2:31: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1822+
1823+
count:l
1824+
0
1825+
;
1826+
1827+
airportsDistanceGreaterThanInvalidPoint
1828+
required_capability: st_distance
1829+
required_capability: geo_validation
1830+
1831+
FROM airports
1832+
| EVAL distance = ST_DISTANCE(location, TO_GEOPOINT("POINT(-28.6359 153.5906)"))
1833+
| KEEP distance, location, name, scalerank
1834+
| WHERE distance > 500 AND scalerank < 6
1835+
| STATS count = COUNT()
1836+
;
1837+
1838+
warning:Line 2:41: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
1839+
warning:Line 2:41: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1840+
1841+
count:l
1842+
0
1843+
;
1844+
1845+
airportsDistanceGreaterThanInvalidPoint2
1846+
required_capability: st_distance
1847+
required_capability: geo_validation
1848+
1849+
FROM airports
1850+
| EVAL distance = ST_DISTANCE(TO_GEOPOINT("POINT(-28.6359 153.5906)"), location)
1851+
| KEEP distance, location, name, scalerank
1852+
| WHERE distance > 500 AND scalerank < 6
1853+
| STATS count = COUNT()
1854+
;
1855+
1856+
warning:Line 2:31: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
1857+
warning:Line 2:31: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1858+
1859+
count:l
1860+
0
1861+
;
1862+
17911863
###############################################
17921864
# Tests for Equality and casting with GEO_POINT
17931865

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ public enum Cap {
5555
*/
5656
SPATIAL_SHAPES,
5757

58+
/**
59+
* Do validation check on geo_point and geo_shape fields. Done in #128259.
60+
*/
61+
GEO_VALIDATION,
62+
5863
/**
5964
* Support for spatial aggregation {@code ST_CENTROID}. Done in #104269.
6065
*/

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToGeoPoint.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_POINT;
2727
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
2828
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
29-
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToSpatial;
29+
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToGeo;
3030

3131
public class ToGeoPoint extends AbstractConvertFunction {
3232
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
@@ -91,6 +91,6 @@ protected NodeInfo<? extends Expression> info() {
9191

9292
@ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
9393
static BytesRef fromKeyword(BytesRef in) {
94-
return stringToSpatial(in.utf8ToString());
94+
return stringToGeo(in.utf8ToString());
9595
}
9696
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToGeoShape.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_SHAPE;
2828
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
2929
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
30-
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToSpatial;
30+
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToGeo;
3131

3232
public class ToGeoShape extends AbstractConvertFunction {
3333
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
@@ -93,6 +93,6 @@ protected NodeInfo<? extends Expression> info() {
9393

9494
@ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
9595
static BytesRef fromKeyword(BytesRef in) {
96-
return stringToSpatial(in.utf8ToString());
96+
return stringToGeo(in.utf8ToString());
9797
}
9898
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypeConverter.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@
104104
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asLongUnsigned;
105105
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asUnsignedLong;
106106
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.unsignedLongAsNumber;
107+
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.GEO;
107108
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.UNSPECIFIED;
108109

109110
public class EsqlDataTypeConverter {
@@ -234,6 +235,9 @@ public static Converter converterFor(DataType from, DataType to) {
234235
if (to == DataType.BOOLEAN) {
235236
return EsqlConverter.STRING_TO_BOOLEAN;
236237
}
238+
if (DataType.isSpatialGeo(to)) {
239+
return EsqlConverter.STRING_TO_GEO;
240+
}
237241
if (DataType.isSpatial(to)) {
238242
return EsqlConverter.STRING_TO_SPATIAL;
239243
}
@@ -540,6 +544,10 @@ public static String spatialToString(BytesRef field) {
540544
return UNSPECIFIED.wkbToWkt(field);
541545
}
542546

547+
public static BytesRef stringToGeo(String field) {
548+
return GEO.wktToWkb(field);
549+
}
550+
543551
public static BytesRef stringToSpatial(String field) {
544552
return UNSPECIFIED.wktToWkb(field);
545553
}
@@ -772,6 +780,7 @@ public enum EsqlConverter implements Converter {
772780
STRING_TO_LONG(x -> EsqlDataTypeConverter.stringToLong((String) x)),
773781
STRING_TO_INT(x -> EsqlDataTypeConverter.stringToInt((String) x)),
774782
STRING_TO_BOOLEAN(x -> EsqlDataTypeConverter.stringToBoolean((String) x)),
783+
STRING_TO_GEO(x -> EsqlDataTypeConverter.stringToGeo((String) x)),
775784
STRING_TO_SPATIAL(x -> EsqlDataTypeConverter.stringToSpatial((String) x));
776785

777786
private static final String NAME = "esql-converter";

0 commit comments

Comments
 (0)