diff --git a/x-pack/plugin/esql/qa/server/mixed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/mixed/MixedClusterEsqlSpecIT.java b/x-pack/plugin/esql/qa/server/mixed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/mixed/MixedClusterEsqlSpecIT.java index 0e23b29172c32..801e1d12b1d4a 100644 --- a/x-pack/plugin/esql/qa/server/mixed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/mixed/MixedClusterEsqlSpecIT.java +++ b/x-pack/plugin/esql/qa/server/mixed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/mixed/MixedClusterEsqlSpecIT.java @@ -91,4 +91,15 @@ protected boolean enableRoundingDoubleValuesOnAsserting() { protected boolean supportsInferenceTestService() { return false; } + + @Override + protected boolean deduplicateExactWarnings() { + /* + * In ESQL's main tests we shouldn't have to deduplicate but in + * serverless, where we reuse this test case exactly with *slightly* + * different configuration, we must deduplicate. So we do it here. + * It's a bit of a loss of precision, but that's ok. + */ + return true; + } } diff --git a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java index 57f58fc448822..6ebf05755ef5e 100644 --- a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java +++ b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java @@ -26,6 +26,7 @@ import org.elasticsearch.test.rest.ESRestTestCase; import org.elasticsearch.test.rest.TestFeatureService; import org.elasticsearch.xcontent.XContentType; +import org.elasticsearch.xpack.esql.AssertWarnings; import org.elasticsearch.xpack.esql.CsvSpecReader.CsvTestCase; import org.elasticsearch.xpack.esql.CsvTestUtils; import org.elasticsearch.xpack.esql.EsqlTestUtils; @@ -47,7 +48,6 @@ import java.util.Locale; import java.util.Map; import java.util.TreeMap; -import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.LongStream; @@ -230,7 +230,7 @@ protected final void doTest() throws Throwable { builder.tables(tables()); } - Map answer = runEsql(builder.query(testCase.query), testCase.expectedWarnings(), testCase.expectedWarningsRegex()); + Map answer = runEsql(builder.query(testCase.query), testCase.assertWarnings(deduplicateExactWarnings())); var expectedColumnsWithValues = loadCsvSpecValues(testCase.expectedResults); @@ -248,16 +248,30 @@ protected final void doTest() throws Throwable { assertResults(expectedColumnsWithValues, actualColumns, actualValues, testCase.ignoreOrder, logger); } - private Map runEsql( - RequestObjectBuilder requestObject, - List expectedWarnings, - List expectedWarningsRegex - ) throws IOException { + /** + * Should warnings be de-duplicated before checking for exact matches. Defaults + * to {@code false}, but in some environments we emit duplicate warnings. We'd prefer + * not to emit duplicate warnings but for now it isn't worth fighting with. So! In + * those environments we override this to deduplicate. + *

+ * Note: This only applies to warnings declared as {@code warning:}. Those + * declared as {@code warningRegex:} are always a list of + * allowed warnings. {@code warningRegex:} matches 0 or more + * warnings. There is no need to deduplicate because there's no expectation + * of an exact match. + *

+ * + */ + protected boolean deduplicateExactWarnings() { + return false; + } + + private Map runEsql(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException { if (mode == Mode.ASYNC) { assert supportsAsync(); - return RestEsqlTestCase.runEsqlAsync(requestObject, expectedWarnings, expectedWarningsRegex); + return RestEsqlTestCase.runEsqlAsync(requestObject, assertWarnings); } else { - return RestEsqlTestCase.runEsqlSync(requestObject, expectedWarnings, expectedWarningsRegex); + return RestEsqlTestCase.runEsqlSync(requestObject, assertWarnings); } } diff --git a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java index ef1e77280d0ee..505ab3adc553b 100644 --- a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java +++ b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java @@ -31,6 +31,7 @@ import org.elasticsearch.xcontent.ToXContent; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentType; +import org.elasticsearch.xpack.esql.AssertWarnings; import org.elasticsearch.xpack.esql.EsqlTestUtils; import org.elasticsearch.xpack.esql.action.EsqlCapabilities; import org.junit.After; @@ -53,7 +54,6 @@ import java.util.Map; import java.util.Set; import java.util.function.IntFunction; -import java.util.regex.Pattern; import static java.util.Collections.emptySet; import static java.util.Map.entry; @@ -83,9 +83,6 @@ public abstract class RestEsqlTestCase extends ESRestTestCase { private static final Logger LOGGER = LogManager.getLogger(RestEsqlTestCase.class); - private static final List NO_WARNINGS = List.of(); - private static final List NO_WARNINGS_REGEX = List.of(); - private static final String MAPPING_ALL_TYPES; static { @@ -379,7 +376,7 @@ public void testCSVNoHeaderMode() throws IOException { options.addHeader("Content-Type", mediaType); options.addHeader("Accept", "text/csv; header=absent"); request.setOptions(options); - HttpEntity entity = performRequest(request, NO_WARNINGS, NO_WARNINGS_REGEX); + HttpEntity entity = performRequest(request, new AssertWarnings.NoWarnings()); String actual = Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8)); assertEquals("keyword0,0\r\n", actual); } @@ -430,11 +427,13 @@ public void testOutOfRangeComparisons() throws IOException { for (String truePredicate : trueForSingleValuesPredicates) { String comparison = fieldWithType + truePredicate; var query = requestObjectBuilder().query(format(null, "from {} | where {}", testIndexName(), comparison)); - List expectedWarnings = List.of( - "Line 1:29: evaluation of [" + comparison + "] failed, treating result as null. Only first 20 failures recorded.", - "Line 1:29: java.lang.IllegalArgumentException: single-value function encountered multi-value" + AssertWarnings assertWarnings = new AssertWarnings.ExactStrings( + List.of( + "Line 1:29: evaluation of [" + comparison + "] failed, treating result as null. Only first 20 failures recorded.", + "Line 1:29: java.lang.IllegalArgumentException: single-value function encountered multi-value" + ) ); - var result = runEsql(query, expectedWarnings, NO_WARNINGS_REGEX, mode); + var result = runEsql(query, assertWarnings, mode); var values = as(result.get("values"), ArrayList.class); assertThat( @@ -504,7 +503,7 @@ public void testInternalRange() throws IOException { for (String p : predicates) { var query = requestObjectBuilder().query(format(null, "from {} | where {}", testIndexName(), p)); - var result = runEsql(query, List.of(), NO_WARNINGS_REGEX, mode); + var result = runEsql(query, new AssertWarnings.NoWarnings(), mode); var values = as(result.get("values"), ArrayList.class); assertThat( 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 } public Map runEsql(RequestObjectBuilder requestObject) throws IOException { - return runEsql(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX, mode); + return runEsql(requestObject, new AssertWarnings.NoWarnings(), mode); } public static Map runEsqlSync(RequestObjectBuilder requestObject) throws IOException { - return runEsqlSync(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX); + return runEsqlSync(requestObject, new AssertWarnings.NoWarnings()); } public static Map runEsqlAsync(RequestObjectBuilder requestObject) throws IOException { - return runEsqlAsync(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX); + return runEsqlAsync(requestObject, new AssertWarnings.NoWarnings()); } - static Map runEsql( - RequestObjectBuilder requestObject, - List expectedWarnings, - List expectedWarningsRegex, - Mode mode - ) throws IOException { + static Map runEsql(RequestObjectBuilder requestObject, AssertWarnings assertWarnings, Mode mode) throws IOException { if (mode == ASYNC) { - return runEsqlAsync(requestObject, expectedWarnings, expectedWarningsRegex); + return runEsqlAsync(requestObject, assertWarnings); } else { - return runEsqlSync(requestObject, expectedWarnings, expectedWarningsRegex); + return runEsqlSync(requestObject, assertWarnings); } } - public static Map runEsqlSync( - RequestObjectBuilder requestObject, - List expectedWarnings, - List expectedWarningsRegex - ) throws IOException { + public static Map runEsqlSync(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException { requestObject.build(); Request request = prepareRequest(SYNC); String mediaType = attachBody(requestObject, request); @@ -1040,15 +1030,11 @@ public static Map runEsqlSync( } request.setOptions(options); - HttpEntity entity = performRequest(request, expectedWarnings, expectedWarningsRegex); + HttpEntity entity = performRequest(request, assertWarnings); return entityToMap(entity, requestObject.contentType()); } - public static Map runEsqlAsync( - RequestObjectBuilder requestObject, - List expectedWarnings, - List expectedWarningsRegex - ) throws IOException { + public static Map runEsqlAsync(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException { addAsyncParameters(requestObject); requestObject.build(); Request request = prepareRequest(ASYNC); @@ -1088,7 +1074,7 @@ public static Map runEsqlAsync( assertThat(response.getHeader("X-Elasticsearch-Async-Id"), nullValue()); assertThat(response.getHeader("X-Elasticsearch-Async-Is-Running"), is("?0")); } - assertWarnings(response, expectedWarnings, expectedWarningsRegex); + assertWarnings(response, assertWarnings); json.remove("is_running"); // remove this to not mess up later map assertions return Collections.unmodifiableMap(json); } else { @@ -1098,7 +1084,7 @@ public static Map runEsqlAsync( if (isRunning == false) { // must have completed immediately so keep_on_completion must be true assertThat(requestObject.keepOnCompletion(), is(true)); - assertWarnings(response, expectedWarnings, expectedWarningsRegex); + assertWarnings(response, assertWarnings); // we already have the results, but let's remember them so that we can compare to async get initialColumns = json.get("columns"); initialValues = json.get("values"); @@ -1128,7 +1114,7 @@ public static Map runEsqlAsync( assertEquals(initialValues, result.get("values")); } - assertWarnings(response, expectedWarnings, expectedWarningsRegex); + assertWarnings(response, assertWarnings); assertDeletable(id); return removeAsyncProperties(result); } @@ -1202,7 +1188,7 @@ static String runEsqlAsTextWithFormat(RequestObjectBuilder builder, String forma } request.setOptions(options); - HttpEntity entity = performRequest(request, NO_WARNINGS, NO_WARNINGS_REGEX); + HttpEntity entity = performRequest(request, new AssertWarnings.NoWarnings()); return Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8)); } @@ -1235,9 +1221,8 @@ private static String attachBody(RequestObjectBuilder requestObject, Request req return mediaType; } - private static HttpEntity performRequest(Request request, List allowedWarnings, List allowedWarningsRegex) - throws IOException { - return assertWarnings(performRequest(request), allowedWarnings, allowedWarningsRegex); + private static HttpEntity performRequest(Request request, AssertWarnings assertWarnings) throws IOException { + return assertWarnings(performRequest(request), assertWarnings); } private static Response performRequest(Request request) throws IOException { @@ -1250,13 +1235,13 @@ private static Response performRequest(Request request) throws IOException { return response; } - private static HttpEntity assertWarnings(Response response, List allowedWarnings, List allowedWarningsRegex) { + private static HttpEntity assertWarnings(Response response, AssertWarnings assertWarnings) { List warnings = new ArrayList<>(response.getWarnings()); warnings.removeAll(mutedWarnings()); if (shouldLog()) { LOGGER.info("RESPONSE warnings (after muted)={}", warnings); } - EsqlTestUtils.assertWarnings(warnings, allowedWarnings, allowedWarningsRegex); + assertWarnings.assertWarnings(warnings); return response.getEntity(); } diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/AssertWarnings.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/AssertWarnings.java new file mode 100644 index 0000000000000..f606d36ee6b6c --- /dev/null +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/AssertWarnings.java @@ -0,0 +1,52 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.esql; + +import java.util.List; +import java.util.regex.Pattern; + +import static org.elasticsearch.test.ListMatcher.matchesList; +import static org.elasticsearch.test.MapMatcher.assertMap; +import static org.junit.Assert.assertTrue; + +/** + * How should we assert the warnings returned by ESQL. + */ +public interface AssertWarnings { + void assertWarnings(List warnings); + + record NoWarnings() implements AssertWarnings { + @Override + public void assertWarnings(List warnings) { + assertMap(warnings.stream().sorted().toList(), matchesList()); + } + } + + record ExactStrings(List expected) implements AssertWarnings { + @Override + public void assertWarnings(List warnings) { + assertMap(warnings.stream().sorted().toList(), matchesList(expected.stream().sorted().toList())); + } + } + + record DeduplicatedStrings(List expected) implements AssertWarnings { + @Override + public void assertWarnings(List warnings) { + assertMap(warnings.stream().sorted().distinct().toList(), matchesList(expected.stream().sorted().toList())); + } + } + + record AllowedRegexes(List expected) implements AssertWarnings { + @Override + public void assertWarnings(List warnings) { + for (String warning : warnings) { + assertTrue("Unexpected warning: " + warning, expected.stream().anyMatch(x -> x.matcher(warning).matches())); + } + } + } +} diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvSpecReader.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvSpecReader.java index 781ae5531c6f0..84e06e0c1b674 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvSpecReader.java +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvSpecReader.java @@ -142,6 +142,26 @@ public void adjustExpectedWarnings(Function updater) { public List expectedWarningsRegex() { return expectedWarningsRegex; } + + /** + * How should we assert the warnings returned by ESQL. + * @param deduplicateExact Should tests configured with {@code warnings:} deduplicate + * the warnings before asserting? Normally don't do it because + * duplicate warnings are lame. We'd like to fix them all. But + * in multi-node and multi-shard tests we can emit duplicate + * warnings and it isn't worth fixing them now. + */ + public AssertWarnings assertWarnings(boolean deduplicateExact) { + if (expectedWarnings.isEmpty() == false) { + return deduplicateExact + ? new AssertWarnings.DeduplicatedStrings(expectedWarnings) + : new AssertWarnings.ExactStrings(expectedWarnings); + } + if (expectedWarningsRegex.isEmpty() == false) { + return new AssertWarnings.AllowedRegexes(expectedWarningsRegex); + } + return new AssertWarnings.NoWarnings(); + } } } diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java index e755ddb4d0d10..c5c3788fbce47 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java @@ -97,7 +97,6 @@ import java.util.Set; import java.util.TreeMap; import java.util.jar.JarInputStream; -import java.util.regex.Pattern; import java.util.zip.ZipEntry; import static java.util.Collections.emptyList; @@ -118,8 +117,6 @@ import static org.elasticsearch.test.ESTestCase.randomMillisUpToYear9999; import static org.elasticsearch.test.ESTestCase.randomShort; import static org.elasticsearch.test.ESTestCase.randomZone; -import static org.elasticsearch.test.ListMatcher.matchesList; -import static org.elasticsearch.test.MapMatcher.assertMap; import static org.elasticsearch.xpack.esql.core.tree.Source.EMPTY; import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER; import static org.elasticsearch.xpack.esql.core.type.DataType.NULL; @@ -129,7 +126,6 @@ import static org.elasticsearch.xpack.esql.parser.ParserUtils.ParamClassification.PATTERN; import static org.elasticsearch.xpack.esql.parser.ParserUtils.ParamClassification.VALUE; import static org.hamcrest.Matchers.instanceOf; -import static org.junit.Assert.assertTrue; public final class EsqlTestUtils { @@ -407,16 +403,6 @@ public static String randomEnrichCommand(String name, Enrich.Mode mode, String m return String.join(" | ", all); } - public static void assertWarnings(List warnings, List allowedWarnings, List allowedWarningsRegex) { - if (allowedWarningsRegex.isEmpty()) { - assertMap(warnings.stream().sorted().toList(), matchesList(allowedWarnings.stream().sorted().toList())); - } else { - for (String warning : warnings) { - assertTrue("Unexpected warning: " + warning, allowedWarningsRegex.stream().anyMatch(x -> x.matcher(warning).matches())); - } - } - } - /** * "tables" provided in the context for the LOOKUP command. If you * add to this, you must also add to {@code EsqlSpecTestCase#tables}; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java index c05a7d51b3b2f..348ca4acd100e 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java @@ -496,7 +496,7 @@ private void assertWarnings(List warnings) { normalized.add(normW); } } - EsqlTestUtils.assertWarnings(normalized, testCase.expectedWarnings(), testCase.expectedWarningsRegex()); + testCase.assertWarnings(false).assertWarnings(normalized); } PlanRunner planRunner(BigArrays bigArrays, TestPhysicalOperationProviders physicalOperationProviders) {