Replace build with scope parameter (elastic#118643) (elastic#118930)

This change replaces the build with scope parameter in order to make it cleaner when parameter is build and how it is used. (cherry picked from commit b8f4677)
idegtiarenko · Dec 18, 2024 · 1c85bdb · 1c85bdb
1 parent 61deb2a
commit 1c85bdb
Show file tree

Hide file tree

Showing 13 changed files with 61 additions and 29 deletions.
diff --git a/x-pack/plugin/esql/compute/ann/src/main/java/org/elasticsearch/compute/ann/Fixed.java b/x-pack/plugin/esql/compute/ann/src/main/java/org/elasticsearch/compute/ann/Fixed.java
@@ -11,7 +11,6 @@
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
-import java.util.function.Function;
 
 /**
  * Used on parameters on methods annotated with {@link Evaluator} to indicate
@@ -27,12 +26,23 @@
     boolean includeInToString() default true;
 
     /**
-     * Should the Evaluator's factory build this per evaluator with a
-     * {@code Function<DriverContext, T>} or just take fixed implementation?
-     * This is typically set to {@code true} to use the {@link Function}
-     * to make "scratch" objects which have to be isolated in a single thread.
-     * This is typically set to {@code false} when the parameter is simply
-     * immutable and can be shared.
+     * Defines the scope of the parameter.
+     * - SINGLETON (default) will build a single instance and share it across all evaluators
+     * - THREAD_LOCAL will build a new instance for each evaluator thread
      */
-    boolean build() default false;
+    Scope scope() default Scope.SINGLETON;
+
+    /**
+     * Defines the parameter scope
+     */
+    enum Scope {
+        /**
+         * Should be used for immutable parameters that can be shared across different threads
+         */
+        SINGLETON,
+        /**
+         * Should be used for mutable or not thread safe parameters
+         */
+        THREAD_LOCAL,
+    }
 }
diff --git a/...in/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java b/...in/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java
@@ -16,6 +16,7 @@
 import com.squareup.javapoet.TypeSpec;
 
 import org.elasticsearch.compute.ann.Fixed;
+import org.elasticsearch.compute.ann.Fixed.Scope;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -725,7 +726,7 @@ public String closeInvocation() {
         }
     }
 
-    private record FixedProcessFunctionArg(TypeName type, String name, boolean includeInToString, boolean build, boolean releasable)
+    private record FixedProcessFunctionArg(TypeName type, String name, boolean includeInToString, Scope scope, boolean releasable)
         implements
             ProcessFunctionArg {
         @Override
@@ -762,12 +763,18 @@ public void implementFactoryCtor(MethodSpec.Builder builder) {
         }
 
         private TypeName factoryFieldType() {
-            return build ? ParameterizedTypeName.get(ClassName.get(Function.class), DRIVER_CONTEXT, type.box()) : type;
+            return switch (scope) {
+                case SINGLETON -> type;
+                case THREAD_LOCAL -> ParameterizedTypeName.get(ClassName.get(Function.class), DRIVER_CONTEXT, type.box());
+            };
         }
 
         @Override
         public String factoryInvocation(MethodSpec.Builder factoryMethodBuilder) {
-            return build ? name + ".apply(context)" : name;
+            return switch (scope) {
+                case SINGLETON -> name;
+                case THREAD_LOCAL -> name + ".apply(context)";
+            };
         }
 
         @Override
@@ -1020,7 +1027,7 @@ private ProcessFunction(
                             type,
                             name,
                             fixed.includeInToString(),
-                            fixed.build(),
+                            fixed.scope(),
                             Types.extendsSuper(types, v.asType(), "org.elasticsearch.core.Releasable")
                         )
                     );

diff --git a/...main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/FromBase64.java b/...main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/FromBase64.java
@@ -30,6 +30,7 @@
 import java.util.Base64;
 import java.util.List;
 
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
 import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
 
@@ -85,7 +86,7 @@ protected NodeInfo<? extends Expression> info() {
     }
 
     @Evaluator()
