Skip to content

Commit c489173

Browse files
craigtavernercraigtaverner
authored
Added geometry validation for GEO types to exit early on invalid latitudes (#128259) (#128425)
It turns out in #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 #128234 happening in future.
1 parent 7876cd3 commit c489173

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
@@ -56,6 +56,11 @@ public enum Cap {
5656
*/
5757
SPATIAL_SHAPES,
5858

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

‎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
@@ -27,7 +27,7 @@
2727
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
2828
import static org.elasticsearch.xpack.esql.core.type.DataType.SEMANTIC_TEXT;
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 ToGeoPoint 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/expression/function/scalar/convert/ToGeoShape.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
2929
import static org.elasticsearch.xpack.esql.core.type.DataType.SEMANTIC_TEXT;
3030
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
31-
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToSpatial;
31+
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToGeo;
3232

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

9696
@ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
9797
static BytesRef fromKeyword(BytesRef in) {
98-
return stringToSpatial(in.utf8ToString());
98+
return stringToGeo(in.utf8ToString());
9999
}
100100
}

‎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
@@ -94,6 +94,7 @@
9494
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asLongUnsigned;
9595
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asUnsignedLong;
9696
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.unsignedLongAsNumber;
97+
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.GEO;
9798
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.UNSPECIFIED;
9899

99100
public class EsqlDataTypeConverter {
@@ -223,6 +224,9 @@ public static Converter converterFor(DataType from, DataType to) {
223224
if (to == DataType.BOOLEAN) {
224225
return EsqlConverter.STRING_TO_BOOLEAN;
225226
}
227+
if (DataType.isSpatialGeo(to)) {
228+
return EsqlConverter.STRING_TO_GEO;
229+
}
226230
if (DataType.isSpatial(to)) {
227231
return EsqlConverter.STRING_TO_SPATIAL;
228232
}
@@ -529,6 +533,10 @@ public static String spatialToString(BytesRef field) {
529533
return UNSPECIFIED.wkbToWkt(field);
530534
}
531535

536+
public static BytesRef stringToGeo(String field) {
537+
return GEO.wktToWkb(field);
538+
}
539+
532540
public static BytesRef stringToSpatial(String field) {
533541
return UNSPECIFIED.wktToWkb(field);
534542
}
@@ -687,6 +695,7 @@ public enum EsqlConverter implements Converter {
687695
STRING_TO_LONG(x -> EsqlDataTypeConverter.stringToLong((String) x)),
688696
STRING_TO_INT(x -> EsqlDataTypeConverter.stringToInt((String) x)),
689697
STRING_TO_BOOLEAN(x -> EsqlDataTypeConverter.stringToBoolean((String) x)),
698+
STRING_TO_GEO(x -> EsqlDataTypeConverter.stringToGeo((String) x)),
690699
STRING_TO_SPATIAL(x -> EsqlDataTypeConverter.stringToSpatial((String) x));
691700

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

0 commit comments

Comments
 (0)