Remove SpatialPoint from block implementation

Using instead double x and y values (or double[] xValues, yValues). This might improve memory usage and performance, but we still need to verify that.
craigtaverner · Dec 18, 2023 · 0d0d20d · 0d0d20d
1 parent 4b24c08
commit 0d0d20d
Show file tree

Hide file tree

Showing 92 changed files with 1,066 additions and 586 deletions.
diff --git a/...src/main/java/org/elasticsearch/benchmark/compute/operator/MultivalueDedupeBenchmark.java b/...src/main/java/org/elasticsearch/benchmark/compute/operator/MultivalueDedupeBenchmark.java
@@ -167,7 +167,7 @@ public void setup() {
                     Randomness.shuffle(values);
                     builder.beginPositionEntry();
                     for (SpatialPoint v : values) {
-                        builder.appendPoint(v);
+                        builder.appendPoint(v.getX(), v.getY());
                     }
                     builder.endPositionEntry();
                 }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BlockLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/BlockLoader.java
@@ -461,9 +461,9 @@ interface BytesRefBuilder extends Builder {
 
     interface PointBuilder extends Builder {
         /**
-         * Appends a SpatialPoint to the current entry.
+         * Appends a spatial point to the current entry.
          */
-        PointBuilder appendPoint(SpatialPoint value);
+        PointBuilder appendPoint(double x, double y);
     }
 
     interface DoubleBuilder extends Builder {

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BlockSourceReader.java b/server/src/main/java/org/elasticsearch/index/mapper/BlockSourceReader.java
@@ -181,14 +181,13 @@ private static class Points extends BlockSourceReader {
         @Override
         protected void append(BlockLoader.Builder builder, Object v) {
             if (v instanceof SpatialPoint point) {
-                ((BlockLoader.PointBuilder) builder).appendPoint(point);
+                ((BlockLoader.PointBuilder) builder).appendPoint(point.getX(), point.getY());
             } else if (v instanceof String wkt) {
                 try {
                     // TODO: figure out why this is not already happening in the GeoPointFieldMapper
                     Geometry geometry = WellKnownText.fromWKT(GeometryValidator.NOOP, false, wkt);
                     if (geometry instanceof Point point) {
-                        // TODO: perhaps we should not create points for later GC here, and pass in primitives only?
-                        ((BlockLoader.PointBuilder) builder).appendPoint(new SpatialPoint(point.getX(), point.getY()));
+                        ((BlockLoader.PointBuilder) builder).appendPoint(point.getX(), point.getY());
                     } else {
                         throw new IllegalArgumentException("Cannot convert geometry into point:: " + geometry.type());
                     }

diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/TestBlock.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/TestBlock.java
@@ -11,7 +11,6 @@
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.index.SortedDocValues;
 import org.apache.lucene.util.BytesRef;
-import org.elasticsearch.common.geo.SpatialPoint;
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
@@ -62,9 +61,9 @@ public BytesRefsBuilder appendBytesRef(BytesRef value) {
             public BlockLoader.PointBuilder points(int expectedCount) {
                 class PointsBuilder extends TestBlock.Builder implements BlockLoader.PointBuilder {
                     @Override
-                    public PointsBuilder appendPoint(SpatialPoint value) {
-                        add(value.getX());
-                        add(value.getY());
+                    public PointsBuilder appendPoint(double x, double y) {
+                        add(x);
+                        add(y);
                         return this;
                     }
                 }

diff --git a/x-pack/plugin/esql/compute/build.gradle b/x-pack/plugin/esql/compute/build.gradle
@@ -30,30 +30,34 @@ spotless {
   }
 }
 
-def prop(Type, type, TYPE, BYTES, Array) {
+def prop(Type, type, TYPE, BYTES, Array, typeValue, value, typeArrayValues, arrayValues) {
   return [
     "Type" : Type,
     "type" : type,
     "TYPE" : TYPE,
     "BYTES" : BYTES,
     "Array" : Array,
+    "typeValue" : typeValue,
+    "value": value,
+    "typeArrayValues" : typeArrayValues,
+    "arrayValues" : arrayValues,
 
-    "int" : type == "int" ? "true" : "",
-    "long" : type == "long" ? "true" : "",
-    "double" : type == "double" ? "true" : "",
-    "BytesRef" : type == "BytesRef" ? "true" : "",
-    "Point" : type == "SpatialPoint" ? "true" : "",
-    "boolean" : type == "boolean" ? "true" : "",
+    "int" : Type == "Int" ? "true" : "",
+    "long" : Type == "Long" ? "true" : "",
+    "double" : Type == "Double" ? "true" : "",
+    "BytesRef" : Type == "BytesRef" ? "true" : "",
+    "Point" : Type == "Point" ? "true" : "",
+    "boolean" : Type == "Boolean" ? "true" : "",
   ]
 }
 
 tasks.named('stringTemplates').configure {
-  var intProperties      = prop("Int", "int", "INT", "Integer.BYTES", "IntArray")
-  var longProperties     = prop("Long", "long", "LONG", "Long.BYTES", "LongArray")
-  var doubleProperties   = prop("Double", "double", "DOUBLE", "Double.BYTES", "DoubleArray")
-  var bytesRefProperties = prop("BytesRef", "BytesRef", "BYTES_REF", "org.apache.lucene.util.RamUsageEstimator.NUM_BYTES_OBJECT_REF", "")
-  var pointProperties    = prop("Point", "SpatialPoint", "POINT", "16", "ObjectArray<SpatialPoint>")
-  var booleanProperties  = prop("Boolean", "boolean", "BOOLEAN", "Byte.BYTES", "BitArray")
+  var intProperties      = prop("Int", "int", "INT", "Integer.BYTES", "IntArray", "int value", "value", "int[] values", "values")
+  var longProperties     = prop("Long", "long", "LONG", "Long.BYTES", "LongArray", "long value", "value", "long[] values", "values")
+  var doubleProperties   = prop("Double", "double", "DOUBLE", "Double.BYTES", "DoubleArray", "double value", "value", "double[] values", "values")
+  var bytesRefProperties = prop("BytesRef", "BytesRef", "BYTES_REF", "org.apache.lucene.util.RamUsageEstimator.NUM_BYTES_OBJECT_REF", "", "BytesRef value", "value", "BytesRefArray values", "values")
+  var pointProperties    = prop("Point", "double", "POINT", "16", "DoubleArray", "double x, double y", "x, y", "double[] xValues, double[] yValues", "xValues, yValues")
+  var booleanProperties  = prop("Boolean", "boolean", "BOOLEAN", "Byte.BYTES", "BitArray", "boolean value", "value", "boolean[] values", "values")
   // primitive vectors
   File vectorInputFile = new File("${projectDir}/src/main/java/org/elasticsearch/compute/data/X-Vector.java.st")
   template {

diff --git a/.../compute/gen/src/main/java/org/elasticsearch/compute/gen/ConvertEvaluatorImplementer.java b/.../compute/gen/src/main/java/org/elasticsearch/compute/gen/ConvertEvaluatorImplementer.java
@@ -32,6 +32,7 @@
 import static org.elasticsearch.compute.gen.Types.DRIVER_CONTEXT;
 import static org.elasticsearch.compute.gen.Types.EXPRESSION_EVALUATOR;
 import static org.elasticsearch.compute.gen.Types.EXPRESSION_EVALUATOR_FACTORY;
+import static org.elasticsearch.compute.gen.Types.POINT;
 import static org.elasticsearch.compute.gen.Types.SOURCE;
 import static org.elasticsearch.compute.gen.Types.VECTOR;
 import static org.elasticsearch.compute.gen.Types.blockType;
@@ -130,11 +131,19 @@ private MethodSpec evalVector() {
         {
             catchingWarnExceptions(builder, () -> {
                 var constVectType = blockType(resultType);
-                builder.addStatement(
-                    "return driverContext.blockFactory().newConstant$TWith($N, positionCount)",
-                    constVectType,
-                    evalValueCall("vector", "0", scratchPadName)
-                );
+                if (resultType.equals(POINT)) {
+                    builder.addStatement("$T point = $N", resultType, evalValueCall("vector", "0", scratchPadName));
+                    builder.addStatement(
+                        "return driverContext.blockFactory().newConstant$TWith(point.getX(), point.getY(), positionCount)",
+                        constVectType
+                    );
+                } else {
+                    builder.addStatement(
+                        "return driverContext.blockFactory().newConstant$TWith($N, positionCount)",
+                        constVectType,
+                        evalValueCall("vector", "0", scratchPadName)
+                    );
+                }
             }, () -> builder.addStatement("return driverContext.blockFactory().newConstantNullBlock(positionCount)"));
         }
         builder.endControlFlow();
@@ -148,11 +157,14 @@ private MethodSpec evalVector() {
         {
             builder.beginControlFlow("for (int p = 0; p < positionCount; p++)");
             {
-                catchingWarnExceptions(
-                    builder,
-                    () -> builder.addStatement("builder.$L($N)", appendMethod(resultType), evalValueCall("vector", "p", scratchPadName)),
-                    () -> builder.addStatement("builder.appendNull()")
-                );
+                catchingWarnExceptions(builder, () -> {
+                    if (resultType.equals(POINT)) {
+                        builder.addStatement("$T point = $N", resultType, evalValueCall("vector", "p", scratchPadName));
+                        builder.addStatement("builder.$L(point.getX(), point.getY())", appendMethod(resultType));
+                    } else {
+                        builder.addStatement("builder.$L($N)", appendMethod(resultType), evalValueCall("vector", "p", scratchPadName));
+                    }
+                }, () -> builder.addStatement("builder.appendNull()"));
             }
             builder.endControlFlow();
             builder.addStatement("return builder.build()");
@@ -215,7 +227,11 @@ private MethodSpec evalBlock() {
                         builder.addStatement("positionOpened = true");
                     }
                     builder.endControlFlow();
-                    builder.addStatement("builder.$N(value)", appendMethod);
+                    if (resultType.equals(POINT)) {
+                        builder.addStatement("builder.$N(value.getX(), value.getY())", appendMethod);
+                    } else {
+                        builder.addStatement("builder.$N(value)", appendMethod);
+                    }
                     builder.addStatement("valuesAppended = true");
                 }, () -> {});
             }
@@ -265,6 +281,10 @@ private MethodSpec evalValue(boolean forVector) {
         if (argumentType.equals(BYTES_REF)) {
             builder.addParameter(BYTES_REF, "scratchPad");
             builder.addStatement("$T value = container.$N(index, scratchPad)", argumentType, getMethod(argumentType));
+        } else if (argumentType.equals(POINT)) {
+            builder.addStatement("double x = container.getX(index)");
+            builder.addStatement("double y = container.getY(index)");
+            builder.addStatement("$T value = new $T(x, y)", argumentType, argumentType);
         } else {
             builder.addStatement("$T value = container.$N(index)", argumentType, getMethod(argumentType));
         }

diff --git a/...sql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanArrayVector.java b/...sql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanArrayVector.java
@@ -12,7 +12,7 @@
 import java.util.Arrays;
 
 /**
- * Vector implementation that stores an array of boolean values.
+ * Vector implementation that stores an array of Boolean values.
  * This class is generated. Do not edit it.
  */
 public final class BooleanArrayVector extends AbstractVector implements BooleanVector {

diff --git a/...ugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBlock.java b/...ugin/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBlock.java
@@ -15,7 +15,7 @@
 import java.io.IOException;
 
 /**
- * Block that stores boolean values.
+ * Block that stores Boolean values.
  * This class is generated. Do not edit it.
  */
 public sealed interface BooleanBlock extends Block permits BooleanArrayBlock, BooleanVectorBlock, ConstantNullBlock {
@@ -236,14 +236,14 @@ sealed interface Builder extends Block.Builder, BlockLoader.BooleanBuilder permi
         Builder mvOrdering(Block.MvOrdering mvOrdering);
 
         /**
-         * Appends the all values of the given block into a the current position
+         * Appends the all values of the given block into the current position
          * in this builder.
          */
         @Override
         Builder appendAllValuesToCurrentPosition(Block block);
 
         /**
-         * Appends the all values of the given block into a the current position
+         * Appends the all values of the given block into the current position
          * in this builder.
          */
         Builder appendAllValuesToCurrentPosition(BooleanBlock block);

diff --git a/...ql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBlockBuilder.java b/...ql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBlockBuilder.java
@@ -108,8 +108,8 @@ public BooleanBlockBuilder appendAllValuesToCurrentPosition(BooleanBlock block)
             for (int p = 0; p < positionCount; p++) {
                 int count = block.getValueCount(p);
                 int i = block.getFirstValueIndex(p);
-                for (int v = 0; v < count; v++) {
-                    appendBoolean(block.getBoolean(i++));
+                for (int v = 0; v < count; v++, i++) {
+                    appendBoolean(block.getBoolean(i));
                 }
             }
         }
@@ -158,8 +158,8 @@ private void copyFromBlock(BooleanBlock block, int beginInclusive, int endExclus
                 beginPositionEntry();
             }
             int i = block.getFirstValueIndex(p);
-            for (int v = 0; v < count; v++) {
-                appendBoolean(block.getBoolean(i++));
+            for (int v = 0; v < count; v++, i++) {
+                appendBoolean(block.getBoolean(i));
             }
             if (count > 1) {
                 endPositionEntry();
@@ -187,8 +187,8 @@ public BooleanBlock build() {
             if (hasNonNullValue && positionCount == 1 && valueCount == 1) {
                 theBlock = blockFactory.newConstantBooleanBlockWith(values[0], 1, estimatedBytes);
             } else {
-                if (values.length - valueCount > 1024 || valueCount < (values.length / 2)) {
-                    values = Arrays.copyOf(values, valueCount);
+                if (valuesLength() - valueCount > 1024 || valueCount < (valuesLength() / 2)) {
+                    growValuesArray(valueCount);
                 }
                 if (isDense() && singleValued()) {
                     theBlock = blockFactory.newBooleanArrayVector(values, positionCount, estimatedBytes).asBlock();

diff --git a/...gin/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanVector.java b/...gin/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanVector.java
@@ -13,7 +13,7 @@
 import java.io.IOException;
 
 /**
- * Vector that stores boolean values.
+ * Vector that stores Boolean values.
  * This class is generated. Do not edit it.
  */
 public sealed interface BooleanVector extends Vector permits ConstantBooleanVector, BooleanArrayVector, BooleanBigArrayVector,

diff --git a/...l/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanVectorBuilder.java b/...l/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanVectorBuilder.java
@@ -54,8 +54,8 @@ public BooleanVector build() {
         if (valueCount == 1) {
             vector = blockFactory.newConstantBooleanBlockWith(values[0], 1, estimatedBytes).asVector();
         } else {
-            if (values.length - valueCount > 1024 || valueCount < (values.length / 2)) {
-                values = Arrays.copyOf(values, valueCount);
+            if (valuesLength() - valueCount > 1024 || valueCount < (valuesLength() / 2)) {
+                growValuesArray(valueCount);
             }
             vector = blockFactory.newBooleanArrayVector(values, valueCount, estimatedBytes);
         }

diff --git a/...pute/src/main/generated-src/org/elasticsearch/compute/data/BooleanVectorFixedBuilder.java b/...pute/src/main/generated-src/org/elasticsearch/compute/data/BooleanVectorFixedBuilder.java
@@ -46,20 +46,24 @@ private static long ramBytesUsed(int size) {
             );
     }
 
+    private int valuesLength() {
+        return values.length;
+    }
+
     @Override
     public BooleanVector build() {
         if (nextIndex < 0) {
             throw new IllegalStateException("already closed");
         }
-        if (nextIndex != values.length) {
-            throw new IllegalStateException("expected to write [" + values.length + "] entries but wrote [" + nextIndex + "]");
+        if (nextIndex != valuesLength()) {
+            throw new IllegalStateException("expected to write [" + valuesLength() + "] entries but wrote [" + nextIndex + "]");
         }
         nextIndex = -1;
         BooleanVector vector;
-        if (values.length == 1) {
+        if (valuesLength() == 1) {
             vector = blockFactory.newConstantBooleanBlockWith(values[0], 1, preAdjustedBytes).asVector();
         } else {
-            vector = blockFactory.newBooleanArrayVector(values, values.length, preAdjustedBytes);
+            vector = blockFactory.newBooleanArrayVector(values, valuesLength(), preAdjustedBytes);
         }
         assert vector.ramBytesUsed() == preAdjustedBytes : "fixed Builders should estimate the exact ram bytes used";
         return vector;

diff --git a/...gin/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BytesRefBlock.java b/...gin/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BytesRefBlock.java
@@ -241,14 +241,14 @@ sealed interface Builder extends Block.Builder, BlockLoader.BytesRefBuilder perm
         Builder mvOrdering(Block.MvOrdering mvOrdering);
 
         /**
-         * Appends the all values of the given block into a the current position
+         * Appends the all values of the given block into the current position
          * in this builder.
          */
         @Override
         Builder appendAllValuesToCurrentPosition(Block block);
 
         /**
-         * Appends the all values of the given block into a the current position
+         * Appends the all values of the given block into the current position
          * in this builder.
          */
         Builder appendAllValuesToCurrentPosition(BytesRefBlock block);

diff --git a/...l/compute/src/main/generated-src/org/elasticsearch/compute/data/BytesRefBlockBuilder.java b/...l/compute/src/main/generated-src/org/elasticsearch/compute/data/BytesRefBlockBuilder.java
@@ -117,8 +117,8 @@ public BytesRefBlockBuilder appendAllValuesToCurrentPosition(BytesRefBlock block
             for (int p = 0; p < positionCount; p++) {
                 int count = block.getValueCount(p);
                 int i = block.getFirstValueIndex(p);
-                for (int v = 0; v < count; v++) {
-                    appendBytesRef(block.getBytesRef(i++, scratch));
+                for (int v = 0; v < count; v++, i++) {
+                    appendBytesRef(block.getBytesRef(i, scratch));
                 }
             }
         }
@@ -168,8 +168,8 @@ private void copyFromBlock(BytesRefBlock block, int beginInclusive, int endExclu
                 beginPositionEntry();
             }
             int i = block.getFirstValueIndex(p);
-            for (int v = 0; v < count; v++) {
-                appendBytesRef(block.getBytesRef(i++, scratch));
+            for (int v = 0; v < count; v++, i++) {
+                appendBytesRef(block.getBytesRef(i, scratch));
             }
             if (count > 1) {
                 endPositionEntry();