ESQL: GEO_POINT and CARTESIAN_POINT type support #102177

craigtaverner · 2023-11-14T18:26:34Z

This is a port of a subset of the earlier prototype at #98845

The goal is support for geo_point and point in ES|QL.

At this point we support:

Support displaying geo_point columns in ESQL output tables
Support to_geopoint(field) function for encoded long and WKT keyword fields
Support for geo_point as argument to to_string(field) (renders as WKT)
Support format=csv with WKT output (defaults to GeoPoint.toString() which is "lat,lon")
Support for point type by duplicating the geo_point work
Disable mv_min/mv_max for spatial types (removing concept of sortability)
~~Support for geo_point and point with full precision (from source instead of doc-values)~~

This last option, using source and full precision requires a new Block type, or a new encoding to the BytesRef block type, and as such is being done in a followup PR at #103698

Fixes #103585

elasticsearchmachine · 2023-11-14T18:27:30Z

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

elasticsearchmachine · 2023-11-23T18:02:45Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-11-23T18:02:46Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

luigidellaquila

Thank you very much @craigtaverner!
I had just a very quick look (I'll need more time for a proper review) but there is one thing that is worth discussing at high level IMHO: I see you are returning geo_point values as WKT, but I wonder if it would be better to have plain JSON instead; it would be the same as in _search, more compact and easier to manipulate for clients

iverase · 2023-11-24T13:42:30Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMaxTests.java

@@ -44,6 +44,7 @@ public static Iterable<Object[]> parameters() {
            (size, values) -> equalTo(NumericUtils.asLongUnsigned(values.reduce(BigInteger::max).get()))
        );
        dateTimes(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.max().getAsLong()));
+        geoPoints(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.max().getAsLong()));


I don't think that makes a lot of sense, do we need to support it?

Good question. It was simplest to code this, rather than explicitly reject these, but we can discuss alternatives. I think, however, that a deterministic ordering of all points is something that might be necessary anyway, and in that case ordering by the doc-value is as good a choice as any (order by lat first then lon second).

iverase · 2023-11-24T13:42:50Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMinTests.java

@@ -44,6 +44,7 @@ public static Iterable<Object[]> parameters() {
            (size, values) -> equalTo(NumericUtils.asLongUnsigned(values.reduce(BigInteger::min).get()))
        );
        dateTimes(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.min().getAsLong()));
+        geoPoints(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.min().getAsLong()));


The same as above, I don't think that makes a lot of sense, do we need to support it?

iverase · 2023-11-24T13:44:11Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_unsupported_types.yml

@@ -175,25 +189,27 @@ unsupported:
  - match: { values.0.5: null }
  - match: { values.0.6: null }
  - match: { values.0.7: null }
-  - match: { values.0.8: null }
-  - match: { values.0.9: null }
+  - match: { values.0.8: "POINT (9.999999990686774 11.999999997206032)" }


That's the problem with this approach, using doc values for reading points is loosy.

Yes, and in fact my earlier experiment with getting from source started by fixing this test. We now have that planned as a followup PR.

iverase · 2023-11-24T13:48:37Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypes.java

