Skip to content

Commit 7bed305

Browse files
nik9000nik9000
authored
ESQL: Allow duplicate warnings in some tests (#116199) (#116367)
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 99fcffc commit 7bed305

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
}
@@ -431,11 +428,13 @@ public void testOutOfRangeComparisons() throws IOException {
431428
for (String truePredicate : trueForSingleValuesPredicates) {
432429
String comparison = fieldWithType + truePredicate;
433430
var query = requestObjectBuilder().query(format(null, "from {} | where {}", testIndexName(), comparison));
434-
List<String> expectedWarnings = List.of(
435-
"Line 1:29: evaluation of [" + comparison + "] failed, treating result as null. Only first 20 failures recorded.",
436-
"Line 1:29: java.lang.IllegalArgumentException: single-value function encountered multi-value"
431+
AssertWarnings assertWarnings = new AssertWarnings.ExactStrings(
432+
List.of(
433+
"Line 1:29: evaluation of [" + comparison + "] failed, treating result as null. Only first 20 failures recorded.",
434+
"Line 1:29: java.lang.IllegalArgumentException: single-value function encountered multi-value"
435+
)
437436
);
438-
var result = runEsql(query, expectedWarnings, NO_WARNINGS_REGEX, mode);
437+
var result = runEsql(query, assertWarnings, mode);
439438

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

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

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

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

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

1011-
static Map<String, Object> runEsql(
1012-
RequestObjectBuilder requestObject,
1013-
List<String> expectedWarnings,
1014-
List<Pattern> expectedWarningsRegex,
1015-
Mode mode
1016-
) throws IOException {
1010+
static Map<String, Object> runEsql(RequestObjectBuilder requestObject, AssertWarnings assertWarnings, Mode mode) throws IOException {
10171011
if (mode == ASYNC) {
1018-
return runEsqlAsync(requestObject, expectedWarnings, expectedWarningsRegex);
1012+
return runEsqlAsync(requestObject, assertWarnings);
10191013
} else {
1020-
return runEsqlSync(requestObject, expectedWarnings, expectedWarningsRegex);
1014+
return runEsqlSync(requestObject, assertWarnings);
10211015
}
10221016
}
10231017

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

1044-
HttpEntity entity = performRequest(request, expectedWarnings, expectedWarningsRegex);
1034+
HttpEntity entity = performRequest(request, assertWarnings);
10451035
return entityToMap(entity, requestObject.contentType());
10461036
}
10471037

1048-
public static Map<String, Object> runEsqlAsync(
1049-
RequestObjectBuilder requestObject,
1050-
List<String> expectedWarnings,
1051-
List<Pattern> expectedWarningsRegex
1052-
) throws IOException {
1038+
public static Map<String, Object> runEsqlAsync(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException {
10531039
addAsyncParameters(requestObject);
10541040
requestObject.build();
10551041
Request request = prepareRequest(ASYNC);
@@ -1089,7 +1075,7 @@ public static Map<String, Object> runEsqlAsync(
10891075
assertThat(response.getHeader("X-Elasticsearch-Async-Id"), nullValue());
10901076
assertThat(response.getHeader("X-Elasticsearch-Async-Is-Running"), is("?0"));
10911077
}
1092-
assertWarnings(response, expectedWarnings, expectedWarningsRegex);
1078+
assertWarnings(response, assertWarnings);
10931079
json.remove("is_running"); // remove this to not mess up later map assertions
10941080
return Collections.unmodifiableMap(json);
10951081
} else {
@@ -1099,7 +1085,7 @@ public static Map<String, Object> runEsqlAsync(
10991085
if (isRunning == false) {
11001086
// must have completed immediately so keep_on_completion must be true
11011087
assertThat(requestObject.keepOnCompletion(), is(true));
1102-
assertWarnings(response, expectedWarnings, expectedWarningsRegex);
1088+
assertWarnings(response, assertWarnings);
11031089
// we already have the results, but let's remember them so that we can compare to async get
11041090
initialColumns = json.get("columns");
11051091
initialValues = json.get("values");
@@ -1129,7 +1115,7 @@ public static Map<String, Object> runEsqlAsync(
11291115
assertEquals(initialValues, result.get("values"));
11301116
}
11311117

1132-
assertWarnings(response, expectedWarnings, expectedWarningsRegex);
1118+
assertWarnings(response, assertWarnings);
11331119
assertDeletable(id);
11341120
return removeAsyncProperties(result);
11351121
}
@@ -1203,7 +1189,7 @@ static String runEsqlAsTextWithFormat(RequestObjectBuilder builder, String forma
12031189
}
12041190
request.setOptions(options);
12051191

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

@@ -1236,9 +1222,8 @@ private static String attachBody(RequestObjectBuilder requestObject, Request req
12361222
return mediaType;
12371223
}
12381224

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

12441229
private static Response performRequest(Request request) throws IOException {
@@ -1251,13 +1236,13 @@ private static Response performRequest(Request request) throws IOException {
12511236
return response;
12521237
}
12531238

1254-
private static HttpEntity assertWarnings(Response response, List<String> allowedWarnings, List<Pattern> allowedWarningsRegex) {
1239+
private static HttpEntity assertWarnings(Response response, AssertWarnings assertWarnings) {
12551240
List<String> warnings = new ArrayList<>(response.getWarnings());
12561241
warnings.removeAll(mutedWarnings());
12571242
if (shouldLog()) {
12581243
LOGGER.info("RESPONSE warnings (after muted)={}", warnings);
12591244
}
1260-
EsqlTestUtils.assertWarnings(warnings, allowedWarnings, allowedWarningsRegex);
1245+
assertWarnings.assertWarnings(warnings);
12611246
return response.getEntity();
12621247
}
12631248

‎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)