Skip to content

Commit dc59784

Browse files
nik9000nik9000
authored
ESQL: Allow duplicate warnings in some tests (#116199)
This fixes a test, actually in serverless Elasticsearch, that gets duplicate warnings. We'd like not to emit these duplicate warnings, but at this point it isn't worth it. So, for now, in some tests we allow duplicate warnings. In most of our tests we do not allow duplicate warnings so that we don't make *more* duplicate warnings without thinking about it.
1 parent 9829cd2 commit dc59784

File tree

7 files changed

+133
-65
lines changed

7 files changed

+133
-65
lines changed

‎x-pack/plugin/esql/qa/server/mixed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/mixed/MixedClusterEsqlSpecIT.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,15 @@ protected boolean enableRoundingDoubleValuesOnAsserting() {
9191
protected boolean supportsInferenceTestService() {
9292
return false;
9393
}
94+
95+
@Override
96+
protected boolean deduplicateExactWarnings() {
97+
/*
98+
* In ESQL's main tests we shouldn't have to deduplicate but in
99+
* serverless, where we reuse this test case exactly with *slightly*
100+
* different configuration, we must deduplicate. So we do it here.
101+
* It's a bit of a loss of precision, but that's ok.
102+
*/
103+
return true;
104+
}
94105
}

‎x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.elasticsearch.test.rest.ESRestTestCase;
2727
import org.elasticsearch.test.rest.TestFeatureService;
2828
import org.elasticsearch.xcontent.XContentType;
29+
import org.elasticsearch.xpack.esql.AssertWarnings;
2930
import org.elasticsearch.xpack.esql.CsvSpecReader.CsvTestCase;
3031
import org.elasticsearch.xpack.esql.CsvTestUtils;
3132
import org.elasticsearch.xpack.esql.EsqlTestUtils;
@@ -47,7 +48,6 @@
4748
import java.util.Locale;
4849
import java.util.Map;
4950
import java.util.TreeMap;
50-
import java.util.regex.Pattern;
5151
import java.util.stream.Collectors;
5252
import java.util.stream.IntStream;
5353
import java.util.stream.LongStream;
@@ -230,7 +230,7 @@ protected final void doTest() throws Throwable {
230230
builder.tables(tables());
231231
}
232232

233-
Map<String, Object> answer = runEsql(builder.query(testCase.query), testCase.expectedWarnings(), testCase.expectedWarningsRegex());
233+
Map<String, Object> answer = runEsql(builder.query(testCase.query), testCase.assertWarnings(deduplicateExactWarnings()));
234234

235235
var expectedColumnsWithValues = loadCsvSpecValues(testCase.expectedResults);
236236

@@ -248,16 +248,30 @@ protected final void doTest() throws Throwable {
248248
assertResults(expectedColumnsWithValues, actualColumns, actualValues, testCase.ignoreOrder, logger);
249249
}
250250

251-
private Map<String, Object> runEsql(
252-
RequestObjectBuilder requestObject,
253-
List<String> expectedWarnings,
254-
List<Pattern> expectedWarningsRegex
255-
) throws IOException {
251+
/**
252+
* Should warnings be de-duplicated before checking for exact matches. Defaults
253+
* to {@code false}, but in some environments we emit duplicate warnings. We'd prefer
254+
* not to emit duplicate warnings but for now it isn't worth fighting with. So! In
255+
* those environments we override this to deduplicate.
256+
* <p>
257+
* Note: This only applies to warnings declared as {@code warning:}. Those
258+
* declared as {@code warningRegex:} are always a list of
259+
* <strong>allowed</strong> warnings. {@code warningRegex:} matches 0 or more
260+
* warnings. There is no need to deduplicate because there's no expectation
261+
* of an exact match.
262+
* </p>
263+
*
264+
*/
265+
protected boolean deduplicateExactWarnings() {
266+
return false;
267+
}
268+
269+
private Map<String, Object> runEsql(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException {
256270
if (mode == Mode.ASYNC) {
257271
assert supportsAsync();
258-
return RestEsqlTestCase.runEsqlAsync(requestObject, expectedWarnings, expectedWarningsRegex);
272+
return RestEsqlTestCase.runEsqlAsync(requestObject, assertWarnings);
259273
} else {
260-
return RestEsqlTestCase.runEsqlSync(requestObject, expectedWarnings, expectedWarningsRegex);
274+
return RestEsqlTestCase.runEsqlSync(requestObject, assertWarnings);
261275
}
262276
}
263277

‎x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java‎

Lines changed: 26 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.elasticsearch.xcontent.ToXContent;
3232
import org.elasticsearch.xcontent.XContentBuilder;
3333
import org.elasticsearch.xcontent.XContentType;
34+
import org.elasticsearch.xpack.esql.AssertWarnings;
3435
import org.elasticsearch.xpack.esql.EsqlTestUtils;
3536
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
3637
import org.junit.After;
@@ -53,7 +54,6 @@
5354
import java.util.Map;
5455
import java.util.Set;
5556
import java.util.function.IntFunction;
56-
import java.util.regex.Pattern;
5757

5858
import static java.util.Collections.emptySet;
5959
import static java.util.Map.entry;
@@ -83,9 +83,6 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
8383

8484
private static final Logger LOGGER = LogManager.getLogger(RestEsqlTestCase.class);
8585

86-
private static final List<String> NO_WARNINGS = List.of();
87-
private static final List<Pattern> NO_WARNINGS_REGEX = List.of();
88-
8986
private static final String MAPPING_ALL_TYPES;
9087

9188
static {
@@ -379,7 +376,7 @@ public void testCSVNoHeaderMode() throws IOException {
379376
options.addHeader("Content-Type", mediaType);
380377
options.addHeader("Accept", "text/csv; header=absent");
381378
request.setOptions(options);
382-
HttpEntity entity = performRequest(request, NO_WARNINGS, NO_WARNINGS_REGEX);
379+
HttpEntity entity = performRequest(request, new AssertWarnings.NoWarnings());
383380
String actual = Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8));
384381
assertEquals("keyword0,0\r\n", actual);
385382
}
@@ -430,11 +427,13 @@ public void testOutOfRangeComparisons() throws IOException {
430427
for (String truePredicate : trueForSingleValuesPredicates) {
431428
String comparison = fieldWithType + truePredicate;
432429
var query = requestObjectBuilder().query(format(null, "from {} | where {}", testIndexName(), comparison));
433-
List<String> expectedWarnings = List.of(
434-
"Line 1:29: evaluation of [" + comparison + "] failed, treating result as null. Only first 20 failures recorded.",
435-
"Line 1:29: java.lang.IllegalArgumentException: single-value function encountered multi-value"
430+
AssertWarnings assertWarnings = new AssertWarnings.ExactStrings(
431+
List.of(
432+
"Line 1:29: evaluation of [" + comparison + "] failed, treating result as null. Only first 20 failures recorded.",
433+
"Line 1:29: java.lang.IllegalArgumentException: single-value function encountered multi-value"
434+
)
436435
);
437-
var result = runEsql(query, expectedWarnings, NO_WARNINGS_REGEX, mode);
436+
var result = runEsql(query, assertWarnings, mode);
438437

439438
var values = as(result.get("values"), ArrayList.class);
440439
assertThat(
@@ -504,7 +503,7 @@ public void testInternalRange() throws IOException {
504503

505504
for (String p : predicates) {
506505
var query = requestObjectBuilder().query(format(null, "from {} | where {}", testIndexName(), p));
507-
var result = runEsql(query, List.of(), NO_WARNINGS_REGEX, mode);
506+
var result = runEsql(query, new AssertWarnings.NoWarnings(), mode);
508507
var values = as(result.get("values"), ArrayList.class);
509508
assertThat(
510509
format(null, "Comparison [{}] should return all rows with single values.", p),
@@ -996,35 +995,26 @@ private static String expectedTextBody(String format, int count, @Nullable Chara
996995
}
997996

998997
public Map<String, Object> runEsql(RequestObjectBuilder requestObject) throws IOException {
999-
return runEsql(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX, mode);
998+
return runEsql(requestObject, new AssertWarnings.NoWarnings(), mode);
1000999
}
10011000

10021001
public static Map<String, Object> runEsqlSync(RequestObjectBuilder requestObject) throws IOException {
1003-
return runEsqlSync(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX);
1002+
return runEsqlSync(requestObject, new AssertWarnings.NoWarnings());
10041003
}
10051004

10061005
public static Map<String, Object> runEsqlAsync(RequestObjectBuilder requestObject) throws IOException {
1007-
return runEsqlAsync(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX);
1006+
return runEsqlAsync(requestObject, new AssertWarnings.NoWarnings());
10081007
}
10091008

1010-
static Map<String, Object> runEsql(
1011-
RequestObjectBuilder requestObject,
1012-
List<String> expectedWarnings,
1013-
List<Pattern> expectedWarningsRegex,
1014-
Mode mode
1015-
) throws IOException {
1009+
static Map<String, Object> runEsql(RequestObjectBuilder requestObject, AssertWarnings assertWarnings, Mode mode) throws IOException {
10161010
if (mode == ASYNC) {
1017-
return runEsqlAsync(requestObject, expectedWarnings, expectedWarningsRegex);
1011+
return runEsqlAsync(requestObject, assertWarnings);
10181012
} else {
1019-
return runEsqlSync(requestObject, expectedWarnings, expectedWarningsRegex);
1013+
return runEsqlSync(requestObject, assertWarnings);
10201014
}
10211015
}
10221016

1023-
public static Map<String, Object> runEsqlSync(
1024-
RequestObjectBuilder requestObject,
1025-
List<String> expectedWarnings,
1026-
List<Pattern> expectedWarningsRegex
1027-
) throws IOException {
1017+
public static Map<String, Object> runEsqlSync(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException {
10281018
requestObject.build();
10291019
Request request = prepareRequest(SYNC);
10301020
String mediaType = attachBody(requestObject, request);
@@ -1040,15 +1030,11 @@ public static Map<String, Object> runEsqlSync(
10401030
}
10411031
request.setOptions(options);
10421032

1043-
HttpEntity entity = performRequest(request, expectedWarnings, expectedWarningsRegex);
1033+
HttpEntity entity = performRequest(request, assertWarnings);
10441034
return entityToMap(entity, requestObject.contentType());
10451035
}
10461036

1047-
public static Map<String, Object> runEsqlAsync(
1048-
RequestObjectBuilder requestObject,
1049-
List<String> expectedWarnings,
1050-
List<Pattern> expectedWarningsRegex
1051-
) throws IOException {
1037+
public static Map<String, Object> runEsqlAsync(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException {
10521038
addAsyncParameters(requestObject);
10531039
requestObject.build();
10541040
Request request = prepareRequest(ASYNC);
@@ -1088,7 +1074,7 @@ public static Map<String, Object> runEsqlAsync(
10881074
assertThat(response.getHeader("X-Elasticsearch-Async-Id"), nullValue());
10891075
assertThat(response.getHeader("X-Elasticsearch-Async-Is-Running"), is("?0"));
10901076
}
1091-
assertWarnings(response, expectedWarnings, expectedWarningsRegex);
1077+
assertWarnings(response, assertWarnings);
10921078
json.remove("is_running"); // remove this to not mess up later map assertions
10931079
return Collections.unmodifiableMap(json);
10941080
} else {
@@ -1098,7 +1084,7 @@ public static Map<String, Object> runEsqlAsync(
10981084
if (isRunning == false) {
10991085
// must have completed immediately so keep_on_completion must be true
11001086
assertThat(requestObject.keepOnCompletion(), is(true));
1101-
assertWarnings(response, expectedWarnings, expectedWarningsRegex);
1087+
assertWarnings(response, assertWarnings);
11021088
// we already have the results, but let's remember them so that we can compare to async get
11031089
initialColumns = json.get("columns");
11041090
initialValues = json.get("values");
@@ -1128,7 +1114,7 @@ public static Map<String, Object> runEsqlAsync(
11281114
assertEquals(initialValues, result.get("values"));
11291115
}
11301116

1131-
assertWarnings(response, expectedWarnings, expectedWarningsRegex);
1117+
assertWarnings(response, assertWarnings);
11321118
assertDeletable(id);
11331119
return removeAsyncProperties(result);
11341120
}
@@ -1202,7 +1188,7 @@ static String runEsqlAsTextWithFormat(RequestObjectBuilder builder, String forma
12021188
}
12031189
request.setOptions(options);
12041190

1205-
HttpEntity entity = performRequest(request, NO_WARNINGS, NO_WARNINGS_REGEX);
1191+
HttpEntity entity = performRequest(request, new AssertWarnings.NoWarnings());
12061192
return Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8));
12071193
}
12081194

@@ -1235,9 +1221,8 @@ private static String attachBody(RequestObjectBuilder requestObject, Request req
12351221
return mediaType;
12361222
}
12371223

1238-
private static HttpEntity performRequest(Request request, List<String> allowedWarnings, List<Pattern> allowedWarningsRegex)
1239-
throws IOException {
1240-
return assertWarnings(performRequest(request), allowedWarnings, allowedWarningsRegex);
1224+
private static HttpEntity performRequest(Request request, AssertWarnings assertWarnings) throws IOException {
1225+
return assertWarnings(performRequest(request), assertWarnings);
12411226
}
12421227

12431228
private static Response performRequest(Request request) throws IOException {
@@ -1250,13 +1235,13 @@ private static Response performRequest(Request request) throws IOException {
12501235
return response;
12511236
}
12521237

1253-
private static HttpEntity assertWarnings(Response response, List<String> allowedWarnings, List<Pattern> allowedWarningsRegex) {
1238+
private static HttpEntity assertWarnings(Response response, AssertWarnings assertWarnings) {
12541239
List<String> warnings = new ArrayList<>(response.getWarnings());
12551240
warnings.removeAll(mutedWarnings());
12561241
if (shouldLog()) {
12571242
LOGGER.info("RESPONSE warnings (after muted)={}", warnings);
12581243
}
1259-
EsqlTestUtils.assertWarnings(warnings, allowedWarnings, allowedWarningsRegex);
1244+
assertWarnings.assertWarnings(warnings);
12601245
return response.getEntity();
12611246
}
12621247

‎x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/AssertWarnings.java‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.esql;
9+
10+
import java.util.List;
11+
import java.util.regex.Pattern;
12+
13+
import static org.elasticsearch.test.ListMatcher.matchesList;
14+
import static org.elasticsearch.test.MapMatcher.assertMap;
15+
import static org.junit.Assert.assertTrue;
16+
17+
/**
18+
* How should we assert the warnings returned by ESQL.
19+
*/
20+
public interface AssertWarnings {
21+
void assertWarnings(List<String> warnings);
22+
23+
record NoWarnings() implements AssertWarnings {
24+
@Override
25+
public void assertWarnings(List<String> warnings) {
26+
assertMap(warnings.stream().sorted().toList(), matchesList());
27+
}
28+
}
29+
30+
record ExactStrings(List<String> expected) implements AssertWarnings {
31+
@Override
32+
public void assertWarnings(List<String> warnings) {
33+
assertMap(warnings.stream().sorted().toList(), matchesList(expected.stream().sorted().toList()));
34+
}
35+
}
36+
37+
record DeduplicatedStrings(List<String> expected) implements AssertWarnings {
38+
@Override
39+
public void assertWarnings(List<String> warnings) {
40+
assertMap(warnings.stream().sorted().distinct().toList(), matchesList(expected.stream().sorted().toList()));
41+
}
42+
}
43+
44+
record AllowedRegexes(List<Pattern> expected) implements AssertWarnings {
45+
@Override
46+
public void assertWarnings(List<String> warnings) {
47+
for (String warning : warnings) {
48+
assertTrue("Unexpected warning: " + warning, expected.stream().anyMatch(x -> x.matcher(warning).matches()));
49+
}
50+
}
51+
}
52+
}

‎x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvSpecReader.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,26 @@ public void adjustExpectedWarnings(Function<String, String> updater) {
142142
public List<Pattern> expectedWarningsRegex() {
143143
return expectedWarningsRegex;
144144
}
145+
146+
/**
147+
* How should we assert the warnings returned by ESQL.
148+
* @param deduplicateExact Should tests configured with {@code warnings:} deduplicate
149+
* the warnings before asserting? Normally don't do it because
150+
* duplicate warnings are lame. We'd like to fix them all. But
151+
* in multi-node and multi-shard tests we can emit duplicate
152+
* warnings and it isn't worth fixing them now.
153+
*/
154+
public AssertWarnings assertWarnings(boolean deduplicateExact) {
155+
if (expectedWarnings.isEmpty() == false) {
156+
return deduplicateExact
157+
? new AssertWarnings.DeduplicatedStrings(expectedWarnings)
158+
: new AssertWarnings.ExactStrings(expectedWarnings);
159+
}
160+
if (expectedWarningsRegex.isEmpty() == false) {
161+
return new AssertWarnings.AllowedRegexes(expectedWarningsRegex);
162+
}
163+
return new AssertWarnings.NoWarnings();
164+
}
145165
}
146166

147167
}

‎x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,6 @@
9797
import java.util.Set;
9898
import java.util.TreeMap;
9999
import java.util.jar.JarInputStream;
100-
import java.util.regex.Pattern;
101100
import java.util.zip.ZipEntry;
102101

103102
import static java.util.Collections.emptyList;
@@ -118,8 +117,6 @@
118117
import static org.elasticsearch.test.ESTestCase.randomMillisUpToYear9999;
119118
import static org.elasticsearch.test.ESTestCase.randomShort;
120119
import static org.elasticsearch.test.ESTestCase.randomZone;
121-
import static org.elasticsearch.test.ListMatcher.matchesList;
122-
import static org.elasticsearch.test.MapMatcher.assertMap;
123120
import static org.elasticsearch.xpack.esql.core.tree.Source.EMPTY;
124121
import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER;
125122
import static org.elasticsearch.xpack.esql.core.type.DataType.NULL;
@@ -129,7 +126,6 @@
129126
import static org.elasticsearch.xpack.esql.parser.ParserUtils.ParamClassification.PATTERN;
130127
import static org.elasticsearch.xpack.esql.parser.ParserUtils.ParamClassification.VALUE;
131128
import static org.hamcrest.Matchers.instanceOf;
132-
import static org.junit.Assert.assertTrue;
133129

134130
public final class EsqlTestUtils {
135131

@@ -407,16 +403,6 @@ public static String randomEnrichCommand(String name, Enrich.Mode mode, String m
407403
return String.join(" | ", all);
408404
}
409405

410-
public static void assertWarnings(List<String> warnings, List<String> allowedWarnings, List<Pattern> allowedWarningsRegex) {
411-
if (allowedWarningsRegex.isEmpty()) {
412-
assertMap(warnings.stream().sorted().toList(), matchesList(allowedWarnings.stream().sorted().toList()));
413-
} else {
414-
for (String warning : warnings) {
415-
assertTrue("Unexpected warning: " + warning, allowedWarningsRegex.stream().anyMatch(x -> x.matcher(warning).matches()));
416-
}
417-
}
418-
}
419-
420406
/**
421407
* "tables" provided in the context for the LOOKUP command. If you
422408
* add to this, you must also add to {@code EsqlSpecTestCase#tables};

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -496,7 +496,7 @@ private void assertWarnings(List<String> warnings) {
496496
normalized.add(normW);
497497
}
498498
}
499-
EsqlTestUtils.assertWarnings(normalized, testCase.expectedWarnings(), testCase.expectedWarningsRegex());
499+
testCase.assertWarnings(false).assertWarnings(normalized);
500500
}
501501

502502
PlanRunner planRunner(BigArrays bigArrays, TestPhysicalOperationProviders physicalOperationProviders) {

0 commit comments

Comments
 (0)