-    static BytesRef process(BytesRef field, @Fixed(includeInToString = false, build = true) BytesRefBuilder oScratch) {
+    static BytesRef process(BytesRef field, @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRefBuilder oScratch) {
         byte[] bytes = new byte[field.length];
         System.arraycopy(field.bytes, field.offset, bytes, 0, field.length);
         oScratch.grow(field.length);

diff --git a/...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToBase64.java b/...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToBase64.java
@@ -30,6 +30,7 @@
 import java.util.Base64;
 import java.util.List;
 
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
 import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
 
@@ -78,7 +79,7 @@ protected NodeInfo<? extends Expression> info() {
     }
 
     @Evaluator(warnExceptions = { ArithmeticException.class })
-    static BytesRef process(BytesRef field, @Fixed(includeInToString = false, build = true) BytesRefBuilder oScratch) {
+    static BytesRef process(BytesRef field, @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRefBuilder oScratch) {
         int outLength = Math.multiplyExact(4, (Math.addExact(field.length, 2) / 3));
         byte[] bytes = new byte[field.length];
         System.arraycopy(field.bytes, field.offset, bytes, 0, field.length);

diff --git a/...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/IpPrefix.java b/...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/IpPrefix.java
@@ -30,6 +30,7 @@
 import java.util.Arrays;
 import java.util.List;
 
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.THIRD;
@@ -138,7 +139,7 @@ static BytesRef process(
         BytesRef ip,
         int prefixLengthV4,
         int prefixLengthV6,
-        @Fixed(includeInToString = false, build = true) BytesRef scratch
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef scratch
     ) {
         if (prefixLengthV4 < 0 || prefixLengthV4 > 32) {
             throw new IllegalArgumentException("Prefix length v4 must be in range [0, 32], found " + prefixLengthV4);

diff --git a/.../elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java b/.../elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java
@@ -33,6 +33,7 @@
 import java.util.Arrays;
 import java.util.List;
 
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isFoldable;
@@ -144,7 +145,7 @@ static void process(
         DoubleBlock.Builder builder,
         int position,
         DoubleBlock block,
-        @Fixed(includeInToString = false, build = true) CompensatedSum sum,
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) CompensatedSum sum,
         @Fixed double p
     ) {
         sum.reset(0, 0);

diff --git a/...java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPercentile.java b/...java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPercentile.java
@@ -35,6 +35,7 @@
 import java.util.Arrays;
 import java.util.List;
 
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
@@ -167,7 +168,7 @@ static void process(
         int position,
         DoubleBlock values,
         double percentile,
-        @Fixed(includeInToString = false, build = true) DoubleSortingScratch scratch
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) DoubleSortingScratch scratch
     ) {
         int valueCount = values.getValueCount(position);
         int firstValueIndex = values.getFirstValueIndex(position);
@@ -190,7 +191,7 @@ static void process(
         int position,
         IntBlock values,
         double percentile,
-        @Fixed(includeInToString = false, build = true) IntSortingScratch scratch
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) IntSortingScratch scratch
     ) {
         int valueCount = values.getValueCount(position);
         int firstValueIndex = values.getFirstValueIndex(position);
@@ -213,7 +214,7 @@ static void process(
         int position,
         LongBlock values,
         double percentile,
-        @Fixed(includeInToString = false, build = true) LongSortingScratch scratch
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) LongSortingScratch scratch
     ) {
         int valueCount = values.getValueCount(position);
         int firstValueIndex = values.getFirstValueIndex(position);

diff --git a/.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java b/.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java
@@ -32,6 +32,7 @@
 import java.util.stream.Stream;
 
 import static org.elasticsearch.common.unit.ByteSizeUnit.MB;
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.DEFAULT;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
 
@@ -111,7 +112,7 @@ public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) {
     }
 
     @Evaluator
-    static BytesRef process(@Fixed(includeInToString = false, build = true) BreakingBytesRefBuilder scratch, BytesRef[] values) {
+    static BytesRef process(@Fixed(includeInToString = false, scope = THREAD_LOCAL) BreakingBytesRefBuilder scratch, BytesRef[] values) {
         scratch.grow(checkedTotalLength(values));
         scratch.clear();
         for (int i = 0; i < values.length; i++) {

diff --git a/...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Left.java b/...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Left.java
@@ -30,6 +30,7 @@
 import java.util.Arrays;
 import java.util.List;
 
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
@@ -77,8 +78,8 @@ public String getWriteableName() {
 
     @Evaluator
     static BytesRef process(
-        @Fixed(includeInToString = false, build = true) BytesRef out,
-        @Fixed(includeInToString = false, build = true) UnicodeUtil.UTF8CodePoint cp,
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef out,
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) UnicodeUtil.UTF8CodePoint cp,
         BytesRef str,
         int length
     ) {

diff --git a/.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Repeat.java b/.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Repeat.java
@@ -31,6 +31,7 @@
 import java.util.List;
 
 import static org.elasticsearch.common.unit.ByteSizeUnit.MB;
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
@@ -101,15 +102,19 @@ public boolean foldable() {
 
     @Evaluator(extraName = "Constant", warnExceptions = { IllegalArgumentException.class })
     static BytesRef processConstantNumber(
-        @Fixed(includeInToString = false, build = true) BreakingBytesRefBuilder scratch,
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) BreakingBytesRefBuilder scratch,
         BytesRef str,
         @Fixed int number
     ) {
         return processInner(scratch, str, number);
     }
 
     @Evaluator(warnExceptions = { IllegalArgumentException.class })
-    static BytesRef process(@Fixed(includeInToString = false, build = true) BreakingBytesRefBuilder scratch, BytesRef str, int number) {
+    static BytesRef process(
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) BreakingBytesRefBuilder scratch,
+        BytesRef str,
+        int number
+    ) {
         if (number < 0) {
             throw new IllegalArgumentException("Number parameter cannot be negative, found [" + number + "]");
         }

diff --git a/...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Right.java b/...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Right.java
@@ -30,6 +30,7 @@
 import java.util.Arrays;
 import java.util.List;
 
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
@@ -77,8 +78,8 @@ public String getWriteableName() {
 
     @Evaluator
     static BytesRef process(
-        @Fixed(includeInToString = false, build = true) BytesRef out,
-        @Fixed(includeInToString = false, build = true) UnicodeUtil.UTF8CodePoint cp,
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef out,
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) UnicodeUtil.UTF8CodePoint cp,
         BytesRef str,
         int length
     ) {

diff --git a/...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Space.java b/...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Space.java
@@ -31,6 +31,7 @@
 import java.util.List;
 
 import static org.elasticsearch.common.unit.ByteSizeUnit.MB;
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.DEFAULT;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
 import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
@@ -82,7 +83,7 @@ protected TypeResolution resolveType() {
     }
 
     @Evaluator(warnExceptions = { IllegalArgumentException.class })
-    static BytesRef process(@Fixed(includeInToString = false, build = true) BreakingBytesRefBuilder scratch, int number) {
+    static BytesRef process(@Fixed(includeInToString = false, scope = THREAD_LOCAL) BreakingBytesRefBuilder scratch, int number) {
         checkNumber(number);
         scratch.grow(number);
         scratch.setLength(number);

diff --git a/...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Split.java b/...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Split.java
@@ -29,6 +29,7 @@
 
 import java.io.IOException;
 
+import static org.elasticsearch.compute.ann.Fixed.Scope.THREAD_LOCAL;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.expression.EsqlTypeResolutions.isStringAndExact;
@@ -110,7 +111,7 @@ static void process(
         BytesRefBlock.Builder builder,
         BytesRef str,
         @Fixed byte delim,
-        @Fixed(includeInToString = false, build = true) BytesRef scratch
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef scratch
     ) {
         scratch.bytes = str.bytes;
         scratch.offset = str.offset;
@@ -140,7 +141,7 @@ static void process(
         BytesRefBlock.Builder builder,
         BytesRef str,
         BytesRef delim,
-        @Fixed(includeInToString = false, build = true) BytesRef scratch
+        @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef scratch
     ) {
         checkDelimiter(delim);
         process(builder, str, delim.bytes[delim.offset], scratch);