Fix ST_DISTANCE handling of invalid geometry literals that fold to null by craigtaverner · Pull Request #140116 · elastic/elasticsearch

craigtaverner · 2026-01-02T10:50:04Z

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

Fixes #138594

GalLalouche · 2026-01-02T10:56:04Z

...ava/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialRelatesUtils.java

-                "Unsupported combination of literal [" + value.getClass().getSimpleName() + "] of type [" + dataType + "]"
-            );
+            var objType = (value == null ? "null" : value.getClass().getSimpleName());
+            throw new IllegalArgumentException("Unsupported combination of literal [" + objType + "] of type [" + dataType + "]");


Isn't there a difference between the Java null value, and an object of type null (i.e., ESQL's NULL)? Or is the former the way we represent the latter in this case?

Yes, the java null should never appear here, which is why I've been unable to reproduce this. I think the actual bug lies elsewhere and is creating this java null during the fold operation, somewhere in another function.

So it seems my st_distance.fold function was not correctly following null folding rules. We need explicit support for nulls here, and I've changed this code to return null instead of throwing an exception.

ivancea · 2026-01-02T11:35:32Z

...ava/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialRelatesUtils.java

Got to reproduce this with:

ROW x = MV_SLICE(["1"], 0, 0)::geo_point | EVAL y = st_distance(x, x)

I had to add that MV_SLICE part, that ends up being null, but probably avoids some other null folding rules

Thanks Ivan, that helped me track down and fix the real bug. It appears that fold needs to explicitly handle nulls. The default implementation of fold in EvaluatorMapper returns a raw java null, and all overriding implementations appear to need to do that too.

elasticsearchmachine · 2026-01-05T10:57:46Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2026-01-05T10:57:47Z

Hi @craigtaverner, I've created a changelog YAML for you.

…icsearch into geometry_literal_npe

elasticsearchmachine · 2026-01-06T10:53:23Z

Hi @craigtaverner, I've updated the changelog YAML for you.

…icsearch into geometry_literal_npe

GalLalouche · 2026-01-06T11:15:48Z

...main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialWithin.java

-            throw new IllegalArgumentException("Failed to fold constant fields: " + e.getMessage(), e);
-        }
+    protected SpatialRelations getSpatialRelations() {
+        return (crsType() == SpatialCrsType.GEO) ? GEO : CARTESIAN;


Redundant parens.

I find this kind of ternary expression easier to read when the condition is within parentheses. But your point is valid, I've been using parenthesis so long I started thinking they were required...

So I did this anyway, since the expression is so simple.

GalLalouche · 2026-01-06T11:18:42Z

.../org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialRelatesFunction.java

+                    return foldGeoGrid(ctx, left(), right(), right().dataType());
+                }
+            }
+            GeometryDocValueReader docValueReader = asGeometryDocValueReader(ctx, crsType(), left());


FWIW, you could shave a few cycles if you early exit here when docValuesReader is already null (though IIUC folding is done during planning so is probably not on the hot path).

Yes, not on the hot path. I think the code is simpler with a single if.

GalLalouche · 2026-01-06T11:21:29Z

...in/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialDisjoint.java

