Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/128259.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 128259
summary: Added geometry validation for GEO types to exit early on invalid latitudes
area: Geo
type: bug
issues:
- 128234
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.geometry.Geometry;
import org.elasticsearch.geometry.Point;
import org.elasticsearch.geometry.utils.GeographyValidator;
import org.elasticsearch.geometry.utils.GeometryValidator;
import org.elasticsearch.geometry.utils.WellKnownBinary;
import org.elasticsearch.geometry.utils.WellKnownText;
Expand All @@ -32,6 +33,11 @@ public long pointAsLong(double x, double y) {
int longitudeEncoded = encodeLongitude(x);
return (((long) latitudeEncoded) << 32) | (longitudeEncoded & 0xFFFFFFFFL);
}

public GeometryValidator validator() {
// We validate the lat/lon values, and ignore any z values
return GeographyValidator.instance(true);
}
},
CARTESIAN {

Expand Down Expand Up @@ -67,6 +73,10 @@ public long pointAsLong(double x, double y) {
}
};

protected GeometryValidator validator() {
return GeometryValidator.NOOP;
}

public abstract Point longAsPoint(long encoded);

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

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

public Geometry wktToGeometry(String wkt) {
try {
return WellKnownText.fromWKT(GeometryValidator.NOOP, false, wkt);
} catch (Exception e) {
throw new IllegalArgumentException("Failed to parse WKT: " + e.getMessage(), e);
}
}

public String wkbToWkt(BytesRef wkb) {
return WellKnownText.fromWKB(wkb.bytes, wkb.offset, wkb.length);
}

public Geometry wkbToGeometry(BytesRef wkb) {
return WellKnownBinary.fromWKB(GeometryValidator.NOOP, false, wkb.bytes, wkb.offset, wkb.length);
return WellKnownBinary.fromWKB(validator(), false, wkb.bytes, wkb.offset, wkb.length);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1788,6 +1788,78 @@ count:long
12
;

airportsDistanceLessThanInvalidPoint
required_capability: st_distance
required_capability: geo_validation

FROM airports
| EVAL distance = ST_DISTANCE(location, TO_GEOPOINT("POINT(-28.6359 153.5906)"))
| KEEP distance, location, name, scalerank
| WHERE distance < 500000 AND scalerank < 6
| STATS count = COUNT()
;

warning:Line 2:41: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
warning:Line 2:41: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0

count:l
0
;

airportsDistanceLessThanInvalidPoint2
required_capability: st_distance
required_capability: geo_validation

FROM airports
| EVAL distance = ST_DISTANCE(TO_GEOPOINT("POINT(-28.6359 153.5906)"), location)
| KEEP distance, location, name, scalerank
| WHERE distance < 500000 AND scalerank < 6
| STATS count = COUNT()
;

warning:Line 2:31: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
warning:Line 2:31: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0

count:l
0
;

airportsDistanceGreaterThanInvalidPoint
required_capability: st_distance
required_capability: geo_validation

FROM airports
| EVAL distance = ST_DISTANCE(location, TO_GEOPOINT("POINT(-28.6359 153.5906)"))
| KEEP distance, location, name, scalerank
| WHERE distance > 500 AND scalerank < 6
| STATS count = COUNT()
;

warning:Line 2:41: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
warning:Line 2:41: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0

count:l
0
;

airportsDistanceGreaterThanInvalidPoint2
required_capability: st_distance
required_capability: geo_validation

FROM airports
| EVAL distance = ST_DISTANCE(TO_GEOPOINT("POINT(-28.6359 153.5906)"), location)
| KEEP distance, location, name, scalerank
| WHERE distance > 500 AND scalerank < 6
| STATS count = COUNT()
;

warning:Line 2:31: evaluation of [TO_GEOPOINT(\"POINT(-28.6359 153.5906)\")] failed, treating result as null. Only first 20 failures recorded.
warning:Line 2:31: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0

count:l
0
;

###############################################
# Tests for Equality and casting with GEO_POINT

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ public enum Cap {
*/
SPATIAL_SHAPES,

/**
* Do validation check on geo_point and geo_shape fields. Done in #128259.
*/
GEO_VALIDATION,

/**
* Support for spatial aggregation {@code ST_CENTROID}. Done in #104269.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
import static org.elasticsearch.xpack.esql.core.type.DataType.SEMANTIC_TEXT;
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToSpatial;
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToGeo;

public class ToGeoPoint extends AbstractConvertFunction {
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
Expand Down Expand Up @@ -93,6 +93,6 @@ protected NodeInfo<? extends Expression> info() {

@ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
static BytesRef fromKeyword(BytesRef in) {
return stringToSpatial(in.utf8ToString());
return stringToGeo(in.utf8ToString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
import static org.elasticsearch.xpack.esql.core.type.DataType.SEMANTIC_TEXT;
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToSpatial;
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.stringToGeo;

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

@ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
static BytesRef fromKeyword(BytesRef in) {
return stringToSpatial(in.utf8ToString());
return stringToGeo(in.utf8ToString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asLongUnsigned;
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asUnsignedLong;
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.unsignedLongAsNumber;
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.GEO;
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.UNSPECIFIED;

public class EsqlDataTypeConverter {
Expand Down Expand Up @@ -223,6 +224,9 @@ public static Converter converterFor(DataType from, DataType to) {
if (to == DataType.BOOLEAN) {
return EsqlConverter.STRING_TO_BOOLEAN;
}
if (DataType.isSpatialGeo(to)) {
return EsqlConverter.STRING_TO_GEO;
}
if (DataType.isSpatial(to)) {
return EsqlConverter.STRING_TO_SPATIAL;
}
Expand Down Expand Up @@ -529,6 +533,10 @@ public static String spatialToString(BytesRef field) {
return UNSPECIFIED.wkbToWkt(field);
}

public static BytesRef stringToGeo(String field) {
return GEO.wktToWkb(field);
}

public static BytesRef stringToSpatial(String field) {
return UNSPECIFIED.wktToWkb(field);
}
Expand Down Expand Up @@ -687,6 +695,7 @@ public enum EsqlConverter implements Converter {
STRING_TO_LONG(x -> EsqlDataTypeConverter.stringToLong((String) x)),
STRING_TO_INT(x -> EsqlDataTypeConverter.stringToInt((String) x)),
STRING_TO_BOOLEAN(x -> EsqlDataTypeConverter.stringToBoolean((String) x)),
STRING_TO_GEO(x -> EsqlDataTypeConverter.stringToGeo((String) x)),
STRING_TO_SPATIAL(x -> EsqlDataTypeConverter.stringToSpatial((String) x));

private static final String NAME = "esql-converter";
Expand Down