From 1c85bdb29f27697c5e2ed75ba9ad11da8c29090e Mon Sep 17 00:00:00 2001 From: Ievgen Degtiarenko Date: Wed, 18 Dec 2024 11:32:54 +0100 Subject: [PATCH] Replace build with scope parameter (#118643) (#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 b8f4677254f7a0cd29c8ded0361f3ab201232cd5) --- .../org/elasticsearch/compute/ann/Fixed.java | 26 +++++++++++++------ .../compute/gen/EvaluatorImplementer.java | 15 ++++++++--- .../function/scalar/convert/FromBase64.java | 3 ++- .../function/scalar/convert/ToBase64.java | 3 ++- .../function/scalar/ip/IpPrefix.java | 3 ++- .../multivalue/MvPSeriesWeightedSum.java | 3 ++- .../scalar/multivalue/MvPercentile.java | 7 ++--- .../function/scalar/string/Concat.java | 3 ++- .../function/scalar/string/Left.java | 5 ++-- .../function/scalar/string/Repeat.java | 9 +++++-- .../function/scalar/string/Right.java | 5 ++-- .../function/scalar/string/Space.java | 3 ++- .../function/scalar/string/Split.java | 5 ++-- 13 files changed, 61 insertions(+), 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 index 62703fa400ff7..1f10abf3b9fb0 100644 --- 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} 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/x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java b/x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java index 5869eff23a9ab..b4a0cf9127f23 100644 --- a/x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java +++ b/x-pack/plugin/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/FromBase64.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/FromBase64.java index 7f9d0d3f2e647..832c511a2dc50 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/FromBase64.java +++ b/x-pack/plugin/esql/src/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 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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToBase64.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToBase64.java index c23cef31f32f5..e78968bb209b6 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToBase64.java +++ b/x-pack/plugin/esql/src/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 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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/IpPrefix.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/IpPrefix.java index 26e75e752f681..5fc61c5c07b58 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/IpPrefix.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java index cf49607893aae..4dd447f938880 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java +++ b/x-pack/plugin/esql/src/main/java/org/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPercentile.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPercentile.java index f3a63c835bd34..4e4aee307f1c7 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPercentile.java +++ b/x-pack/plugin/esql/src/main/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java index 46ecc9e026d3d..eb173029876d3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Left.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Left.java index e7572caafd8f5..0d885e3f3c341 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Left.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Repeat.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Repeat.java index 2cc14399df2ae..e91f03de3dd7e 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Repeat.java +++ b/x-pack/plugin/esql/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,7 +102,7 @@ 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 ) { @@ -109,7 +110,11 @@ static BytesRef processConstantNumber( } @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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Right.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Right.java index b069b984ea81e..e0ebed29cca72 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Right.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Space.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Space.java index 6481ce5764e1f..3b9a466966911 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Space.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Split.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Split.java index b1f5da56d011b..24762122f755b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Split.java +++ b/x-pack/plugin/esql/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);