ESQL Support loading points from source into WKB blocks

docs/changelog/103698.yaml

@@ -0,0 +1,5 @@
+pr: 103698
+summary: Reading points from source to reduce precision loss
+area: ES|QL
+type: enhancement
+issues: []

server/src/main/java/org/elasticsearch/TransportVersions.java

@@ -179,6 +179,7 @@ static TransportVersion def(int id) {
     public static final TransportVersion SMALLER_RELOAD_SECURE_SETTINGS_REQUEST = def(8_567_00_0);
     public static final TransportVersion UPDATE_API_KEY_EXPIRATION_TIME_ADDED = def(8_568_00_0);
     public static final TransportVersion LAZY_ROLLOVER_ADDED = def(8_569_00_0);
+    public static final TransportVersion ESQL_PLAN_POINT_LITERAL_WKB = def(8_570_00_0);
 
     /*
      * STOP! READ THIS FIRST! No, really,

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

@@ -12,8 +12,30 @@
  * To facilitate maximizing the use of common code between GeoPoint and projected CRS
  * we introduced this ElasticPoint as an interface of commonality.
  */
-public interface SpatialPoint {
+public interface SpatialPoint extends Comparable<SpatialPoint> {
     double getX();
 
     double getY();
+
+    default String toWKT() {
+        // Code designed to mimic WellKnownText.toWKT, with much less stack depth and object creation
+        return "POINT (" + getX() + " " + getY() + ")";
+    }
+
+    @Override
+    default int compareTo(SpatialPoint other) {
+        if (this.getClass().equals(other.getClass())) {
+            double xd = this.getX() - other.getX();
+            double yd = this.getY() - other.getY();
+            return (xd == 0) ? comparison(yd) : comparison(xd);
+        } else {
+            // TODO: Rather separate based on CRS, but since we don't have that yet, we use class name
+            // The sort order here is unimportant and does not (yet) introduce BWC issues, so we are free to change it later with CRS
+            return this.getClass().getSimpleName().compareTo(other.getClass().getSimpleName());
+        }
+    }
+
+    private int comparison(double delta) {
+        return delta == 0 ? 0 : delta < 0 ? -1 : 1;
+    }
 }

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

@@ -87,16 +87,19 @@ private static XContentParser wrapObject(Object sourceMap) throws IOException {
     public abstract static class AbstractGeometryFieldType<T> extends MappedFieldType {
 
         protected final Parser<T> geometryParser;
+        protected final T nullValue;
 
         protected AbstractGeometryFieldType(
             String name,
             boolean indexed,
             boolean stored,
             boolean hasDocValues,
             Parser<T> geometryParser,
+            T nullValue,
             Map<String, String> meta
         ) {
             super(name, indexed, stored, hasDocValues, TextSearchInfo.NONE, meta);
+            this.nullValue = nullValue;
             this.geometryParser = geometryParser;
         }
 
@@ -127,7 +130,7 @@ protected Object parseSourceValue(Object value) {
 
         public ValueFetcher valueFetcher(Set<String> sourcePaths, Object nullValue, String format) {
             Function<List<T>, List<Object>> formatter = getFormatter(format != null ? format : GeometryFormatterFactory.GEOJSON);
-            return new ArraySourceValueFetcher(sourcePaths, nullValue) {
+            return new ArraySourceValueFetcher(sourcePaths, nullValueAsSource(nullValue)) {
                 @Override
                 protected Object parseSourceValue(Object value) {
                     final List<T> values = new ArrayList<>();
@@ -136,6 +139,8 @@ protected Object parseSourceValue(Object value) {
                 }
             };
         }
+
+        protected abstract Object nullValueAsSource(Object nullValue);
     }
 
     private final Explicit<Boolean> ignoreMalformed;

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

@@ -10,11 +10,14 @@
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.Explicit;
 import org.elasticsearch.common.TriFunction;
+import org.elasticsearch.common.geo.GeometryFormatterFactory;
+import org.elasticsearch.common.geo.SpatialPoint;
 import org.elasticsearch.core.CheckedConsumer;
 import org.elasticsearch.core.CheckedFunction;
 import org.elasticsearch.xcontent.XContentParser;
 
 import java.io.IOException;
+import java.util.Map;
 import java.util.Objects;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -155,4 +158,36 @@ private void parseAndConsumeFromObject(
             }
         }
     }
+
+    public abstract static class AbstractPointFieldType<T extends SpatialPoint> extends AbstractGeometryFieldType<T> {
+
+        protected AbstractPointFieldType(
+            String name,
+            boolean indexed,
+            boolean stored,
+            boolean hasDocValues,
+            Parser<T> geometryParser,
+            T nullValue,
+            Map<String, String> meta
+        ) {
+            super(name, indexed, stored, hasDocValues, geometryParser, nullValue, meta);
+        }
+
+        @Override
+        protected Object nullValueAsSource(Object nullValue) {
+            if (nullValue == null) {
+                return null;
+            }
+            SpatialPoint point = (SpatialPoint) nullValue;
+            return point.toWKT();
+        }
+
+        @Override
+        public BlockLoader blockLoader(BlockLoaderContext blContext) {
+            // Currently we can only load from source in ESQL
+            ValueFetcher fetcher = valueFetcher(blContext.sourcePaths(name()), nullValue, GeometryFormatterFactory.WKB);
+            // TODO consider optimization using BlockSourceReader.lookupFromFieldNames(blContext.fieldNames(), name())
+            return new BlockSourceReader.GeometriesBlockLoader(fetcher, BlockSourceReader.lookupMatchingAll());
+        }
+    }
 }

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

@@ -55,13 +55,25 @@ protected AbstractShapeGeometryFieldType(
             Orientation orientation,
             Map<String, String> meta
         ) {
-            super(name, isSearchable, isStored, hasDocValues, parser, meta);
+            super(name, isSearchable, isStored, hasDocValues, parser, null, meta);
             this.orientation = orientation;
         }
 
         public Orientation orientation() {
             return this.orientation;
         }
+
+        @Override
+        public BlockLoader blockLoader(BlockLoaderContext blContext) {
+            // TODO: Support shapes in ESQL
+            return null;
+        }
+
+        @Override
+        protected Object nullValueAsSource(Object nullValue) {
+            // TODO: When we support shapes in ESQL; we need to return a shape in source format here
+            return nullValue;
+        }
     }
 
     protected Explicit<Boolean> coerce;

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

@@ -199,6 +199,27 @@ protected String name() {
         }
     }
 
+    public static class GeometriesBlockLoader extends SourceBlockLoader {
+        public GeometriesBlockLoader(ValueFetcher fetcher, LeafIteratorLookup lookup) {
+            super(fetcher, lookup);
+        }
+
+        @Override
+        public final Builder builder(BlockFactory factory, int expectedCount) {
+            return factory.bytesRefs(expectedCount);
+        }
+
+        @Override
+        protected RowStrideReader rowStrideReader(LeafReaderContext context, DocIdSetIterator iter) {
+            return new Geometries(fetcher, iter);
+        }
+
+        @Override
+        protected String name() {
+            return "Geometries";
+        }
+    }
+
     private static class BytesRefs extends BlockSourceReader {
         private final BytesRef scratch = new BytesRef();
 
@@ -217,6 +238,27 @@ public String toString() {
         }
     }
 
+    private static class Geometries extends BlockSourceReader {
+
+        Geometries(ValueFetcher fetcher, DocIdSetIterator iter) {
+            super(fetcher, iter);
+        }
+
+        @Override
+        protected void append(BlockLoader.Builder builder, Object v) {
+            if (v instanceof byte[] wkb) {
+                ((BlockLoader.BytesRefBuilder) builder).appendBytesRef(new BytesRef(wkb));
+            } else {
+                throw new IllegalArgumentException("Unsupported source type for spatial geometry: " + v.getClass().getSimpleName());
+            }
+        }
+
+        @Override
+        public String toString() {
+            return "BlockSourceReader.Geometries";
+        }
+    }
+
     /**
      * Load {@code double}s from {@code _source}.
      */

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

@@ -357,14 +357,13 @@ protected String contentType() {
         return CONTENT_TYPE;
     }
 
-    public static class GeoPointFieldType extends AbstractGeometryFieldType<GeoPoint> implements GeoShapeQueryable {
+    public static class GeoPointFieldType extends AbstractPointFieldType<GeoPoint> implements GeoShapeQueryable {
         private final TimeSeriesParams.MetricType metricType;
 
         public static final GeoFormatterFactory<GeoPoint> GEO_FORMATTER_FACTORY = new GeoFormatterFactory<>(
             List.of(new SimpleVectorTileFormatter())
         );
 
-        private final GeoPoint nullValue;
         private final FieldValues<GeoPoint> scriptValues;
         private final IndexMode indexMode;
 
@@ -380,8 +379,7 @@ private GeoPointFieldType(
             TimeSeriesParams.MetricType metricType,
             IndexMode indexMode
         ) {
-            super(name, indexed, stored, hasDocValues, parser, meta);
-            this.nullValue = nullValue;
+            super(name, indexed, stored, hasDocValues, parser, nullValue, meta);
             this.scriptValues = scriptValues;
             this.metricType = metricType;
             this.indexMode = indexMode;
@@ -482,19 +480,6 @@ public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext
             throw new IllegalStateException("unknown field data type [" + operation.name() + "]");
         }
 
-        @Override
-        public BlockLoader blockLoader(BlockLoaderContext blContext) {
-            if (hasDocValues()) {
-                return new BlockDocValuesReader.LongsBlockLoader(name());
-            }
-            // TODO: Currently we use longs in the compute engine and render to WKT in ESQL
-            ValueFetcher fetcher = valueFetcher(blContext.sourcePaths(name()), nullValue, GeometryFormatterFactory.WKT);
-            BlockSourceReader.LeafIteratorLookup lookup = isStored() || isIndexed()
-                ? BlockSourceReader.lookupFromFieldNames(blContext.fieldNames(), name())
-                : BlockSourceReader.lookupMatchingAll();
-            return new BlockSourceReader.LongsBlockLoader(fetcher, lookup);
-        }
-
         @Override
         public Query distanceFeatureQuery(Object origin, String pivot, SearchExecutionContext context) {
             failIfNotIndexedNorDocValuesFallback(context);

server/src/test/java/org/elasticsearch/common/geo/SpatialPointTests.java

@@ -0,0 +1,103 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.common.geo;
+
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.lessThan;
+import static org.hamcrest.Matchers.not;
+
+public class SpatialPointTests extends ESTestCase {
+
+    public void testEqualsAndHashcode() {
+        for (int i = 0; i < 100; i++) {
+            SpatialPoint point = randomGeoPoint();
+            GeoPoint geoPoint = new GeoPoint(point);
+            TestPoint testPoint = new TestPoint(point);
+            TestPoint testPoint2 = new TestPoint(point);
+            assertEqualsAndHashcode("Same point", point, point);
+            assertEqualsAndHashcode("Same geo-point", point, geoPoint);
+            assertNotEqualsAndHashcode("Same location, but different class", point, testPoint);
+            assertEqualsAndHashcode("Same location, same class", testPoint, testPoint2);
+        }
+    }
+
+    public void testCompareTo() {
+        for (int i = 0; i < 100; i++) {
+            SpatialPoint point = randomValueOtherThanMany(p -> p.getX() < -170 || p.getX() > 170, ESTestCase::randomGeoPoint);
+            GeoPoint smaller = new GeoPoint(point.getY(), point.getX() - 1);
+            GeoPoint bigger = new GeoPoint(point.getY(), point.getX() + 1);
+            TestPoint testSmaller = new TestPoint(smaller);
+            TestPoint testBigger = new TestPoint(bigger);
+            assertThat(smaller + " smaller than " + point, smaller.compareTo(point), lessThan(0));
+            assertThat(bigger + " bigger than " + point, bigger.compareTo(point), greaterThan(0));
+            assertThat(testSmaller + " smaller than " + testBigger, testSmaller.compareTo(testBigger), lessThan(0));
+            // TestPoint always greater than GeoPoint
+            assertThat(testSmaller + " bigger than " + point, testSmaller.compareTo(point), greaterThan(0));
+            assertThat(testBigger + " bigger than " + point, testBigger.compareTo(point), greaterThan(0));
+        }
+    }
+
+    private void assertEqualsAndHashcode(String message, SpatialPoint a, SpatialPoint b) {
+        assertThat("Equals: " + message, a, equalTo(b));
+        assertThat("Hashcode: " + message, a.hashCode(), equalTo(b.hashCode()));
+        assertThat("Compare: " + message, a.compareTo(b), equalTo(0));
+    }
+
+    private void assertNotEqualsAndHashcode(String message, SpatialPoint a, SpatialPoint b) {
+        assertThat("Equals: " + message, a, not(equalTo(b)));
+        assertThat("Hashcode: " + message, a.hashCode(), not(equalTo(b.hashCode())));
+        assertThat("Compare: " + message, a.compareTo(b), not(equalTo(0)));
+    }
+
+    /**
+     * This test class used to be trivial, when SpatialPoint was a concrete class.
+     * If we ever revert back to a concrete class, we can simplify this test class.
+     * The only requirement is that it extends SpatialPoint, but have a different class name.
+     */
+    private static class TestPoint implements SpatialPoint {
+        double x;
+        double y;
+
+        private TestPoint(SpatialPoint template) {
+            this.x = template.getX();
+            this.y = template.getY();
+        }
+
+        @Override
+        public double getX() {
+            return x;
+        }
+
+        @Override
+        public double getY() {
+            return y;
+        }
+
+        @Override
+        public int hashCode() {
+            return 31 * 31 * getClass().getSimpleName().hashCode() + 31 * Double.hashCode(x) + Double.hashCode(y);
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj) return true;
+            if (obj == null || getClass() != obj.getClass()) return false;
+            SpatialPoint point = (SpatialPoint) obj;
+            return (Double.compare(point.getX(), x) == 0) && Double.compare(point.getY(), y) == 0;
+        }
+
+        @Override
+        public String toString() {
+            return toWKT();
+        }
+    }
+}