Add painless script support for Geoshape field #72886

iverase · 2021-05-10T13:23:43Z

This PR proposes the extension of Painless API to support accessing selected information stored in geo fields. The new methods are based in two immediate needs:

We have request to be able to query geo shapes by some geometric characteristics. e.g. height or width (Query by width/height or area of a geo-shape #54218). This information is not available in the lucene index but it is available in the geoshape doc values. Therefore adding the possibility to get that information with painless, it we ca support such queries using runtime fields. For example, we can sort our geoshapes by height using the following syntax:

GET /example/_search
{
   "runtime_mappings": {
    "width": {
      "type": "double",
      "script": {
        "source": "emit(doc['location'].width())"
      }
    }
  },
  "sort": [
    {
      "width": {
        "order": "desc"
      }
    }
  ]
}

Accessing the centroid of a geoshape. This allows for an. application to use that information to place a label in the centroid of a geoshape. The syntax to access that information would look like:

GET /example/_search
{
   "runtime_mappings": {
    "centroidLat": {
      "type": "double",
      "script": {
        "source": "emit(doc['location'].getCentroidLat())"
      }
    },
   "centroidLon": {
      "type": "double",
      "script": {
        "source": "emit(doc['location'].getCentroidLon())"
      }
    }
  },
  "fields": [centroidLon, centroidLat]
}

The new methods will ideally be supported by both geo points and geo shapes.

This PR added a new abstract class ScriptDocValues.Geometry that is the base class for ScriptDocValues.GeoPoints and GeoShapeScriptValues. This class exposes the following new methods:

        /** Centroid latitude */
        public abstract double getCentroidLat();
        /** Centroid longitude */
        public abstract double getCentroidLon();
        /** width of the geometry in degrees */
        public abstract double width();
        /** height of the geometry in degrees */
        public abstract double height();

Let me know what you think.

elasticmachine · 2021-05-10T13:23:47Z

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

elasticmachine · 2021-05-10T13:23:47Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

jdconrad · 2021-05-10T16:02:46Z

This looks good to me. I'd like to see some tests in Painless (guessing yaml makes the most sense) just to see this working correctly. Also is there ever a chance a geoshape has no points?

imotov

LGTM in general. I wonder if we can make the interface a bit more generic.

imotov · 2021-05-10T22:17:39Z

modules/lang-painless/src/main/resources/org/elasticsearch/painless/spi/org.elasticsearch.txt

+        double getCentroidLat();
+        double getCentroidLon();
+        double width();
+        double height();


I think these name might be confusing since these are not width and height of the shape but rather width and height of the bounding box. Which makes me think maybe we should return a point and a bounding box here. In case of the bounding box we will also give access to min/max lat/lon which we have to calculate anyway.

iverase · 2021-05-11T10:15:08Z

Thanks for the quick feedback.

@imotov : Yes, I think a more generic interface is a must and height and width are weird as the mean distances but they are expressed in degrees. We normally represent distances in meters in geo objects. I have changed to the following:

class org.elasticsearch.index.fielddata.ScriptDocValues$Geometry {
   org.elasticsearch.common.geo.GeoPoint getCentroid()
   org.elasticsearch.common.geo.GeoBoundingBox getBoundingBox()
 }

class org.elasticsearch.common.geo.GeoBoundingBox {
   double top()
   double bottom()
   double left()
   double right()
 }

We can use two geopoints for GeoBoundingBox but because the current order is so anti natural (top left and bottom right) I decided to use singular calls. I can change my mind.

@jdconrad I added yaml test and I left some questions. I don't think we accept empty geoshapes so we should be good there.

iverase · 2021-05-11T11:11:49Z

x-pack/plugin/spatial/src/yamlRestTest/resources/rest-api-spec/test/70_script_doc_values.yml

+              script:
+                source: "doc['geo_shape'].getBoundingBox()"
+  - match: { hits.hits.0.fields.bbox.0.top_left.lat: 59.942749994806945 }
+  - match: { hits.hits.0.fields.bbox.0.top_left.lon: 24.045249950140715 }


if you use in source doc['geo_shape'].get(0), you get a horrible error. What should we do in that case?

What is the error?

This is the error which is expected:

1> "type" : "illegal_argument_exception", 1> "reason" : "cannot write xcontent for unknown value of type class org.elasticsearch.xpack.spatial.index.fielddata.GeoShapeValues$GeoShapeValue", 1> "stack_trace" : "org.elasticsearch.ElasticsearchException$1: cannot write xcontent for unknown value of type class org.elasticsearch.xpack.spatial.index.fielddata.GeoShapeValues$GeoShapeValue

GeoShapeValue is just a binary doc value that do not have XContent representation.

OH! I thought this referenced a Painless error. I see why the change to GeoBoundingBox now. I'll defer to @imotov for this :)

Yes, painless is doing the right thing. Is there any guideline when the actual doc value has no XContent value but only methods to do stuff?

I understand, this is a bit of an edge case. geo_shape doc values are an abstract representation of the geometry where you can get some information like the centroid or bounding box or some actions like checking spatial relationships with other geometries.

I guess the question is if we should error out if someone try to get the serialise representation or we should consider some representation, e.g in that case we just return the dimensional type?

The field script that is used in the test returns Object. Whatever concrete type is concerned needs to be representable in JSON. The error is due to GeoShapeValue not being writable to xcontent. You'll need to add an xcontent writer (see for example XContentElasticsearchExtension.java which registers the writer for many date classes that may be returned from a script).

I understand. The problem is the geo_shape doc values is an interval tree of the tessellated shape design to perform very fast spatial operations but it currently looses the ability to construct the original shape. Therefore we cannot serialise into anything meaningful to the user.

GeoShapeValue now implements ToXContentFragment and throws an error if toXContent is called. I have some ideas of what we can do here but it requires more discussions.

It would be awesome to serialize it into GeoJSON of the original shape, but in the way it is currently stored it is indeed pretty useless. So throwing an error message or serializing it into a string containing a placeholder like "<geo_shape>" or something like this is probably the best we can do at the moment.

My vote goes to throw an error so users do not have the temptation to rely in whatever value is used as placeholder?

iverase · 2021-05-11T11:13:15Z

server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java

                }
