Skip to content

Commit 617129a

Browse files
craigtavernercraigtaverner
authored
Added geometry validation for GEO types to exit early on invalid latitudes (elastic#128259) (elastic#128426) (elastic#128427)
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 c2a2a08 commit 617129a

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
@@ -1725,6 +1725,78 @@ count:long
17251725
12
17261726
;
17271727

1728+
airportsDistanceLessThanInvalidPoint
1729+
required_capability: st_distance
1730+
required_capability: geo_validation
1731+
1732+
FROM airports
1733+
| EVAL distance = ST_DISTANCE(location, TO_GEOPOINT("POINT(-28.6359 153.5906)"))
1734+
| KEEP distance, location, name, scalerank
1735+
| WHERE distance < 500000 AND scalerank < 6
1736+
| STATS count = COUNT()
1737+
;
1738+
1739+
warning:Line 2:41: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
1740+
warning:Line 2:41: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1741+
1742+
count:l
1743+
0
1744+
;
1745+
1746+
airportsDistanceLessThanInvalidPoint2
1747+
required_capability: st_distance
1748+
required_capability: geo_validation
1749+
1750+
FROM airports
1751+
| EVAL distance = ST_DISTANCE(TO_GEOPOINT("POINT(-28.6359 153.5906)"), location)
1752+
| KEEP distance, location, name, scalerank
1753+
| WHERE distance < 500000 AND scalerank < 6
1754+
| STATS count = COUNT()
1755+
;
1756+
1757+
warning:Line 2:31: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
1758+
warning:Line 2:31: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1759+
1760+
count:l
1761+
0
1762+
;
1763+
1764+
airportsDistanceGreaterThanInvalidPoint
1765+
required_capability: st_distance
1766+
required_capability: geo_validation
1767+
1768+
FROM airports
1769+
| EVAL distance = ST_DISTANCE(location, TO_GEOPOINT("POINT(-28.6359 153.5906)"))
1770+
| KEEP distance, location, name, scalerank
1771+
| WHERE distance > 500 AND scalerank < 6
1772+
| STATS count = COUNT()
1773+
;
1774+
1775+
warning:Line 2:41: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
1776+
warning:Line 2:41: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1777+
1778+
count:l
1779+
0
1780+
;
1781+
1782+
airportsDistanceGreaterThanInvalidPoint2
1783+
required_capability: st_distance
1784+
required_capability: geo_validation
1785+
1786+
FROM airports
1787+
| EVAL distance = ST_DISTANCE(TO_GEOPOINT("POINT(-28.6359 153.5906)"), location)
1788+
| KEEP distance, location, name, scalerank
1789+
| WHERE distance > 500 AND scalerank < 6
1790+
| STATS count = COUNT()
1791+
;
1792+
1793+
warning:Line 2:31: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
1794+
warning:Line 2:31: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1795+
1796+
count:l
1797+
0
1798+
;
1799+
17281800
###############################################
17291801
# Tests for Equality and casting with GEO_POINT
17301802

‎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
@@ -220,6 +220,11 @@ public enum Cap {
220220
*/
221221
SPATIAL_CENTROID_NO_RECORDS,
222222

223+
/**
224+
* Do validation check on geo_point and geo_shape fields. Done in #128259.
225+
*/
226+
GEO_VALIDATION,
227+
223228
/**
224229
* Support ST_ENVELOPE function (and related ST_XMIN, etc.).
225230
*/

‎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
@@ -96,6 +96,7 @@
9696
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asLongUnsigned;
9797
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asUnsignedLong;
9898
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.unsignedLongAsNumber;
99+
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.GEO;
99100
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.UNSPECIFIED;
100101

101102
public class EsqlDataTypeConverter {
@@ -225,6 +226,9 @@ public static Converter converterFor(DataType from, DataType to) {
225226
if (to == DataType.BOOLEAN) {
226227
return EsqlConverter.STRING_TO_BOOLEAN;
227228
}
229+
if (DataType.isSpatialGeo(to)) {
230+
return EsqlConverter.STRING_TO_GEO;
231+
}
228232
if (DataType.isSpatial(to)) {
229233
return EsqlConverter.STRING_TO_SPATIAL;
230234
}
@@ -536,6 +540,10 @@ public static String spatialToString(BytesRef field) {
536540
return UNSPECIFIED.wkbToWkt(field);
537541
}
538542

543+
public static BytesRef stringToGeo(String field) {
544+
return GEO.wktToWkb(field);
545+
}
546+
539547
public static BytesRef stringToSpatial(String field) {
540548
return UNSPECIFIED.wktToWkb(field);
541549
}
@@ -694,6 +702,7 @@ public enum EsqlConverter implements Converter {
694702
STRING_TO_LONG(x -> EsqlDataTypeConverter.stringToLong((String) x)),
695703
STRING_TO_INT(x -> EsqlDataTypeConverter.stringToInt((String) x)),
696704
STRING_TO_BOOLEAN(x -> EsqlDataTypeConverter.stringToBoolean((String) x)),
705+
STRING_TO_GEO(x -> EsqlDataTypeConverter.stringToGeo((String) x)),
697706
STRING_TO_SPATIAL(x -> EsqlDataTypeConverter.stringToSpatial((String) x));
698707

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

0 commit comments

Comments
 (0)