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.

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

@@ -91,4 +91,15 @@ public class MixedClusterEsqlSpecIT extends EsqlSpecTestCase {
     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;
+    }
 }

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

@@ -26,6 +26,7 @@ import org.elasticsearch.logging.Logger;
 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.List;
 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 @@ public abstract class EsqlSpecTestCase extends ESRestTestCase {
             builder.tables(tables());
         }
 
-        Map<String, Object> answer = runEsql(builder.query(testCase.query), testCase.expectedWarnings(), testCase.expectedWarningsRegex());
+        Map<String, Object> answer = runEsql(builder.query(testCase.query), testCase.assertWarnings(deduplicateExactWarnings()));
 
         var expectedColumnsWithValues = loadCsvSpecValues(testCase.expectedResults);
 
@@ -248,16 +248,30 @@ public abstract class EsqlSpecTestCase extends ESRestTestCase {
         assertResults(expectedColumnsWithValues, actualColumns, actualValues, testCase.ignoreOrder, logger);
     }
 
-    private Map<String, Object> runEsql(
-        RequestObjectBuilder requestObject,
-        List<String> expectedWarnings,
-        List<Pattern> 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.
+     * <p>
+     *     Note: This only applies to warnings declared as {@code warning:}. Those
+     *     declared as {@code warningRegex:} are always a list of
+     *     <strong>allowed</strong> warnings. {@code warningRegex:} matches 0 or more
+     *     warnings. There is no need to deduplicate because there's no expectation
+     *     of an exact match.
+     * </p>
+     *
+     */
+    protected boolean deduplicateExactWarnings() {
+        return false;
+    }
+
+    private Map<String, Object> 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);
         }
     }
 

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

@@ -31,6 +31,7 @@ import org.elasticsearch.test.rest.ESRestTestCase;
 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.Locale;
 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<String> NO_WARNINGS = List.of();
-    private static final List<Pattern> NO_WARNINGS_REGEX = List.of();
-
     private static final String MAPPING_ALL_TYPES;
 
     static {
@@ -379,7 +376,7 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
         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);
     }
@@ -431,11 +428,13 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
             for (String truePredicate : trueForSingleValuesPredicates) {
                 String comparison = fieldWithType + truePredicate;
                 var query = requestObjectBuilder().query(format(null, "from {} | where {}", testIndexName(), comparison));
-                List<String> 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(
@@ -505,7 +504,7 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
 
         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),
@@ -997,35 +996,26 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
     }
 
     public Map<String, Object> runEsql(RequestObjectBuilder requestObject) throws IOException {
-        return runEsql(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX, mode);
+        return runEsql(requestObject, new AssertWarnings.NoWarnings(), mode);
     }
 
     public static Map<String, Object> runEsqlSync(RequestObjectBuilder requestObject) throws IOException {
-        return runEsqlSync(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX);
+        return runEsqlSync(requestObject, new AssertWarnings.NoWarnings());
     }
 
     public static Map<String, Object> runEsqlAsync(RequestObjectBuilder requestObject) throws IOException {
-        return runEsqlAsync(requestObject, NO_WARNINGS, NO_WARNINGS_REGEX);
+        return runEsqlAsync(requestObject, new AssertWarnings.NoWarnings());
     }
 