-        long gridId = (Long) valueOf(ctx, gridExp);
-        return GEO.compareGeometryAndGrid(makeGeometryFromLiteral(ctx, spatialExp), gridId, gridType);
+    protected SpatialRelations getSpatialRelations() {
+        return (crsType() == SpatialCrsType.GEO) ? GEO : CARTESIAN;


Redundant parens

GalLalouche · 2026-01-06T11:21:37Z

.../java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialIntersects.java

-        long gridId = (Long) valueOf(ctx, gridExp);
-        return GEO.compareGeometryAndGrid(makeGeometryFromLiteral(ctx, spatialExp), gridId, gridType);
+    protected SpatialRelations getSpatialRelations() {
+        return (crsType() == SpatialCrsType.GEO) ? GEO : CARTESIAN;


Redundant parens.

GalLalouche · 2026-01-06T11:24:46Z

.../org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialRelatesFunction.java

    }

+    /** Spatial relates functions always use Lucene GeometryDocValueReader and Component2D instead of geometries */
+    protected Object fold(Geometry leftGeom, Geometry rightGeom) {


Missing a @Override.

GalLalouche · 2026-01-06T11:25:25Z

...in/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialContains.java

-            throw new IllegalArgumentException("Failed to fold constant fields: " + e.getMessage(), e);
-        }
+    protected SpatialRelations getSpatialRelations() {
+        return (crsType() == SpatialCrsType.GEO) ? GEO : CARTESIAN;


Redundant parens.

GalLalouche · 2026-01-06T11:25:53Z

...a/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/BinarySpatialFunction.java

+
+    @Override
+    public Object fold(FoldContext ctx) {
+        var leftGeom = makeGeometryFromLiteral(ctx, left());


FWIW, you could shave a few cycles if you early exit here when leftGeom is already null (though IIUC folding is done during planning so is probably not on the hot path).

I'll stick with a single if for simplicity.

ST_CONTAINS needs a custom version of fold() due to the need to split all components into separate components. We could consider doing the same for all other functions, but that would require larger refactoring and could incut performance costs.

elasticsearchmachine · 2026-01-06T15:46:25Z

💔 Backport failed

Status	Branch	Result
❌	9.2	Commit could not be cherrypicked due to conflicts
❌	9.3	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 140116

…ll (elastic#140116) 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 elastic#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

craigtaverner · 2026-01-07T12:21:09Z

Manual backport in #140265

…ll (#140116) (#140265) 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

…ll (elastic#140116) (elastic#140265) 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 elastic#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

…ll (#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

…ll (elastic#140116) 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 elastic#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

craigtaverner added >bug :Analytics/Geo Indexing, search aggregations of geo points and shapes backport Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL branch:9.3 labels Jan 2, 2026

elasticsearchmachine added v9.3.1 v9.4.0 and removed branch:9.3 labels Jan 2, 2026

GalLalouche reviewed Jan 2, 2026

View reviewed changes

ivancea reviewed Jan 2, 2026

View reviewed changes

Better support of null in st_distance.fold

26513fb

craigtaverner force-pushed the geometry_literal_npe branch from db40e5f to 26513fb Compare January 5, 2026 10:45

craigtaverner changed the title ~~Geometry Literal: Change NullPointerException to IllegalArgumentException~~ Jan 5, 2026

craigtaverner added branch:9.2 auto-backport Automatically create backport pull requests when merged and removed backport labels Jan 5, 2026

elasticsearchmachine added v9.2.4 and removed branch:9.2 labels Jan 5, 2026

Update docs/changelog/140116.yaml

33297f8

GalLalouche approved these changes Jan 5, 2026

View reviewed changes

craigtaverner added 5 commits January 6, 2026 11:50

Expand null-handling to relates functions and reduce code duplication

ea18649

Merge remote-tracking branch 'origin/main' into geometry_literal_npe

0e99e83

Merge branch 'geometry_literal_npe' of github.com:craigtaverner/elast…

052e756

…icsearch into geometry_literal_npe

Fix changelog

9daef4e

Update docs/changelog/140116.yaml

81f411d

Expand test coverage to single null values

c82a1b7

craigtaverner added 2 commits January 6, 2026 12:06

Merge branch 'geometry_literal_npe' of github.com:craigtaverner/elast…

56556b1

…icsearch into geometry_literal_npe

Fixed comment

49b6a6b

GalLalouche approved these changes Jan 6, 2026

View reviewed changes

craigtaverner added 2 commits January 6, 2026 12:33

Code review fixes

86fc04b

craigtaverner merged commit db5f41f into elastic:main Jan 6, 2026
35 checks passed

elasticsearchmachine added the backport pending label Jan 6, 2026

craigtaverner mentioned this pull request Jan 7, 2026

[9.3] Fix ST_DISTANCE handling of invalid geometry literals that fold to null (#140116) #140265

Merged

craigtaverner removed the backport pending label Jan 7, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ST_DISTANCE handling of invalid geometry literals that fold to null#140116

Fix ST_DISTANCE handling of invalid geometry literals that fold to null#140116
craigtaverner merged 12 commits intoelastic:mainfrom
craigtaverner:geometry_literal_npe

craigtaverner commented Jan 2, 2026 •

edited

Loading

GalLalouche Jan 2, 2026

craigtaverner Jan 5, 2026

craigtaverner Jan 5, 2026

ivancea Jan 2, 2026 •

edited

Loading

craigtaverner Jan 5, 2026 •

edited

Loading

elasticsearchmachine commented Jan 5, 2026

elasticsearchmachine commented Jan 5, 2026

elasticsearchmachine commented Jan 6, 2026

GalLalouche Jan 6, 2026

craigtaverner Jan 6, 2026

craigtaverner Jan 6, 2026

GalLalouche Jan 6, 2026

craigtaverner Jan 6, 2026

GalLalouche Jan 6, 2026

craigtaverner Jan 6, 2026

GalLalouche Jan 6, 2026

craigtaverner Jan 6, 2026

GalLalouche Jan 6, 2026

craigtaverner Jan 6, 2026

GalLalouche Jan 6, 2026

craigtaverner Jan 6, 2026

GalLalouche Jan 6, 2026

craigtaverner Jan 6, 2026

Uh oh!

elasticsearchmachine commented Jan 6, 2026

craigtaverner commented Jan 7, 2026

Labels

4 participants

Conversation

craigtaverner commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivancea Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

craigtaverner Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 5, 2026

elasticsearchmachine commented Jan 5, 2026

elasticsearchmachine commented Jan 6, 2026

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jan 6, 2026

💔 Backport failed

craigtaverner commented Jan 7, 2026

Labels

4 participants

craigtaverner commented Jan 2, 2026 •

edited

Loading

ivancea Jan 2, 2026 •

edited

Loading

craigtaverner Jan 5, 2026 •

edited

Loading