+                centroid.reset(centroidLat / count, centroidLon / count);
+                boundingBox.topLeft().reset(maxLat, minLon);
+                boundingBox.bottomRight().reset(minLat, maxLon);


One of my worries here is to make the construction of a GeoPoint script value expensive.

On the other hand it seems there is a small bug here as we were building new GeoPoint objects for each new doc value which seems wasteful.

Given this is an internal detail we could always attempt to improve this at a later time.

iverase · 2021-05-11T11:13:47Z

server/src/main/java/org/elasticsearch/common/geo/GeoBoundingBox.java

@@ -29,7 +29,7 @@
 * A class representing a Geo-Bounding-Box for use by Geo queries and aggregations
 * that deal with extents/rectangles representing rectangular areas of interest.
 */
-public class GeoBoundingBox implements ToXContentObject, Writeable {
+public class GeoBoundingBox implements ToXContentFragment, Writeable {


I have to make GeoBoundingBox to behave more like a GeoPoint

imotov · 2021-05-11T15:13:14Z

If width and height are important, we can add them to GeoBoundingBox as well. On this class it will make perfect sense and will not be misleading.

iverase · 2021-05-11T16:34:05Z

After speaking with @imotov and more thinking, the final API should look lie:

class org.elasticsearch.index.fielddata.ScriptDocValues$Geometry {
  int getDimensionalType()
  org.elasticsearch.common.geo.GeoPoint getCentroid()
  org.elasticsearch.common.geo.GeoBoundingBox getBoundingBox()
 }

class org.elasticsearch.common.geo.GeoBoundingBox {
  org.elasticsearch.common.geo.GeoPoint topLeft()
  org.elasticsearch.common.geo.GeoPoint bottomRight()
 }

iverase · 2021-05-12T07:42:57Z

I added more test, in particular when the geometry is null. In the case getCentroid() and getBoundingBox() returns null and getDimensionalType() returns -1.

iverase · 2021-05-26T13:30:15Z

@jdconrad is there anything to do in order to move this forward?

jdconrad

LGTM! Thanks for adding this.

Users can access the centroid, bounding box and dimensional type of the shape.

Add painless script support for Geoshape field

d13edc0

iverase added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 labels May 10, 2021

elasticmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Core/Infra Meta label for core/infra team labels May 10, 2021

iverase marked this pull request as draft May 10, 2021 13:23

iverase requested review from imotov and rjernst May 10, 2021 13:24

imotov reviewed May 10, 2021

View reviewed changes

Make new interface more generic

7eb560e

Make GeoBoundingBox a ToXContentFragment

4febdc3

iverase commented May 11, 2021

View reviewed changes

Merge branch 'master' into GeoShapeScriptDocValues

f488c94

iverase added 2 commits May 11, 2021 17:53

Use geopoints for geoboundingbox

8be69d0

Add dimensional type

31d6b9b

iverase added 3 commits May 12, 2021 06:59

Merge branch 'master' into GeoShapeScriptDocValues

b019a47

handle null values properly

b994a04

geoshapevalue now implements ToXContentFragment

35154e7

iverase marked this pull request as ready for review May 12, 2021 07:43

iverase added the v7.14.0 label May 12, 2021

jdconrad approved these changes May 26, 2021

View reviewed changes

Merge branch 'master' into GeoShapeScriptDocValues

1de954e

iverase merged commit 409b6ce into elastic:master May 26, 2021

iverase mentioned this pull request May 26, 2021

Add painless script support for geo_shape field (#72886) #73432

Merged

iverase deleted the GeoShapeScriptDocValues branch May 26, 2021 16:59

iverase added a commit that referenced this pull request May 27, 2021

Add painless script support for geo_shape field (#72886) (#73432)

f926d42

Users can access the centroid, bounding box and dimensional type of the shape.

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

craigtaverner mentioned this pull request Sep 1, 2023

Failed Serialization of Scripted Geo fields #99089

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add painless script support for Geoshape field #72886

Add painless script support for Geoshape field #72886

iverase commented May 10, 2021 •

edited

Loading

elasticmachine commented May 10, 2021

elasticmachine commented May 10, 2021

jdconrad commented May 10, 2021

imotov left a comment

imotov May 10, 2021

iverase commented May 11, 2021

iverase May 11, 2021

jdconrad May 11, 2021

iverase May 11, 2021 •

edited

Loading

jdconrad May 11, 2021

iverase May 11, 2021

iverase May 11, 2021

rjernst May 11, 2021 •

edited

Loading

iverase May 12, 2021

imotov May 12, 2021

iverase May 25, 2021

iverase May 11, 2021

jdconrad May 11, 2021

iverase May 11, 2021

imotov commented May 11, 2021

iverase commented May 11, 2021 •

edited

Loading

iverase commented May 12, 2021

iverase commented May 26, 2021

jdconrad left a comment

Add painless script support for Geoshape field #72886

Add painless script support for Geoshape field #72886

Conversation

iverase commented May 10, 2021 • edited Loading

elasticmachine commented May 10, 2021

elasticmachine commented May 10, 2021

jdconrad commented May 10, 2021

imotov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iverase commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iverase May 11, 2021 • edited Loading

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

rjernst May 11, 2021 • edited Loading

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

imotov commented May 11, 2021

iverase commented May 11, 2021 • edited Loading

iverase commented May 12, 2021

iverase commented May 26, 2021

jdconrad left a comment

Choose a reason for hiding this comment

iverase commented May 10, 2021 •

edited

Loading

iverase May 11, 2021 •

edited

Loading

rjernst May 11, 2021 •

edited

Loading

iverase commented May 11, 2021 •

edited

Loading