Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESQL: Allow duplicate warnings in some tests #116199

Merged
merged 5 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -230,7 +230,7 @@ protected final void doTest() throws Throwable {
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);

Expand All @@ -248,16 +248,30 @@ protected final void doTest() throws Throwable {
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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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<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(
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -996,35 +995,26 @@ private static String expectedTextBody(String format, int count, @Nullable Chara
}

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);
Expand All @@ -1040,15 +1030,11 @@ public static Map<String, Object> runEsqlSync(
}
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);
Expand Down Expand Up @@ -1088,7 +1074,7 @@ public static Map<String, Object> 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 {
Expand All @@ -1098,7 +1084,7 @@ public static Map<String, Object> 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");
Expand Down Expand Up @@ -1128,7 +1114,7 @@ public static Map<String, Object> runEsqlAsync(
assertEquals(initialValues, result.get("values"));
}

assertWarnings(response, expectedWarnings, expectedWarningsRegex);
assertWarnings(response, assertWarnings);
assertDeletable(id);
return removeAsyncProperties(result);
}
Expand Down Expand Up @@ -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));
}

Expand Down Expand Up @@ -1235,9 +1221,8 @@ private static String attachBody(RequestObjectBuilder requestObject, Request req
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 {
Expand All @@ -1250,13 +1235,13 @@ private static Response performRequest(Request request) throws IOException {
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();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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()));
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,22 @@ public void adjustExpectedWarnings(Function<String, String> updater) {
public List<Pattern> expectedWarningsRegex() {
return expectedWarningsRegex;
}

/**
* How should we assert the warnings returned by ESQL.
*/
public AssertWarnings assertWarnings(boolean deduplicateExact) {
if (expectedWarnings.isEmpty() == false) {
if (deduplicateExact) {
return new AssertWarnings.DeduplicatedStrings(expectedWarnings);
}
return new AssertWarnings.ExactStrings(expectedWarnings);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the ternary operator would make this and below more legible to me, but it's fine this way too.

}
if (expectedWarningsRegex.isEmpty() == false) {
return new AssertWarnings.AllowedRegexes(expectedWarningsRegex);
}
return new AssertWarnings.NoWarnings();
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 {

Expand Down Expand Up @@ -407,16 +403,6 @@ public static String randomEnrichCommand(String name, Enrich.Mode mode, String m
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};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ private void assertWarnings(List<String> warnings) {
normalized.add(normW);
}
}
EsqlTestUtils.assertWarnings(normalized, testCase.expectedWarnings(), testCase.expectedWarningsRegex());
testCase.assertWarnings(false).assertWarnings(normalized);
}

PlanRunner planRunner(BigArrays bigArrays, TestPhysicalOperationProviders physicalOperationProviders) {
Expand Down