Skip to content

Commit c7473ad

Browse files
nik9000nik9000
authored
ESQL: Delay construction of warnings (#114368) (#114459)
Delay construction of `Warnings` until they are needed to save memory when evaluating many many many expressions. Most expressions won't use warnings at all and there isn't any need to make registering warnings super duper fast. So let's make the construction lazy to save a little memory. It's like 200 bytes per expression which isn't much, but it's possible to have thousands of expressions in a single query. Abusive, but possible. This also consolidates all `Warnings` usages to a single `Warnings` class. We had two. We don't need two.
1 parent e7b16cc commit c7473ad

File tree

235 files changed

+4278
-1257
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

235 files changed

+4278
-1257
lines changed

‎docs/changelog/114368.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 114368
2+
summary: "ESQL: Delay construction of warnings"
3+
area: EQL
4+
type: enhancement
5+
issues: []

‎test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -409,9 +409,6 @@ protected void doRun() throws Exception {
409409
TimeValue elapsed = TimeValue.timeValueNanos(System.nanoTime() - startedTimeInNanos);
410410
logger.info("--> test {} triggering OOM after {}", getTestName(), elapsed);
411411
Request triggerOOM = new Request("POST", "/_trigger_out_of_memory");
412-
RequestConfig requestConfig = RequestConfig.custom()
413-
.setSocketTimeout(Math.toIntExact(TimeValue.timeValueMinutes(2).millis()))
414-
.build();
415412
client().performRequest(triggerOOM);
416413
}
417414
}, TimeValue.timeValueMinutes(5), testThreadPool.executor(ThreadPool.Names.GENERIC));

‎x-pack/plugin/esql/build.gradle‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ tasks.named('stringTemplates').configure {
317317
}
318318

319319

