Skip to content

Commit 67a0e48

Browse files
dnhatndnhatn
committed
feedback
1 parent 5ad3b19 commit 67a0e48

File tree

5 files changed

+61
-140
lines changed

5 files changed

+61
-140
lines changed

‎x-pack/plugin/esql/compute/ann/src/main/java/org/elasticsearch/compute/ann/Evaluator.java

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,10 +42,4 @@
4242
* into a warning and turn into a null value.
4343
*/
4444
Class<? extends Exception>[] warnExceptions() default {};
45-
46-
/**
47-
* The estimated execution cost of evaluating one row of this evaluator. The Driver periodically checks for cancellation or early
48-
* termination after the accumulated cost exceeds the threshold (2048). Increase this estimate for expensive evaluators.
49-
*/
50-
int executionCost() default 0;
5145
}

‎x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java

Lines changed: 60 additions & 120 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
import static org.elasticsearch.compute.gen.Types.BOOLEAN_BLOCK;
4141
import static org.elasticsearch.compute.gen.Types.BYTES_REF;
4242
import static org.elasticsearch.compute.gen.Types.BYTES_REF_BLOCK;
43-
import static org.elasticsearch.compute.gen.Types.BYTES_REF_VECTOR;
4443
import static org.elasticsearch.compute.gen.Types.DOUBLE_BLOCK;
4544
import static org.elasticsearch.compute.gen.Types.DRIVER_CONTEXT;
4645
import static org.elasticsearch.compute.gen.Types.EXPRESSION_EVALUATOR;
@@ -62,23 +61,18 @@ public class EvaluatorImplementer {
6261
private final TypeElement declarationType;
6362
private final ProcessFunction processFunction;
6463
private final ClassName implementation;
65-
private final int executionCost;
6664
private final boolean processOutputsMultivalued;
6765

6866
public EvaluatorImplementer(
6967
Elements elements,
7068
javax.lang.model.util.Types types,
7169
ExecutableElement processFunction,
7270
String extraName,
73-
int executionCost,
7471
List<TypeMirror> warnExceptions
7572
) {
7673
this.declarationType = (TypeElement) processFunction.getEnclosingElement();
7774
this.processFunction = new ProcessFunction(elements, types, processFunction, warnExceptions);
78-
this.executionCost = executionCost;
79-
if (executionCost < 0) {
80-
throw new IllegalArgumentException("executionCost must be non-negative; got " + executionCost);
81-
}
75+
8276
this.implementation = ClassName.get(
8377
elements.getPackageOf(declarationType).toString(),
8478
declarationType.getSimpleName() + extraName + "Evaluator"
@@ -198,132 +192,78 @@ private MethodSpec realEval(boolean blockStyle) {
198192
});
199193

200194
processFunction.args.stream().forEach(a -> a.createScratch(builder));
201-
if (vectorize) {
202-
realEvalWithVectorizedStyle(builder, resultDataType);
203-
} else {
204-
if (executionCost > 0) {
205-
builder.addStatement("int accumulatedCost = 0");
206-
}
207-
builder.beginControlFlow("position: for (int p = 0; p < positionCount; p++)");
208-
{
209-
if (blockStyle) {
210-
if (processOutputsMultivalued == false) {
211-
processFunction.args.stream().forEach(a -> a.skipNull(builder));
212-
} else {
213-
builder.addStatement("boolean allBlocksAreNulls = true");
214-
// allow block type inputs to be null
215-
processFunction.args.stream().forEach(a -> {
216-
if (a instanceof StandardProcessFunctionArg as) {
217-
as.skipNull(builder);
218-
} else if (a instanceof BlockProcessFunctionArg ab) {
219-
builder.beginControlFlow("if (!$N.isNull(p))", ab.paramName(blockStyle));
220-
{
221-
builder.addStatement("allBlocksAreNulls = false");
222-
}
223-
builder.endControlFlow();
224-
}
225-
});
226195

227-
builder.beginControlFlow("if (allBlocksAreNulls)");
228-
{
229-
builder.addStatement("result.appendNull()");
230-
builder.addStatement("continue position");
196+
builder.beginControlFlow("position: for (int p = 0; p < positionCount; p++)");
197+
{
198+
if (blockStyle) {
199+
if (processOutputsMultivalued == false) {
200+
processFunction.args.stream().forEach(a -> a.skipNull(builder));
201+
} else {
202+
builder.addStatement("boolean allBlocksAreNulls = true");
203+
// allow block type inputs to be null
204+
processFunction.args.stream().forEach(a -> {
205+
if (a instanceof StandardProcessFunctionArg as) {
206+
as.skipNull(builder);
207+
} else if (a instanceof BlockProcessFunctionArg ab) {
208+
builder.beginControlFlow("if (!$N.isNull(p))", ab.paramName(blockStyle));
209+
{
210+
builder.addStatement("allBlocksAreNulls = false");
211+
}
212+
builder.endControlFlow();
231213
}
232-
builder.endControlFlow();
233-
}
234-
}
235-
processFunction.args.stream().forEach(a -> a.unpackValues(builder, blockStyle));
236-
237-
StringBuilder pattern = new StringBuilder();
238-
List<Object> args = new ArrayList<>();
239-
pattern.append(processOutputsMultivalued ? "$T.$N(result, p, " : "$T.$N(");
240-
args.add(declarationType);
241-
args.add(processFunction.function.getSimpleName());
242-
processFunction.args.stream().forEach(a -> {
243-
if (args.size() > 2) {
244-
pattern.append(", ");
214+
});
215+
216+
builder.beginControlFlow("if (allBlocksAreNulls)");
217+
{
218+
builder.addStatement("result.appendNull()");
219+
builder.addStatement("continue position");
245220
}
246-
a.buildInvocation(pattern, args, blockStyle);
247-
});
248-
pattern.append(")");
249-
String builtPattern;
250-
if (processFunction.builderArg == null) {
251-
builtPattern = "result.$L(" + pattern + ")";
252-
args.add(0, appendMethod(resultDataType));
253-
} else {
254-
builtPattern = pattern.toString();
255-
}
256-
if (processFunction.warnExceptions.isEmpty() == false) {
257-
builder.beginControlFlow("try");
258-
}
259-
if (executionCost > 0) {
260-
addEarlyTerminationCheck(builder, executionCost);
261-
}
262-
builder.addStatement(builtPattern, args.toArray());
263-
264-
if (processFunction.warnExceptions.isEmpty() == false) {
265-
String catchPattern = "catch ("
266-
+ processFunction.warnExceptions.stream().map(m -> "$T").collect(Collectors.joining(" | "))
267-
+ " e)";
268-
builder.nextControlFlow(catchPattern, processFunction.warnExceptions.stream().map(m -> TypeName.get(m)).toArray());
269-
builder.addStatement("warnings().registerException(e)");
270-
builder.addStatement("result.appendNull()");
271221
builder.endControlFlow();
272222
}
273223
}
274-
builder.endControlFlow();
275-
}
276-
builder.addStatement("return result.build()");
277-
}
278-
builder.endControlFlow();
224+
processFunction.args.stream().forEach(a -> a.unpackValues(builder, blockStyle));
225+
226+
StringBuilder pattern = new StringBuilder();
227+
List<Object> args = new ArrayList<>();
228+
pattern.append(processOutputsMultivalued ? "$T.$N(result, p, " : "$T.$N(");
229+
args.add(declarationType);
230+
args.add(processFunction.function.getSimpleName());
231+
processFunction.args.stream().forEach(a -> {
232+
if (args.size() > 2) {
233+
pattern.append(", ");
234+
}
235+
a.buildInvocation(pattern, args, blockStyle);
236+
});
237+
pattern.append(")");
238+
String builtPattern;
239+
if (processFunction.builderArg == null) {
240+
builtPattern = vectorize ? "result.$L(p, " + pattern + ")" : "result.$L(" + pattern + ")";
241+
args.add(0, appendMethod(resultDataType));
242+
} else {
243+
builtPattern = pattern.toString();
244+
}
245+
if (processFunction.warnExceptions.isEmpty() == false) {
246+
builder.beginControlFlow("try");
247+
}
279248

280-
return builder.build();
281-
}
249+
builder.addStatement(builtPattern, args.toArray());
282250

283-
private void realEvalWithVectorizedStyle(MethodSpec.Builder builder, ClassName resultDataType) {
284-
boolean checkEarlyTerminationPerRow = executionCost > 0
285-
&& processFunction.args.stream().anyMatch(a -> a.dataType(false).equals(BYTES_REF_VECTOR));
286-
if (checkEarlyTerminationPerRow) {
287-
builder.addStatement("int accumulatedCost = 0");
288-
}
289-
builder.beginControlFlow("position: for (int p = 0; p < positionCount; p++)");
290-
{
291-
processFunction.args.forEach(a -> a.unpackValues(builder, false));
292-
StringBuilder pattern = new StringBuilder();
293-
List<Object> args = new ArrayList<>();
294-
pattern.append("$T.$N(");
295-
args.add(declarationType);
296-
args.add(processFunction.function.getSimpleName());
297-
processFunction.args.forEach(a -> {
298-
if (args.size() > 2) {
299-
pattern.append(", ");
251+
if (processFunction.warnExceptions.isEmpty() == false) {
252+
String catchPattern = "catch ("
253+
+ processFunction.warnExceptions.stream().map(m -> "$T").collect(Collectors.joining(" | "))
254+
+ " e)";
255+
builder.nextControlFlow(catchPattern, processFunction.warnExceptions.stream().map(m -> TypeName.get(m)).toArray());
256+
builder.addStatement("warnings().registerException(e)");
257+
builder.addStatement("result.appendNull()");
258+
builder.endControlFlow();
300259
}
301-
a.buildInvocation(pattern, args, false);
302-
});
303-
pattern.append(")");
304-
String builtPattern;
305-
if (processFunction.builderArg == null) {
306-
builtPattern = "result.$L(p, " + pattern + ")";
307-
args.add(0, appendMethod(resultDataType));
308-
} else {
309-
builtPattern = pattern.toString();
310-
}
311-
if (checkEarlyTerminationPerRow) {
312-
addEarlyTerminationCheck(builder, executionCost);
313260
}
314-
builder.addStatement(builtPattern, args.toArray());
261+
builder.endControlFlow();
262+
builder.addStatement("return result.build()");
315263
}
316264
builder.endControlFlow();
317-
}
318265

319-
static void addEarlyTerminationCheck(MethodSpec.Builder builder, int executionCost) {
320-
builder.addStatement("accumulatedCost += $L", executionCost);
321-
builder.beginControlFlow("if (accumulatedCost >= DriverContext.CHECK_FOR_EARLY_TERMINATION_COST_THRESHOLD)");
322-
{
323-
builder.addStatement("accumulatedCost = 0");
324-
builder.addStatement("driverContext.checkForEarlyTermination()");
325-
}
326-
builder.endControlFlow();
266+
return builder.build();
327267
}
328268

329269
private static void skipNull(MethodSpec.Builder builder, String value) {

‎x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorProcessor.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@ public boolean process(Set<? extends TypeElement> set, RoundEnvironment roundEnv
8282
env.getTypeUtils(),
8383
(ExecutableElement) evaluatorMethod,
8484
evaluatorAnn.extraName(),
85-
evaluatorAnn.executionCost(),
8685
warnExceptionsTypes
8786
).sourceFile(),
8887
env

‎x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/string/AutomataMatchEvaluator.java

Lines changed: 0 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/AutomataMatch.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public static EvalOperator.ExpressionEvaluator.Factory toEvaluator(
4141
return new AutomataMatchEvaluator.Factory(source, field, run, toDot(automaton));
4242
}
4343

44-
@Evaluator(executionCost = 50)
44+
@Evaluator
4545
static boolean process(BytesRef input, @Fixed(includeInToString = false) ByteRunAutomaton automaton, @Fixed String pattern) {
4646
if (input == null) {
4747
return false;

0 commit comments

Comments
 (0)