-    static Map<String, Object> runEsql(
-        RequestObjectBuilder requestObject,
-        List<String> expectedWarnings,
-        List<Pattern> expectedWarningsRegex,
-        Mode mode
-    ) throws IOException {
+    static Map<String, Object> 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<String, Object> runEsqlSync(
-        RequestObjectBuilder requestObject,
-        List<String> expectedWarnings,
-        List<Pattern> expectedWarningsRegex
-    ) throws IOException {
+    public static Map<String, Object> runEsqlSync(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException {
         requestObject.build();
         Request request = prepareRequest(SYNC);
         String mediaType = attachBody(requestObject, request);
@@ -1041,15 +1031,11 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
         }
         request.setOptions(options);
 
-        HttpEntity entity = performRequest(request, expectedWarnings, expectedWarningsRegex);
+        HttpEntity entity = performRequest(request, assertWarnings);
         return entityToMap(entity, requestObject.contentType());
     }
 
-    public static Map<String, Object> runEsqlAsync(
-        RequestObjectBuilder requestObject,
-        List<String> expectedWarnings,
-        List<Pattern> expectedWarningsRegex
-    ) throws IOException {
+    public static Map<String, Object> runEsqlAsync(RequestObjectBuilder requestObject, AssertWarnings assertWarnings) throws IOException {
         addAsyncParameters(requestObject);
         requestObject.build();
         Request request = prepareRequest(ASYNC);
@@ -1089,7 +1075,7 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
                 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 {
@@ -1099,7 +1085,7 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
             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");
@@ -1129,7 +1115,7 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
             assertEquals(initialValues, result.get("values"));
         }
 
-        assertWarnings(response, expectedWarnings, expectedWarningsRegex);
+        assertWarnings(response, assertWarnings);
         assertDeletable(id);
         return removeAsyncProperties(result);
     }
@@ -1203,7 +1189,7 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
         }
         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));
     }
 
@@ -1236,9 +1222,8 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
         return mediaType;
     }
 
-    private static HttpEntity performRequest(Request request, List<String> allowedWarnings, List<Pattern> 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 {
@@ -1251,13 +1236,13 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
         return response;
     }
 
-    private static HttpEntity assertWarnings(Response response, List<String> allowedWarnings, List<Pattern> allowedWarningsRegex) {
+    private static HttpEntity assertWarnings(Response response, AssertWarnings assertWarnings) {
         List<String> 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();
     }
 

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<String> warnings);
+
+    record NoWarnings() implements AssertWarnings {
+        @Override
+        public void assertWarnings(List<String> warnings) {
+            assertMap(warnings.stream().sorted().toList(), matchesList());
+        }
+    }
+
+    record ExactStrings(List<String> expected) implements AssertWarnings {
+        @Override
+        public void assertWarnings(List<String> warnings) {
+            assertMap(warnings.stream().sorted().toList(), matchesList(expected.stream().sorted().toList()));
+        }
+    }
+
+    record DeduplicatedStrings(List<String> expected) implements AssertWarnings {
+        @Override
+        public void assertWarnings(List<String> warnings) {
+            assertMap(warnings.stream().sorted().distinct().toList(), matchesList(expected.stream().sorted().toList()));
+        }
+    }
+
+    record AllowedRegexes(List<Pattern> expected) implements AssertWarnings {
+        @Override
+        public void assertWarnings(List<String> warnings) {
+            for (String warning : warnings) {
+                assertTrue("Unexpected warning: " + warning, expected.stream().anyMatch(x -> x.matcher(warning).matches()));
+            }
+        }
+    }
+}

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

@@ -142,6 +142,26 @@ public final class CsvSpecReader {
         public List<Pattern> 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();
+        }
     }
 
 }

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

@@ -97,7 +97,6 @@ import java.util.Map;
 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.randomLongBetween;
 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.ParamClassificatio
 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 final class EsqlTestUtils {
         return String.join(" | ", all);
     }
 
-    public static void assertWarnings(List<String> warnings, List<String> allowedWarnings, List<Pattern> 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};

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

@@ -496,7 +496,7 @@ public class CsvTests extends ESTestCase {
                 normalized.add(normW);
             }
         }
-        EsqlTestUtils.assertWarnings(normalized, testCase.expectedWarnings(), testCase.expectedWarningsRegex());
+        testCase.assertWarnings(false).assertWarnings(normalized);
     }
 
     PlanRunner planRunner(BigArrays bigArrays, TestPhysicalOperationProviders physicalOperationProviders) {