320-
File inInputFile = file("src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/InEvaluator.java.st")
320+
File inInputFile = file("src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/X-InEvaluator.java.st")
321321
template {
322322
it.properties = booleanProperties
323323
it.inputFile = inInputFile

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@
4545
import static org.elasticsearch.compute.gen.Types.BYTES_REF;
4646
import static org.elasticsearch.compute.gen.Types.BYTES_REF_BLOCK;
4747
import static org.elasticsearch.compute.gen.Types.BYTES_REF_VECTOR;
48-
import static org.elasticsearch.compute.gen.Types.COMPUTE_WARNINGS;
4948
import static org.elasticsearch.compute.gen.Types.DOUBLE_BLOCK;
5049
import static org.elasticsearch.compute.gen.Types.DOUBLE_VECTOR;
5150
import static org.elasticsearch.compute.gen.Types.DRIVER_CONTEXT;
@@ -60,6 +59,7 @@
6059
import static org.elasticsearch.compute.gen.Types.LONG_BLOCK;
6160
import static org.elasticsearch.compute.gen.Types.LONG_VECTOR;
6261
import static org.elasticsearch.compute.gen.Types.PAGE;
62+
import static org.elasticsearch.compute.gen.Types.WARNINGS;
6363
import static org.elasticsearch.compute.gen.Types.blockType;
6464
import static org.elasticsearch.compute.gen.Types.vectorType;
6565

@@ -224,7 +224,7 @@ private TypeSpec type() {
224224
);
225225

226226
if (warnExceptions.isEmpty() == false) {
227-
builder.addField(COMPUTE_WARNINGS, "warnings", Modifier.PRIVATE, Modifier.FINAL);
227+
builder.addField(WARNINGS, "warnings", Modifier.PRIVATE, Modifier.FINAL);
228228
}
229229

230230
builder.addField(DRIVER_CONTEXT, "driverContext", Modifier.PRIVATE, Modifier.FINAL);
@@ -256,7 +256,7 @@ private MethodSpec create() {
256256
MethodSpec.Builder builder = MethodSpec.methodBuilder("create");
257257
builder.addModifiers(Modifier.PUBLIC, Modifier.STATIC).returns(implementation);
258258
if (warnExceptions.isEmpty() == false) {
259-
builder.addParameter(COMPUTE_WARNINGS, "warnings");
259+
builder.addParameter(WARNINGS, "warnings");
260260
}
261261
builder.addParameter(DRIVER_CONTEXT, "driverContext");
262262
builder.addParameter(LIST_INTEGER, "channels");
@@ -312,7 +312,7 @@ private CodeBlock initInterState() {
312312
private MethodSpec ctor() {
313313
MethodSpec.Builder builder = MethodSpec.constructorBuilder().addModifiers(Modifier.PUBLIC);
314314
if (warnExceptions.isEmpty() == false) {
315-
builder.addParameter(COMPUTE_WARNINGS, "warnings");
315+
builder.addParameter(WARNINGS, "warnings");
316316
}
317317
builder.addParameter(DRIVER_CONTEXT, "driverContext");
318318
builder.addParameter(LIST_INTEGER, "channels");

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

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,12 @@ private TypeSpec type() {
9797
builder.addSuperinterface(EXPRESSION_EVALUATOR);
9898
builder.addType(factory());
9999

100-
builder.addField(WARNINGS, "warnings", Modifier.PRIVATE, Modifier.FINAL);
100+
builder.addField(SOURCE, "source", Modifier.PRIVATE, Modifier.FINAL);
101101
processFunction.args.stream().forEach(a -> a.declareField(builder));
102102
builder.addField(DRIVER_CONTEXT, "driverContext", Modifier.PRIVATE, Modifier.FINAL);
103103

104+
builder.addField(WARNINGS, "warnings", Modifier.PRIVATE);
105+
104106
builder.addMethod(ctor());
105107
builder.addMethod(eval());
106108

@@ -116,17 +118,17 @@ private TypeSpec type() {
116118
}
117119
builder.addMethod(toStringMethod());
118120
builder.addMethod(close());
121+
builder.addMethod(warnings());
119122
return builder.build();
120123
}
121124

122125
private MethodSpec ctor() {
123126
MethodSpec.Builder builder = MethodSpec.constructorBuilder().addModifiers(Modifier.PUBLIC);
124127
builder.addParameter(SOURCE, "source");
128+
builder.addStatement("this.source = source");
125129
processFunction.args.stream().forEach(a -> a.implementCtor(builder));
126-
127130
builder.addParameter(DRIVER_CONTEXT, "driverContext");
128131
builder.addStatement("this.driverContext = driverContext");
129-
builder.addStatement("this.warnings = Warnings.createWarnings(driverContext.warningsMode(), source)");
130132
return builder.build();
131133
}
132134

@@ -250,7 +252,7 @@ private MethodSpec realEval(boolean blockStyle) {
250252
+ processFunction.warnExceptions.stream().map(m -> "$T").collect(Collectors.joining(" | "))
251253
+ " e)";
252254
builder.nextControlFlow(catchPattern, processFunction.warnExceptions.stream().map(m -> TypeName.get(m)).toArray());
253-
builder.addStatement("warnings.registerException(e)");
255+
builder.addStatement("warnings().registerException(e)");
254256
builder.addStatement("result.appendNull()");
255257
builder.endControlFlow();
256258
}
@@ -276,7 +278,7 @@ private static void skipNull(MethodSpec.Builder builder, String value) {
276278
{
277279
builder.addStatement(
278280
// TODO: reflection on SingleValueQuery.MULTI_VALUE_WARNING?
279-
"warnings.registerException(new $T(\"single-value function encountered multi-value\"))",
281+
"warnings().registerException(new $T(\"single-value function encountered multi-value\"))",
280282
IllegalArgumentException.class
281283
);
282284
}
@@ -316,6 +318,22 @@ private MethodSpec close() {
316318
return builder.build();
317319
}
318320

321+
static MethodSpec warnings() {
322+
MethodSpec.Builder builder = MethodSpec.methodBuilder("warnings");
323+
builder.addModifiers(Modifier.PRIVATE).returns(WARNINGS);
324+
builder.beginControlFlow("if (warnings == null)");
325+
builder.addStatement("""
326+
this.warnings = Warnings.createWarnings(
327+
driverContext.warningsMode(),
328+
source.source().getLineNumber(),
329+
source.source().getColumnNumber(),
330+
source.text()
331+
)""");
332+
builder.endControlFlow();
333+
builder.addStatement("return warnings");
334+
return builder.build();
335+
}
336+
319337
private TypeSpec factory() {
320338
TypeSpec.Builder builder = TypeSpec.classBuilder("Factory");
321339
builder.addSuperinterface(EXPRESSION_EVALUATOR_FACTORY);

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
import static org.elasticsearch.compute.gen.Types.BIG_ARRAYS;
4141
import static org.elasticsearch.compute.gen.Types.BLOCK_ARRAY;
4242
import static org.elasticsearch.compute.gen.Types.BYTES_REF;
43-
import static org.elasticsearch.compute.gen.Types.COMPUTE_WARNINGS;
4443
import static org.elasticsearch.compute.gen.Types.DRIVER_CONTEXT;
4544
import static org.elasticsearch.compute.gen.Types.ELEMENT_TYPE;
4645
import static org.elasticsearch.compute.gen.Types.GROUPING_AGGREGATOR_FUNCTION;
@@ -54,6 +53,7 @@
5453
import static org.elasticsearch.compute.gen.Types.LONG_VECTOR;
5554
import static org.elasticsearch.compute.gen.Types.PAGE;
5655
import static org.elasticsearch.compute.gen.Types.SEEN_GROUP_IDS;
56+
import static org.elasticsearch.compute.gen.Types.WARNINGS;
5757

5858
/**
5959
* Implements "GroupingAggregationFunction" from a class containing static methods
@@ -164,7 +164,7 @@ private TypeSpec type() {
164164
);
165165
builder.addField(stateType, "state", Modifier.PRIVATE, Modifier.FINAL);
166166
if (warnExceptions.isEmpty() == false) {
167-
builder.addField(COMPUTE_WARNINGS, "warnings", Modifier.PRIVATE, Modifier.FINAL);
167+
builder.addField(WARNINGS, "warnings", Modifier.PRIVATE, Modifier.FINAL);
168168
}
169169
builder.addField(LIST_INTEGER, "channels", Modifier.PRIVATE, Modifier.FINAL);
170170
builder.addField(DRIVER_CONTEXT, "driverContext", Modifier.PRIVATE, Modifier.FINAL);
@@ -196,7 +196,7 @@ private MethodSpec create() {
196196
MethodSpec.Builder builder = MethodSpec.methodBuilder("create");
197197
builder.addModifiers(Modifier.PUBLIC, Modifier.STATIC).returns(implementation);
198198
if (warnExceptions.isEmpty() == false) {
199-
builder.addParameter(COMPUTE_WARNINGS, "warnings");
199+
builder.addParameter(WARNINGS, "warnings");
200200
}
201201
builder.addParameter(LIST_INTEGER, "channels");
202202
builder.addParameter(DRIVER_CONTEXT, "driverContext");
@@ -258,7 +258,7 @@ private CodeBlock initInterState() {
258258
private MethodSpec ctor() {
259259
MethodSpec.Builder builder = MethodSpec.constructorBuilder().addModifiers(Modifier.PUBLIC);
260260
if (warnExceptions.isEmpty() == false) {
261-
builder.addParameter(COMPUTE_WARNINGS, "warnings");
261+
builder.addParameter(WARNINGS, "warnings");
262262
}
263263
builder.addParameter(LIST_INTEGER, "channels");
264264
builder.addParameter(stateType, "state");

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,8 @@ private TypeSpec type() {
134134
builder.superclass(ABSTRACT_MULTIVALUE_FUNCTION_EVALUATOR);
135135
} else {
136136
builder.superclass(ABSTRACT_NULLABLE_MULTIVALUE_FUNCTION_EVALUATOR);
137-
138-
builder.addField(WARNINGS, "warnings", Modifier.PRIVATE, Modifier.FINAL);
137+
builder.addField(SOURCE, "source", Modifier.PRIVATE, Modifier.FINAL);
138+
builder.addField(WARNINGS, "warnings", Modifier.PRIVATE);
139139
}
140140

141141
builder.addMethod(ctor());
@@ -156,6 +156,9 @@ private TypeSpec type() {
156156
}
157157

158158
builder.addType(factory());
159+
if (warnExceptions.isEmpty() == false) {
160+
builder.addMethod(EvaluatorImplementer.warnings());
161+
}
159162
return builder.build();
160163
}
161164

@@ -165,10 +168,10 @@ private MethodSpec ctor() {
165168
builder.addParameter(SOURCE, "source");
166169
}
167170
builder.addParameter(EXPRESSION_EVALUATOR, "field");
168-
builder.addStatement("super(driverContext, field)");
169171
builder.addParameter(DRIVER_CONTEXT, "driverContext");
172+
builder.addStatement("super(driverContext, field)");
170173
if (warnExceptions.isEmpty() == false) {
171-
builder.addStatement("this.warnings = Warnings.createWarnings(driverContext.warningsMode(), source)");
174+
builder.addStatement("this.source = source");
172175
}
173176
return builder.build();
174177
}
@@ -241,7 +244,7 @@ private MethodSpec evalShell(
241244
body.accept(builder);
242245
String catchPattern = "catch (" + warnExceptions.stream().map(m -> "$T").collect(Collectors.joining(" | ")) + " e)";
243246
builder.nextControlFlow(catchPattern, warnExceptions.stream().map(TypeName::get).toArray());
244-
builder.addStatement("warnings.registerException(e)");
247+
builder.addStatement("warnings().registerException(e)");
245248
builder.addStatement("builder.appendNull()");
246249
builder.endControlFlow();
247250
} else {

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

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -129,12 +129,7 @@ public class Types {
129129
"AbstractEvaluator"
130130
);
131131

132-
static final ClassName WARNINGS = ClassName.get("org.elasticsearch.xpack.esql.expression.function", "Warnings");
133-
/**
134-
* Warnings class used in compute module.
135-
* It uses no external dependencies (Like Warnings and Source).
136-
*/
137-
static final ClassName COMPUTE_WARNINGS = ClassName.get("org.elasticsearch.compute.aggregation", "Warnings");
132+
static final ClassName WARNINGS = ClassName.get("org.elasticsearch.compute.operator", "Warnings");
138133

139134
static final ClassName SOURCE = ClassName.get("org.elasticsearch.xpack.esql.core.tree", "Source");
140135

‎x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/Warnings.java‎ renamed to ‎x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/Warnings.java‎

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@
55
* 2.0.
66
*/
77

8-
package org.elasticsearch.compute.aggregation;
9-
10-
import org.elasticsearch.compute.operator.DriverContext;
8+
package org.elasticsearch.compute.operator;
119

1210
import static org.elasticsearch.common.logging.HeaderWarning.addWarning;
1311
import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
@@ -18,12 +16,7 @@
1816
public class Warnings {
1917
static final int MAX_ADDED_WARNINGS = 20;
2018

21-
private final String location;
22-
private final String first;
23-
24-
private int addedWarnings;
25-
26-
public static final Warnings NOOP_WARNINGS = new Warnings(-1, -2, "") {
19+
public static final Warnings NOOP_WARNINGS = new Warnings(-1, -2, "", "") {
2720
@Override
2821
public void registerException(Exception exception) {
2922
// this space intentionally left blank
@@ -39,9 +32,37 @@ public void registerException(Exception exception) {
3932
* @return A warnings collector object
4033
*/
4134
public static Warnings createWarnings(DriverContext.WarningsMode warningsMode, int lineNumber, int columnNumber, String sourceText) {
35+
return createWarnings(warningsMode, lineNumber, columnNumber, sourceText, "treating result as null");
36+
}
37+
38+
/**
39+
* Create a new warnings object based on the given mode which warns that
40+
* it treats the result as {@code false}.
41+
* @param warningsMode The warnings collection strategy to use
42+
* @param lineNumber The line number of the source text. Same as `source.getLineNumber()`
43+
* @param columnNumber The column number of the source text. Same as `source.getColumnNumber()`
44+
* @param sourceText The source text that caused the warning. Same as `source.text()`
45+
* @return A warnings collector object
46+
*/
47+
public static Warnings createWarningsTreatedAsFalse(
48+
DriverContext.WarningsMode warningsMode,
49+
int lineNumber,
50+
int columnNumber,
51+
String sourceText
52+
) {
53+
return createWarnings(warningsMode, lineNumber, columnNumber, sourceText, "treating result as false");
54+
}
55+
56+
private static Warnings createWarnings(
57+
DriverContext.WarningsMode warningsMode,
58+
int lineNumber,
59+
int columnNumber,
60+
String sourceText,
61+
String first
62+
) {
4263
switch (warningsMode) {
4364
case COLLECT -> {
44-
return new Warnings(lineNumber, columnNumber, sourceText);
65+
return new Warnings(lineNumber, columnNumber, sourceText, first);
4566
}
4667
case IGNORE -> {
4768
return NOOP_WARNINGS;
@@ -50,13 +71,19 @@ public static Warnings createWarnings(DriverContext.WarningsMode warningsMode, i
5071
throw new IllegalStateException("Unreachable");
5172
}
5273

53-
public Warnings(int lineNumber, int columnNumber, String sourceText) {
54-
location = format("Line {}:{}: ", lineNumber, columnNumber);
55-
first = format(
74+
private final String location;
75+
private final String first;
76+
77+
private int addedWarnings;
78+
79+
private Warnings(int lineNumber, int columnNumber, String sourceText, String first) {
80+
this.location = format("Line {}:{}: ", lineNumber, columnNumber);
81+
this.first = format(
5682
null,
57-
"{}evaluation of [{}] failed, treating result as null. Only first {} failures recorded.",
83+
"{}evaluation of [{}] failed, {}. Only first {} failures recorded.",
5884
location,
5985
sourceText,
86+
first,
6087
MAX_ADDED_WARNINGS
6188
);
6289
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/WarningsTests.java‎ renamed to ‎x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/operator/WarningsTests.java‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,33 +5,37 @@
55
* 2.0.
66
*/
77

8-
package org.elasticsearch.xpack.esql.expression.function;
8+
package org.elasticsearch.compute.operator;
99

1010
import org.elasticsearch.test.ESTestCase;
11-
import org.elasticsearch.xpack.esql.core.tree.Source;
1211

1312
public class WarningsTests extends ESTestCase {
14-
public void testRegister() {
15-
Warnings warnings = new Warnings(new Source(1, 1, "foo"));
13+
public void testRegisterCollect() {
14+
Warnings warnings = Warnings.createWarnings(DriverContext.WarningsMode.COLLECT, 1, 1, "foo");
1615
warnings.registerException(new IllegalArgumentException());
1716
assertCriticalWarnings(
18-
"Line 1:2: evaluation of [foo] failed, treating result as null. Only first 20 failures recorded.",
19-
"Line 1:2: java.lang.IllegalArgumentException: null"
17+
"Line 1:1: evaluation of [foo] failed, treating result as null. Only first 20 failures recorded.",
18+
"Line 1:1: java.lang.IllegalArgumentException: null"
2019
);
2120
}
2221

23-
public void testRegisterFilled() {
24-
Warnings warnings = new Warnings(new Source(1, 1, "foo"));
22+
public void testRegisterCollectFilled() {
23+
Warnings warnings = Warnings.createWarnings(DriverContext.WarningsMode.COLLECT, 1, 1, "foo");
2524
for (int i = 0; i < Warnings.MAX_ADDED_WARNINGS + 1000; i++) {
2625
warnings.registerException(new IllegalArgumentException(Integer.toString(i)));
2726
}
2827

2928
String[] expected = new String[21];
30-
expected[0] = "Line 1:2: evaluation of [foo] failed, treating result as null. Only first 20 failures recorded.";
29+
expected[0] = "Line 1:1: evaluation of [foo] failed, treating result as null. Only first 20 failures recorded.";
3130
for (int i = 0; i < Warnings.MAX_ADDED_WARNINGS; i++) {
32-
expected[i + 1] = "Line 1:2: java.lang.IllegalArgumentException: " + i;
31+
expected[i + 1] = "Line 1:1: java.lang.IllegalArgumentException: " + i;
3332
}
3433

3534
assertCriticalWarnings(expected);
3635
}
36+
37+
public void testRegisterIgnore() {
38+
Warnings warnings = Warnings.createWarnings(DriverContext.WarningsMode.IGNORE, 1, 1, "foo");
39+
warnings.registerException(new IllegalArgumentException());
40+
}
3741
}

0 commit comments

Comments
 (0)