Add runtime field of type geo_shape #100492

iverase · 2023-10-09T09:49:45Z

This PR adds the possibility to create runtime fields of type geo-shape. In order to create them, user s can define an emit function that takes either a geojson object or a WKT string that internally creates a geo_shape object. For example from a geo_shape doc value, we can generate the WKT representing the bounding box of the shape like (in yaml):

runtime:
              geometry_from_doc_value:
                type: geo_shape
                script:
                  source: |
                    GeoBoundingBox bbox = doc["geometry"].getBoundingBox();
                    emit("BBOX(" + bbox.topLeft().getLon() + "," + bbox.topLeft().getLat() + "," + bbox.bottomRight().getLon() + "," + bbox.bottomRight().getLat() + ")");

closes #61299

elasticsearchmachine · 2023-10-09T09:50:09Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticsearchmachine · 2023-10-09T09:50:10Z

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

romseygeek

Mostly LGTM, but I have a question about concurrency that I think we need to address before merge.

server/src/main/java/org/elasticsearch/script/GeometryFieldScript.java

...main/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java

...main/java/org/elasticsearch/xpack/spatial/search/runtime/GeoShapeScriptFieldExistsQuery.java

...in/java/org/elasticsearch/xpack/spatial/search/runtime/GeoShapeScriptFieldGeoShapeQuery.java

romseygeek

LGTM!

iverase · 2023-10-11T15:10:56Z

@elasticmachine update branch

craigtaverner

Very nice feature. I was worried about how most tests just used points, but I see the yaml tests do a much better job of covering other types. I did have a number of comments that could be considered, and also noticed that there exists documentation for using geo_point_field in painless, but did not see anything for geometry_field. Should that be added to a separate PR?

craigtaverner · 2023-10-13T08:51:59Z

...les/lang-painless/src/main/java/org/elasticsearch/painless/action/PainlessExecuteAction.java

+                        context.lookup(),
+                        OnScriptError.FAIL
+                    );
+                    GeometryFieldScript geoPointFieldScript = leafFactory.newInstance(leafReaderContext);


variable name is from previous block

Right, updated.

craigtaverner · 2023-10-13T09:24:51Z

...s/lang-painless/src/test/java/org/elasticsearch/painless/action/PainlessExecuteApiTests.java

@@ -259,6 +259,50 @@ public void testGeoPointFieldExecutionContext() throws IOException {
        assertEquals("Point", points.get(1).get("type"));
    }

+    @SuppressWarnings("unchecked")
+    public void testGeoShapeFieldExecutionContext() throws IOException {


This test seems to be a copy of the geo_point test, with just the field context name changed. Is this because we cannot test actual shapes in server, so just verify that the new script context can handle points? Should we refactor this to a private method with a single parameter?

I improved the test, thanks!

craigtaverner · 2023-10-13T09:30:09Z

server/src/test/java/org/elasticsearch/index/mapper/GeometryFieldScriptTests.java

+    ) {
+        @Override
+        public void execute() {
+            emitFromObject("POINT(0 0)");


Is there perhaps value in testing something more than a point, perhaps a two point linestring?

craigtaverner · 2023-10-13T09:42:30Z

...main/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java

                geoFormatterFactory,
                meta.get()
            );
+            if (script.get() == null) {
+                return new GeoShapeWithDocValuesFieldMapper(


I noticed these two versions of the constructor, but when looking up the constructor stack, I see they do ultimately end up at the same place, so I wonder if it is not cleaner to have a single constructor with just null for the script parts when missing? This would reduce code in several classes that have multiple constructors just for scripts.

The base class expect a different constructor to be used. This might be a nice follow up but not for this PR as it touches other mappings like geo_point.

craigtaverner · 2023-10-13T09:43:03Z

...main/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java

@@ -339,6 +393,21 @@ public GeoShapeWithDocValuesFieldMapper(
        this.indexer = indexer;
    }

+    protected GeoShapeWithDocValuesFieldMapper(


Same comment as above that we could remove these extra constructors, I think.

craigtaverner · 2023-10-13T09:48:15Z

...src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeScriptFieldTypeTests.java

+import static java.util.Collections.emptyMap;
+import static org.hamcrest.Matchers.equalTo;
+
+public class GeoShapeScriptFieldTypeTests extends AbstractNonTextScriptFieldTypeTestCase {


All these tests seem to test only points. I wonder if there is value in testing something a little more complex, perhaps at least a two point linestring?

I changed to run with a LINESTRING

craigtaverner · 2023-10-13T09:59:45Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/spatial/130_geo_shape_runtime.yml

+      search:
+        index: geometries
+        body:
+          aggs:


The fact that max-lat==max-lon==6 and min-lat==min-lon==-1 makes it harder to test for the possibility that x/y get swapped. When I looked through the assertions below at first I thought there was a x/y swapping error, before I realized my mistake. These results are correct for the original data provided.

…nto geoshape-runtime

Add runtime field of type geo_shape

efa0a6d

iverase added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes v8.12.0 labels Oct 9, 2023

iverase requested review from craigtaverner and javanna October 9, 2023 09:49

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 9, 2023

Update docs/changelog/100492.yaml

ef5fc98

romseygeek reviewed Oct 11, 2023

View reviewed changes

iverase added 5 commits October 11, 2023 12:51

Merge branch 'main' into geoshape-runtime

9216e9c

Address review comments

e6fd8fa

Merge branch 'main' into geoshape-runtime

bcded20

index does not throw IOException anymore

7c29dd2

doh! need to move the Geometry reader to matches too

139e69c

romseygeek approved these changes Oct 11, 2023

View reviewed changes

Add GeometryFieldScript to MockScriptEngine

3a3d7bc

Merge branch 'main' into geoshape-runtime

521a7c2

craigtaverner approved these changes Oct 13, 2023

View reviewed changes

iverase added 4 commits October 15, 2023 11:30

Merge branch 'main' into geoshape-runtime

e1dc926

Merge branch 'geoshape-runtime' of github.com:iverase/elasticsearch i…

999e8b7

…nto geoshape-runtime

painless bit fixed

b56c486

improve GeoShapeScriptFieldTypeTests

161318b

iverase merged commit a82f0ac into elastic:main Oct 16, 2023

iverase deleted the geoshape-runtime branch October 16, 2023 11:14

nreese mentioned this pull request Oct 25, 2023

[maps] add support for geo_shape runtime field elastic/kibana#169892

Closed

This was referenced Dec 5, 2023

Sync with main (ES 8.12-SNAPSHOT) elastic/logstash-filter-elastic_integration#107

Closed

Re-sync with ES 8.12 branch. elastic/logstash-filter-elastic_integration#113

Merged

This was referenced Mar 17, 2025

Fix indices.get_mapping API elastic/elasticsearch-specification#3976

Closed

Fix indices.get_mapping API elastic/elasticsearch-specification#3977

Merged

Add runtime field of type geo_shape #100492

Add runtime field of type geo_shape #100492

Uh oh!

Conversation

iverase commented Oct 9, 2023

Uh oh!

elasticsearchmachine commented Oct 9, 2023

Uh oh!

elasticsearchmachine commented Oct 9, 2023

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

iverase commented Oct 11, 2023

Uh oh!

craigtaverner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iverase Oct 16, 2023 •

edited

Loading