Skip to content

Commit dfc5c38

Browse files
craigtavernercraigtaverner
authored
Fix ST_DISTANCE handling of invalid geometry literals that fold to null (#140116) (#140265) (#140276)
Occasionally on serverless we see a NullPointerException in parsing a geometry literal within ST_DISTANCE functions. This turned out to be hard to reproduce since we cannot see the original query causing this problem. It has been reported in #138594, including a full stack track confirming that it occurs during planning, while folding literals. It turned out to be reproducible with a query like: ``` ROW point = MV_SLICE(["POINT(0)"], 0, 0)::geo_point | EVAL distance = ST_DISTANCE(point, point) ``` Here the WKT is invalid, and the fold over the MV_SLICE would return literal `null` values, since the inner TO_GEOPOINT function was throwing exceptions (since the WKT was invalid). It turned out that MvSlice was using EvaluatorMapper.fold for folding the children, which returns literal java `null` values, and any function using these values needs to also handle literal nulls. So, `fold()` is expected to explicitly return null values when input is null, and the implementation of fold in StDistance did not do this. This PR now covers all descendents of BinarySpatialFunction: * ST_DISTANCE * ST_INTERSECTS * ST_DISJOINT * ST_CONTAINS * ST_WITHIN
1 parent 355b1f4 commit dfc5c38

File tree

11 files changed

+260
-78
lines changed

11 files changed

+260
-78
lines changed

‎docs/changelog/140116.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 140116
2+
summary: Fix ST_DISTANCE handling of invalid geometry literals that fold to null
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 138594

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

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1921,6 +1921,167 @@ wkt:keyword |pt:geo_point
19211921
// end::to_geopoint-str-parse-error-result[]
19221922
;
19231923

1924+
###############################################
1925+
# Tests for bug with literals that evaluate to null
1926+
# (due to inner geometry parsing errors)
1927+
# in ST_DISTANCE, ST_INTERSECTS, ST_DISJOINT, ST_CONTAINS, ST_WITHIN
1928+
# Reported in https://github.com/elastic/elasticsearch/issues/138594
1929+
#
1930+
1931+
airportsDistanceGreaterThanInvalidPoint3
1932+
required_capability: geo_null_literals_folding
1933+
1934+
FROM airports
1935+
| EVAL distance = ST_DISTANCE(TO_GEOPOINT(MV_SLICE(["POINT(-28.6359 153.5906)"], 0, 0)), location)
1936+
| KEEP distance, location, name, scalerank
1937+
| WHERE distance > 500 AND scalerank < 6
1938+
| STATS count = COUNT()
1939+
;
1940+
1941+
warning:Line 2:31: evaluation of [TO_GEOPOINT(MV_SLICE([\"POINT(-28.6359 153.5906)\"], 0, 0))] failed, treating result as null. Only first 20 failures recorded.
1942+
warning:Line 2:31: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1943+
1944+
count:l
1945+
0
1946+
;
1947+
1948+
distancePointsLiteralMvSlice
1949+
required_capability: st_distance
1950+
1951+
ROW point = MV_SLICE(["POINT(0 0)"], 0, 0)::geo_point
1952+
| EVAL distance = ST_DISTANCE(point, point)
1953+
;
1954+
1955+
point:geo_point | distance:double
1956+
POINT(0 0) | 0.0
1957+
;
1958+
1959+
distancePointsLiteralMvSliceInvalid
1960+
required_capability: geo_null_literals_folding
1961+
1962+
ROW point = MV_SLICE(["POINT(0)"], 0, 0)::geo_point
1963+
| EVAL d1 = ST_DISTANCE(point, point)
1964+
| EVAL d2 = ST_DISTANCE(point, "POINT(0 0)"::geo_point)
1965+
| EVAL d3 = ST_DISTANCE("POINT(0 0)"::geo_point, point)
1966+
| EVAL d4 = ST_DISTANCE("POINT(0 0)"::geo_point, "POINT(0 0)"::geo_point)
1967+
;
1968+
1969+
warning:Line 1:13: evaluation of [MV_SLICE([\"POINT(0)\"], 0, 0)::geo_point] failed, treating result as null. Only first 20 failures recorded.
1970+
warning:Line 1:13: java.lang.IllegalArgumentException: Failed to parse WKT: expected number but found: ')'
1971+
1972+
point:geo_point | d1:double | d2:double | d3:double | d4:double
1973+
null | null | null | null | 0.0
1974+
;
1975+
1976+
airportsIntersectsInvalidShapeLiteralMvSlice
1977+
required_capability: geo_null_literals_folding
1978+
1979+
FROM airports
1980+
| WHERE ST_INTERSECTS(TO_GEOSHAPE(MV_SLICE(["POINT(-28.6359 153.5906)"], 0, 0)), location)
1981+
| STATS count = COUNT()
1982+
;
1983+
1984+
warning:Line 2:23: evaluation of [TO_GEOSHAPE(MV_SLICE([\"POINT(-28.6359 153.5906)\"], 0, 0))] failed, treating result as null. Only first 20 failures recorded.
1985+
warning:Line 2:23: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
1986+
1987+
count:l
1988+
0
1989+
;
1990+
1991+
airportsEvalIntersectsInvalidShapeLiteralMvSlice
1992+
required_capability: geo_null_literals_folding
1993+
1994+
FROM airports
1995+
| EVAL intersects = ST_INTERSECTS(TO_GEOSHAPE(MV_SLICE(["POINT(-28.6359 153.5906)"], 0, 0)), location)
1996+
| STATS count = COUNT() BY intersects
1997+
;
1998+
1999+
warning:Line 2:35: evaluation of [TO_GEOSHAPE(MV_SLICE([\"POINT(-28.6359 153.5906)\"], 0, 0))] failed, treating result as null. Only first 20 failures recorded.
2000+
warning:Line 2:35: java.lang.IllegalArgumentException: Failed to parse WKT: invalid latitude 153.5906; must be between -90.0 and 90.0
2001+
2002+
count:l | intersects:boolean
2003+
891 | null
2004+
;
2005+
2006+
intersectsPointsLiteralMvSlice
2007+
required_capability: st_intersects
2008+
2009+
ROW point = MV_SLICE(["POINT(0 0)"], 0, 0)::geo_point
2010+
| EVAL intersects = ST_INTERSECTS(point, point)
2011+
;
2012+
2013+
point:geo_point | intersects:boolean
2014+
POINT(0 0) | true
2015+
;
2016+
2017+
intersectsPointsLiteralMvSliceInvalid
2018+
required_capability: geo_null_literals_folding
2019+
2020+
ROW point = MV_SLICE(["POINT(0)"], 0, 0)::geo_point
2021+
| EVAL a = ST_INTERSECTS(point, point)
2022+
| EVAL b = ST_INTERSECTS(point, "POINT(0 0)"::geo_point)
2023+
| EVAL c = ST_INTERSECTS("POINT(0 0)"::geo_point, point)
2024+
| EVAL d = ST_INTERSECTS("POINT(0 0)"::geo_point, "POINT(0 0)"::geo_point)
2025+
;
2026+
2027+
warning:Line 1:13: evaluation of [MV_SLICE([\"POINT(0)\"], 0, 0)::geo_point] failed, treating result as null. Only first 20 failures recorded.
2028+
warning:Line 1:13: java.lang.IllegalArgumentException: Failed to parse WKT: expected number but found: ')'
2029+
2030+
point:geo_point | a:boolean | b:boolean | c:boolean | d:boolean
2031+
null | null | null | null | true
2032+
;
2033+
2034+
disjointPointsLiteralMvSliceInvalid
2035+
required_capability: geo_null_literals_folding
2036+
2037+
ROW point = MV_SLICE(["POINT(0)"], 0, 0)::geo_point
2038+
| EVAL a = ST_DISJOINT(point, point)
2039+
| EVAL b = ST_DISJOINT(point, "POINT(0 0)"::geo_point)
2040+
| EVAL c = ST_DISJOINT("POINT(0 0)"::geo_point, point)
2041+
| EVAL d = ST_DISJOINT("POINT(0 0)"::geo_point, "POINT(0 0)"::geo_point)
2042+
;
2043+
2044+
warning:Line 1:13: evaluation of [MV_SLICE([\"POINT(0)\"], 0, 0)::geo_point] failed, treating result as null. Only first 20 failures recorded.
2045+
warning:Line 1:13: java.lang.IllegalArgumentException: Failed to parse WKT: expected number but found: ')'
2046+
2047+
point:geo_point | a:boolean | b:boolean | c:boolean | d:boolean
2048+
null | null | null | null | false
2049+
;
2050+
2051+
containsPointsLiteralMvSliceInvalid
2052+
required_capability: geo_null_literals_folding
2053+
2054+
ROW point = MV_SLICE(["POINT(0)"], 0, 0)::geo_point
2055+
| EVAL a = ST_CONTAINS(point, point)
2056+
| EVAL b = ST_CONTAINS(point, "POINT(0 0)"::geo_point)
2057+
| EVAL c = ST_CONTAINS("POINT(0 0)"::geo_point, point)
2058+
| EVAL d = ST_CONTAINS("POINT(0 0)"::geo_point, "POINT(0 0)"::geo_point)
2059+
;
2060+
2061+
warning:Line 1:13: evaluation of [MV_SLICE([\"POINT(0)\"], 0, 0)::geo_point] failed, treating result as null. Only first 20 failures recorded.
2062+
warning:Line 1:13: java.lang.IllegalArgumentException: Failed to parse WKT: expected number but found: ')'
2063+
2064+
point:geo_point | a:boolean | b:boolean | c:boolean | d:boolean
2065+
null | null | null | null | true
2066+
;
2067+
2068+
withinPointsLiteralMvSliceInvalid
2069+
required_capability: geo_null_literals_folding
2070+
2071+
ROW point = MV_SLICE(["POINT(0)"], 0, 0)::geo_point
2072+
| EVAL a = ST_WITHIN(point, point)
2073+
| EVAL b = ST_WITHIN(point, "POINT(0 0)"::geo_point)
2074+
| EVAL c = ST_WITHIN("POINT(0 0)"::geo_point, point)
2075+
| EVAL d = ST_WITHIN("POINT(0 0)"::geo_point, "POINT(0 0)"::geo_point)
2076+
;
2077+
2078+
warning:Line 1:13: evaluation of [MV_SLICE([\"POINT(0)\"], 0, 0)::geo_point] failed, treating result as null. Only first 20 failures recorded.
2079+
warning:Line 1:13: java.lang.IllegalArgumentException: Failed to parse WKT: expected number but found: ')'
2080+
2081+
point:geo_point | a:boolean | b:boolean | c:boolean | d:boolean
2082+
null | null | null | null | true
2083+
;
2084+
19242085
###############################################
19252086
# Tests for CARTESIAN_POINT type
19262087
###############################################

‎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
@@ -58,6 +58,11 @@ public enum Cap {
5858
*/
5959
GEO_VALIDATION,
6060

61+
/**
62+
* Fold in spatial functions should return null for null input.
63+
*/
64+
GEO_NULL_LITERALS_FOLDING,
65+
6166
/**
6267
* Support for spatial aggregation {@code ST_CENTROID}. Done in #104269.
6368
*/

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

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.elasticsearch.xpack.esql.capabilities.TranslationAware;
1717
import org.elasticsearch.xpack.esql.core.expression.Expression;
1818
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
19+
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
1920
import org.elasticsearch.xpack.esql.core.expression.TypeResolutions;
2021
import org.elasticsearch.xpack.esql.core.expression.function.scalar.BinaryScalarFunction;
2122
import org.elasticsearch.xpack.esql.core.tree.Source;
@@ -37,6 +38,7 @@
3738
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_POINT;
3839
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_SHAPE;
3940
import static org.elasticsearch.xpack.esql.core.type.DataType.isNull;
41+
import static org.elasticsearch.xpack.esql.expression.function.scalar.spatial.SpatialRelatesUtils.makeGeometryFromLiteral;
4042

4143
/**
4244
* Spatial functions that take two arguments that must both be spatial types can inherit from this class.
@@ -49,7 +51,7 @@ public abstract class BinarySpatialFunction extends BinaryScalarFunction impleme
4951
"esql_serialize_source_functions_warnings"
5052
);
5153

52-
private final SpatialTypeResolver spatialTypeResolver;
54+
protected final SpatialTypeResolver spatialTypeResolver;
5355
private SpatialCrsType crsType;
5456
protected final boolean leftDocValues;
5557
protected final boolean rightDocValues;
@@ -96,6 +98,18 @@ public void writeTo(StreamOutput out) throws IOException {
9698
// The CRS type is re-resolved from the combination of left and right fields, and also not necessary to serialize
9799
}
98100

101+
protected abstract Object fold(Geometry leftGeom, Geometry rightGeom);
102+
103+
@Override
104+
public Object fold(FoldContext ctx) {
105+
var leftGeom = makeGeometryFromLiteral(ctx, left());
106+
var rightGeom = makeGeometryFromLiteral(ctx, right());
107+
if (leftGeom == null || rightGeom == null) {
108+
return null;
109+
}
110+
return fold(leftGeom, rightGeom);
111+
}
112+
99113
/**
100114
* Mark the function as expecting the specified fields to arrive as doc-values.
101115
*/
@@ -124,10 +138,10 @@ protected TypeResolution resolveType() {
124138
return spatialTypeResolver.resolveType();
125139
}
126140

127-
static class SpatialTypeResolver {
141+
protected static class SpatialTypeResolver {
128142
private final SpatialEvaluatorFactory.SpatialSourceResolution supplier;
129143
private final boolean pointsOnly;
130-
private final boolean supportsGrid;
144+
protected final boolean supportsGrid;
131145

132146
SpatialTypeResolver(SpatialEvaluatorFactory.SpatialSourceResolution supplier, boolean pointsOnly, boolean supportsGrid) {
133147
this.supplier = supplier;

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

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public class SpatialContains extends SpatialRelatesFunction {
7878
* We override the normal behaviour for CONTAINS because we need to test each component separately.
7979
* This applies to multi-component geometries (MultiPolygon, etc.) as well as polygons that cross the dateline.
8080
*/
81-
static final class SpatialRelationsContains extends SpatialRelations {
81+
protected static final class SpatialRelationsContains extends SpatialRelations {
8282

8383
SpatialRelationsContains(SpatialCoordinateTypes spatialCoordinateType, CoordinateEncoder encoder, ShapeIndexer shapeIndexer) {
8484
super(ShapeField.QueryRelation.CONTAINS, spatialCoordinateType, encoder, shapeIndexer);
@@ -215,15 +215,25 @@ protected NodeInfo<? extends Expression> info() {
215215
return NodeInfo.create(this, SpatialContains::new, left(), right());
216216
}
217217

218+
@Override
219+
protected SpatialRelationsContains getSpatialRelations() {
220+
return crsType() == SpatialCrsType.GEO ? GEO : CARTESIAN;
221+
}
222+
223+
/**
224+
* Contains needs to evaluate each component of the right geometry separately,
225+
* so we override the fold method from the parent SpatialRelatesFunction.
226+
*/
218227
@Override
219228
public Object fold(FoldContext ctx) {
220229
try {
221230
GeometryDocValueReader docValueReader = asGeometryDocValueReader(ctx, crsType(), left());
222231
Geometry rightGeom = makeGeometryFromLiteral(ctx, right());
223232
Component2D[] components = asLuceneComponent2Ds(crsType(), rightGeom);
224-
return (crsType() == SpatialCrsType.GEO)
225-
? GEO.geometryRelatesGeometries(docValueReader, components)
226-
: CARTESIAN.geometryRelatesGeometries(docValueReader, components);
233+
if (docValueReader == null || components == null) {
234+
return null;
235+
}
236+
return getSpatialRelations().geometryRelatesGeometries(docValueReader, components);
227237
} catch (IOException e) {
228238
throw new IllegalArgumentException("Failed to fold constant fields: " + e.getMessage(), e);
229239
}

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

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,7 @@
2222
import org.elasticsearch.index.mapper.GeoShapeIndexer;
2323
import org.elasticsearch.lucene.spatial.CartesianShapeIndexer;
2424
import org.elasticsearch.lucene.spatial.CoordinateEncoder;
25-
import org.elasticsearch.lucene.spatial.GeometryDocValueReader;
2625
import org.elasticsearch.xpack.esql.core.expression.Expression;
27-
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
2826
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
2927
import org.elasticsearch.xpack.esql.core.tree.Source;
3028
import org.elasticsearch.xpack.esql.core.type.DataType;
@@ -44,10 +42,6 @@
4442
import static org.elasticsearch.xpack.esql.core.type.DataType.GEOTILE;
4543
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_POINT;
4644
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_SHAPE;
47-
import static org.elasticsearch.xpack.esql.expression.Foldables.valueOf;
48-
import static org.elasticsearch.xpack.esql.expression.function.scalar.spatial.SpatialRelatesUtils.asGeometryDocValueReader;
49-
import static org.elasticsearch.xpack.esql.expression.function.scalar.spatial.SpatialRelatesUtils.asLuceneComponent2D;
50-
import static org.elasticsearch.xpack.esql.expression.function.scalar.spatial.SpatialRelatesUtils.makeGeometryFromLiteral;
5145

5246
/**
5347
* This is the primary class for supporting the function ST_DISJOINT.
@@ -146,26 +140,8 @@ protected NodeInfo<? extends Expression> info() {
146140
}
147141

148142
@Override
149-
public Object fold(FoldContext ctx) {
150-
try {
151-
if (DataType.isGeoGrid(left().dataType())) {
152-
return foldGeoGrid(ctx, right(), left(), left().dataType());
153-
} else if (DataType.isGeoGrid(right().dataType())) {
154-
return foldGeoGrid(ctx, left(), right(), right().dataType());
155-
}
156-
GeometryDocValueReader docValueReader = asGeometryDocValueReader(ctx, crsType(), left());
157-
Component2D component2D = asLuceneComponent2D(ctx, crsType(), right());
158-
return (crsType() == SpatialCrsType.GEO)
159-
? GEO.geometryRelatesGeometry(docValueReader, component2D)
160-
: CARTESIAN.geometryRelatesGeometry(docValueReader, component2D);
161-
} catch (IOException e) {
162-
throw new IllegalArgumentException("Failed to fold constant fields: " + e.getMessage(), e);
163-
}
164-
}
165-
166-
private Object foldGeoGrid(FoldContext ctx, Expression spatialExp, Expression gridExp, DataType gridType) throws IOException {
167-
long gridId = (Long) valueOf(ctx, gridExp);
168-
return GEO.compareGeometryAndGrid(makeGeometryFromLiteral(ctx, spatialExp), gridId, gridType);
143+
protected SpatialRelations getSpatialRelations() {
144+
return crsType() == SpatialCrsType.GEO ? GEO : CARTESIAN;
169145
}
170146

171147
@Override

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

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,7 @@
2222
import org.elasticsearch.index.mapper.GeoShapeIndexer;
2323
import org.elasticsearch.lucene.spatial.CartesianShapeIndexer;
2424
import org.elasticsearch.lucene.spatial.CoordinateEncoder;
25-
import org.elasticsearch.lucene.spatial.GeometryDocValueReader;
2625
import org.elasticsearch.xpack.esql.core.expression.Expression;
27-
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
2826
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
2927
import org.elasticsearch.xpack.esql.core.tree.Source;
3028
import org.elasticsearch.xpack.esql.core.type.DataType;
@@ -44,10 +42,6 @@
4442
import static org.elasticsearch.xpack.esql.core.type.DataType.GEOTILE;
4543
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_POINT;
4644
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_SHAPE;
47-
import static org.elasticsearch.xpack.esql.expression.Foldables.valueOf;
48-
import static org.elasticsearch.xpack.esql.expression.function.scalar.spatial.SpatialRelatesUtils.asGeometryDocValueReader;
49-
import static org.elasticsearch.xpack.esql.expression.function.scalar.spatial.SpatialRelatesUtils.asLuceneComponent2D;
50-
import static org.elasticsearch.xpack.esql.expression.function.scalar.spatial.SpatialRelatesUtils.makeGeometryFromLiteral;
5145

5246
/**
5347
* This is the primary class for supporting the function ST_INTERSECTS.
@@ -144,26 +138,8 @@ protected NodeInfo<? extends Expression> info() {
144138
}
145139

146140
@Override
147-
public Object fold(FoldContext ctx) {
148-
try {
149-
if (DataType.isGeoGrid(left().dataType())) {
150-
return foldGeoGrid(ctx, right(), left(), left().dataType());
151-
} else if (DataType.isGeoGrid(right().dataType())) {
152-
return foldGeoGrid(ctx, left(), right(), right().dataType());
153-
}
154-
GeometryDocValueReader docValueReader = asGeometryDocValueReader(ctx, crsType(), left());
155-
Component2D component2D = asLuceneComponent2D(ctx, crsType(), right());
156-
return (crsType() == SpatialCrsType.GEO)
157-
? GEO.geometryRelatesGeometry(docValueReader, component2D)
158-
: CARTESIAN.geometryRelatesGeometry(docValueReader, component2D);
159-
} catch (IOException e) {
160-
throw new IllegalArgumentException("Failed to fold constant fields: " + e.getMessage(), e);
161-
}
162-
}
163-
164-
private Object foldGeoGrid(FoldContext ctx, Expression spatialExp, Expression gridExp, DataType gridType) throws IOException {
165-
long gridId = (Long) valueOf(ctx, gridExp);
166-
return GEO.compareGeometryAndGrid(makeGeometryFromLiteral(ctx, spatialExp), gridId, gridType);
141+
protected SpatialRelations getSpatialRelations() {
142+
return crsType() == SpatialCrsType.GEO ? GEO : CARTESIAN;
167143
}
168144

169145
@Override

0 commit comments

Comments
 (0)