@@ -44,6 +44,7 @@ public final class EsqlDataTypes {

    public static final DataType DATE_PERIOD = new DataType("DATE_PERIOD", null, 3 * Integer.BYTES, false, false, false);
    public static final DataType TIME_DURATION = new DataType("TIME_DURATION", null, Integer.BYTES + Long.BYTES, false, false, false);
+    public static final DataType GEO_POINT = new DataType("geo_point", Double.BYTES * 2, false, false, false);


I don't understand why the size of the field here is 2 doubles and we treat it everywhere as a long, what is the relationship? how is this information used?

Yes, I spotted that a while back and planned to fix it. I'll look again and see if I can get clarity on this.

iverase

I think the PR is very clean and easy to follow, just have a couple of questions.

My only concern is that using doc values to return the stored value to the user might be confusing as doc values are loosy as they encode the provided values as integers, therefore the transformation back to double yield a double that is different to the provided one (although they are less that 1cm away on the surface of the earth).

If we are to change this, so in a future release we might want to read values from source so when projecting the value to the user, would we need to suffer of backwards compatibility issues?

craigtaverner · 2023-11-24T14:37:17Z

If we are to change this, so in a future release we might want to read values from source so when projecting the value to the user, would we need to suffer of backwards compatibility issues?

The idea is to release as tech-preview and make no commitment to backwards compatibility for this type. Ideally we get the support for reading full precision from source into 8.12, but even if that only makes it into 8.13, since we do not need to commit to BWC, we only have to support this internally. We also don't plan for GA in 8.13, so we can continue to change things in 8.14 (and perhaps even a release after that).

craigtaverner · 2023-11-24T15:11:20Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/util/SpatialUtils.java

@@ -0,0 +1,58 @@
+/*


This should not be in the ql module as it is only used in esql. In addition the class name is not useful, since it is specific to coordinate encoding. Consider renaming and moving to the esql plugin.

I moved this code to the ESQL module, but then server tests failed on classloader errors, so I've moved it back until we figure out what is going on with classloading. All other changes like renaming the class, and making it an enum in preparation for Cartesian support are retained. The only unexpected requirement is that this class must be in the QL module for some reason.

* Based on the earlier prototype at elastic#98845 * The spatial types support changes between 8.11 and 8.12, so mixed clusters of these two versions should claim these types as unsupported, while pure 8.12 clusters should claim the latest type support. * The spatial types support changes between 8.11 and 8.12, so mixed clusters of these two versions should not run the spatial tests. * The test assumed data arrives in the original ingest order, and started with a LIMIT 10 after which predicates were applied. But in many situations this will not work, as the data is not in that order and the LIMIT 10 will result in different documents. So the solution is to create a query that does not depend on original order. * Fix format=csv output for ESQL geo_point * Fix format=csv|tsv output for ESQL geo_point * Refactor SpatialUtils and abstract away GeoPoint * GeoPoint implements SpatialPoint and the geo-specific knowledge was already in the SpatialUtils class, so we could increase the level of abstraction by using SpatialPoint everywhere. * This is in preparation for supporting CartesianPoint, and in future perhaps other systems. * Moved the SpatialCoordinateTypes back to QL module * For some reason the ESQL QA tests could not load it from ESQL, only from QL. Until we figure out what is going wrong with the classloader, we will return these classes to their previous module.

* Primarily the difference is only that mapping to doc-values puts x as high 4 bytes, and y as low 4 bytes, while geo_point does the opposite. * As long as we only support doc-values for the compute engine, this is the only difference we need to worry about.

Even though geo_point and cartesian_point are backed by LONG, making them implicitly sortable, we exclude them from sorting because the sort order is not particularly useful.

iverase · 2023-11-29T14:24:54Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypes.java

@@ -44,6 +44,8 @@ public final class EsqlDataTypes {

    public static final DataType DATE_PERIOD = new DataType("DATE_PERIOD", null, 3 * Integer.BYTES, false, false, false);
    public static final DataType TIME_DURATION = new DataType("TIME_DURATION", null, Integer.BYTES + Long.BYTES, false, false, false);
+    public static final DataType GEO_POINT = new DataType("geo_point", Double.BYTES * 2, false, false, false);
+    public static final DataType CARTESIAN_POINT = new DataType("cartesian_point", Double.BYTES * 2, false, false, false);


This should be Float.Bytes?

iverase

LGTM from the geo side.

I would still like to understand what are the size of the esqltype and what is used for. I think someone with more knowledge on the engine should review it.

craigtaverner · 2023-11-29T15:58:31Z

I would still like to understand what are the size of the esqltype and what is used for. I think someone with more knowledge on the engine should review it.

This was discussed with the team, and even though the current implementation would more accurately be Long.BYTES instead of Double.BYTES * 2, the consequences are not material and relate only to estimating page size allocations. Since the plan is to move towards reading from source and using two doubles in the compute engine in the next PR, I felt it was not worth changing it here, only to revert in the next PR.

nik9000 · 2023-11-28T17:00:53Z

server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java

+            return new BlockSourceReader.LongsBlockLoader(
+                valueFetcher(blContext.sourcePaths(name()), nullValue, GeometryFormatterFactory.WKT)
+            );
+        }


👍

This is plenty so long as we don't support synthetic _source without doc values. If we do that then we need more complexity here ala text or keyword. But this is fine.

Actually - does the WKT thing load everything that _source does?

Actually - does the WKT thing load everything that _source does?

What is the 'WKT thing'? Do you mean the fact that we display stuff as WKT in ESQL? Right now we generate the WKT from doc-values, but will instead generate it from _source in the next PR. Even if the _source was itself WKT, the final display will be generated, because the intermediate format will be binary block values. This differs from _search where the results are never generated, but always _source.

Right right - you'll build WKT when you load from _source. You just aren't doing that. I suppose my question was "is the WKT thing here a parser config or can you still parse all supported point types?" Because I'm ok delaying any questions about parsing from source into the lossless render until later.

nik9000 · 2023-11-29T16:12:43Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

+        return new GeoPoint(randomDoubleBetween(-90, 90, true), randomDoubleBetween(-180, 180, true));
+    }
+
+    public static SpatialPoint randomCartesianPoint() {


These should have javadoc just because they are visible to like 100 people.

nik9000 · 2023-11-29T16:15:42Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/util/SpatialCoordinateTypes.java

+            return new GeoPoint(point.getY(), point.getX());
+        }
+    },
+    Cartesian {


s/Cartesian/CARTESIAN/?

And I presume you want s/Geo/GEO/ too?

nik9000 · 2023-11-29T16:17:08Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/ColumnInfo.java

+                protected XContentBuilder valueToXContent(XContentBuilder builder, ToXContent.Params params, int valueIndex)
+                    throws IOException {
+                    // TODO Perhaps this is just a long for geo_point? And for more advanced types we need a new block type
+                    long encoded = (block instanceof LongVectorBlock)


Could you cast to LongBlock?

I think you can here.

## Summary Add support to `to_geopoint` as in elastic/elasticsearch#102177

craigtaverner added >feature Team:QL (Deprecated) Meta label for query languages team :Analytics/ES|QL AKA ESQL labels Nov 14, 2023

elasticsearchmachine added the v8.12.0 label Nov 14, 2023

craigtaverner changed the title ~~WIP Initial support for geo_point~~ ESQL: GEO_POINT type support Nov 14, 2023

craigtaverner force-pushed the esql_geopoint branch from f2be5eb to 7d0e78b Compare November 14, 2023 20:58

craigtaverner force-pushed the esql_geopoint branch from 7d0e78b to 03ac699 Compare November 22, 2023 10:35

craigtaverner marked this pull request as ready for review November 23, 2023 18:02

luigidellaquila reviewed Nov 24, 2023

View reviewed changes

iverase reviewed Nov 24, 2023

View reviewed changes

craigtaverner commented Nov 24, 2023

View reviewed changes

craigtaverner mentioned this pull request Nov 27, 2023

ESQL: CARTESIAN_POINT type support #102655

Closed

nik9000 self-requested a review November 27, 2023 16:36

craigtaverner added 3 commits November 29, 2023 11:44

Exclude spatial types from sortability (mv_min/max)

532838e

Even though geo_point and cartesian_point are backed by LONG, making them implicitly sortable, we exclude them from sorting because the sort order is not particularly useful.

craigtaverner force-pushed the esql_geopoint branch from cb3520d to 532838e Compare November 29, 2023 10:58

craigtaverner changed the title ~~ESQL: GEO_POINT type support~~ ESQL: GEO_POINT and CARTESIAN_POINT type support Nov 29, 2023

iverase reviewed Nov 29, 2023

View reviewed changes

nik9000 requested changes Nov 29, 2023

View reviewed changes

craigtaverner added 2 commits November 30, 2023 11:26

Code review updates

9a86c58

Update changelog to include cartesian_point

7f548a0

nik9000 approved these changes Nov 30, 2023

View reviewed changes

craigtaverner merged commit 1ea66d7 into elastic:main Nov 30, 2023
15 checks passed

stratoula added the ES|QL-ui Impacts ES|QL UI label Dec 7, 2023

dej611 mentioned this pull request Dec 7, 2023

[ES|QL] Add to_geopoint function support elastic/kibana#172815

Merged

kertal mentioned this pull request Dec 7, 2023

[Discover] Unskip field tokens tests elastic/kibana#172794

Merged

dej611 added a commit to elastic/kibana that referenced this pull request Dec 7, 2023

[ES|QL] Add to_geopoint function support (#172815)

92024dd

## Summary Add support to `to_geopoint` as in elastic/elasticsearch#102177

nreese mentioned this pull request Dec 7, 2023

[maps] ES|QL source elastic/kibana#167648

Closed

This was referenced Dec 8, 2023

ESQL: Documentation for support for geo_point and point #103207

Merged

GEO_POINT for ESQL #98845

Closed

ESQL: Reading points from source #102880

Closed

This was referenced Dec 19, 2023

Support geo_point in ES|QL #103585

Closed

ESQL Support loading points from source into WKB blocks #103698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: GEO_POINT and CARTESIAN_POINT type support #102177

ESQL: GEO_POINT and CARTESIAN_POINT type support #102177

craigtaverner commented Nov 14, 2023 •

edited

Loading

elasticsearchmachine commented Nov 14, 2023

elasticsearchmachine commented Nov 23, 2023

elasticsearchmachine commented Nov 23, 2023

luigidellaquila left a comment

iverase Nov 24, 2023

craigtaverner Nov 24, 2023

iverase Nov 24, 2023

iverase Nov 24, 2023

craigtaverner Nov 24, 2023

iverase Nov 24, 2023

craigtaverner Nov 24, 2023

iverase left a comment

craigtaverner commented Nov 24, 2023

craigtaverner Nov 24, 2023

craigtaverner Nov 24, 2023

iverase Nov 29, 2023

iverase left a comment

craigtaverner commented Nov 29, 2023

nik9000 Nov 28, 2023

nik9000 Nov 28, 2023

craigtaverner Nov 30, 2023

nik9000 Nov 30, 2023

nik9000 Nov 29, 2023

nik9000 Nov 29, 2023

craigtaverner Nov 30, 2023

nik9000 Nov 30, 2023

nik9000 Nov 29, 2023

nik9000 Nov 29, 2023

ESQL: GEO_POINT and CARTESIAN_POINT type support #102177

ESQL: GEO_POINT and CARTESIAN_POINT type support #102177

Conversation

craigtaverner commented Nov 14, 2023 • edited Loading

elasticsearchmachine commented Nov 14, 2023

elasticsearchmachine commented Nov 23, 2023

elasticsearchmachine commented Nov 23, 2023

luigidellaquila left a 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

iverase left a comment

Choose a reason for hiding this comment

craigtaverner commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iverase left a comment

Choose a reason for hiding this comment

craigtaverner commented Nov 29, 2023

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

craigtaverner commented Nov 14, 2023 •

edited

Loading