Skip to content

[ESQL] Remove Named Expcted Types map from testing infrastructure #111213

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.

Sign up for GitHub

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

Merged
elasticsearchmachine merged 7 commits into from
Jul 24, 2024
Merged

[ESQL] Remove Named Expcted Types map from testing infrastructure #111213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b43a286
wire up new first layer
not-napoleon Jul 23, 2024
043e993
wire up a bunch of explicit type strings
not-napoleon Jul 23, 2024
1c132c4
binary comparision tests
not-napoleon Jul 23, 2024
6792e6b
of course concat is special
not-napoleon Jul 23, 2024
026ad66
remove that blasted map
not-napoleon Jul 23, 2024
bb8fbce
spotless apply
not-napoleon Jul 24, 2024
fedab70
added some javadoc
not-napoleon Jul 24, 2024
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
262 changes: 48 additions & 214 deletions ...java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,9 @@
import org.hamcrest.Matcher;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
Expand Down Expand Up @@ -74,10 +71,14 @@ public abstract class AbstractScalarFunctionTestCase extends AbstractFunctionTes
*/
protected static Iterable<Object[]> parameterSuppliersFromTypedDataWithDefaultChecks(
boolean entirelyNullPreservesType,
List<TestCaseSupplier> suppliers
List<TestCaseSupplier> suppliers,
PositionalErrorMessageSupplier positionalErrorMessageSupplier
Copy link
Copy Markdown
Member

@nik9000 nik9000 Jul 24, 2024

Choose a reason for hiding this comment

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

👍 for giving this a name.

) {
return parameterSuppliersFromTypedData(
errorsForCasesWithoutExamples(anyNullIsNull(entirelyNullPreservesType, randomizeBytesRefsOffset(suppliers)))
errorsForCasesWithoutExamples(
anyNullIsNull(entirelyNullPreservesType, randomizeBytesRefsOffset(suppliers)),
positionalErrorMessageSupplier
)
);
}

Expand Down Expand Up @@ -480,8 +481,14 @@ protected static List<TestCaseSupplier> anyNullIsNull(
* Adds test cases containing unsupported parameter types that assert
* that they throw type errors.
*/
protected static List<TestCaseSupplier> errorsForCasesWithoutExamples(List<TestCaseSupplier> testCaseSuppliers) {
return errorsForCasesWithoutExamples(testCaseSuppliers, AbstractScalarFunctionTestCase::typeErrorMessage);
protected static List<TestCaseSupplier> errorsForCasesWithoutExamples(
List<TestCaseSupplier> testCaseSuppliers,
PositionalErrorMessageSupplier positionalErrorMessageSupplier
) {
return errorsForCasesWithoutExamples(
testCaseSuppliers,
(i, v, t) -> AbstractScalarFunctionTestCase.typeErrorMessage(i, v, t, positionalErrorMessageSupplier)
);
}

protected static List<TestCaseSupplier> errorsForCasesWithoutExamples(
Expand Down Expand Up @@ -515,10 +522,11 @@ protected static List<TestCaseSupplier> errorsForCasesWithoutExamples(
public static String errorMessageStringForBinaryOperators(
boolean includeOrdinal,
List<Set<DataType>> validPerPosition,
List<DataType> types
List<DataType> types,
PositionalErrorMessageSupplier positionalErrorMessageSupplier
) {
try {
return typeErrorMessage(includeOrdinal, validPerPosition, types);
return typeErrorMessage(includeOrdinal, validPerPosition, types, positionalErrorMessageSupplier);
} catch (IllegalStateException e) {
// This means all the positional args were okay, so the expected error is from the combination
if (types.get(0).equals(DataType.UNSIGNED_LONG)) {
Expand Down Expand Up @@ -617,12 +625,33 @@ protected interface TypeErrorMessageSupplier {
String apply(boolean includeOrdinal, List<Set<DataType>> validPerPosition, List<DataType> types);
}

@FunctionalInterface
protected interface PositionalErrorMessageSupplier {
Copy link
Copy Markdown
Member

@nik9000 nik9000 Jul 24, 2024

Choose a reason for hiding this comment

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

Probably wants javadoc given the number of places we're using it.

/**
* This interface defines functions to supply error messages for incorrect types in specific positions. Functions which have
* the same type requirements for all positions can simplify this with a lambda returning a string constant.
*
* @param validForPosition - the set of {@link DataType}s that the test infrastructure believes to be allowable in the
* given position.
* @param position - the zero-index position in the list of parameters the function has detected the bad argument to be.
* @return The string describing the acceptable parameters for that position. Note that this function should not return
* the full error string; that will be constructed by the test. Just return the type string for that position.
*/
String apply(Set<DataType> validForPosition, int position);
}

protected static TestCaseSupplier typeErrorSupplier(
boolean includeOrdinal,
List<Set<DataType>> validPerPosition,
List<DataType> types
List<DataType> types,
PositionalErrorMessageSupplier errorMessageSupplier
) {
return typeErrorSupplier(includeOrdinal, validPerPosition, types, AbstractScalarFunctionTestCase::typeErrorMessage);
return typeErrorSupplier(
includeOrdinal,
validPerPosition,
types,
(o, v, t) -> AbstractScalarFunctionTestCase.typeErrorMessage(o, v, t, errorMessageSupplier)
);
}

/**
Expand All @@ -647,7 +676,12 @@ protected static TestCaseSupplier typeErrorSupplier(
/**
* Build the expected error message for an invalid type signature.
*/
protected static String typeErrorMessage(boolean includeOrdinal, List<Set<DataType>> validPerPosition, List<DataType> types) {
protected static String typeErrorMessage(
boolean includeOrdinal,
List<Set<DataType>> validPerPosition,
List<DataType> types,
PositionalErrorMessageSupplier expectedTypeSupplier
) {
int badArgPosition = -1;
for (int i = 0; i < types.size(); i++) {
if (validPerPosition.get(i).contains(types.get(i)) == false) {
Expand All @@ -661,209 +695,9 @@ protected static String typeErrorMessage(boolean includeOrdinal, List<Set<DataTy
);
}
String ordinal = includeOrdinal ? TypeResolutions.ParamOrdinal.fromIndex(badArgPosition).name().toLowerCase(Locale.ROOT) + " " : "";
String expectedType = expectedType(validPerPosition.get(badArgPosition));
String expectedTypeString = expectedTypeSupplier.apply(validPerPosition.get(badArgPosition), badArgPosition);
String name = types.get(badArgPosition).typeName();
return ordinal + "argument of [] must be [" + expectedType + "], found value [" + name + "] type [" + name + "]";
}

private static final Map<Set<DataType>, String> NAMED_EXPECTED_TYPES = Map.ofEntries(
Map.entry(
Set.of(DataType.DATE_PERIOD, DataType.DOUBLE, DataType.INTEGER, DataType.LONG, DataType.TIME_DURATION, DataType.NULL),
"numeric, date_period or time_duration"
),
Map.entry(Set.of(DataType.DATETIME, DataType.NULL), "datetime"),
Map.entry(Set.of(DataType.DOUBLE, DataType.NULL), "double"),
Map.entry(Set.of(DataType.INTEGER, DataType.NULL), "integer"),
Map.entry(Set.of(DataType.IP, DataType.NULL), "ip"),
Map.entry(Set.of(DataType.LONG, DataType.INTEGER, DataType.UNSIGNED_LONG, DataType.DOUBLE, DataType.NULL), "numeric"),
Map.entry(Set.of(DataType.LONG, DataType.INTEGER, DataType.UNSIGNED_LONG, DataType.DOUBLE), "numeric"),
Map.entry(Set.of(DataType.KEYWORD, DataType.TEXT, DataType.VERSION, DataType.NULL), "string or version"),
Map.entry(Set.of(DataType.KEYWORD, DataType.TEXT, DataType.NULL), "string"),
Map.entry(Set.of(DataType.IP, DataType.KEYWORD, DataType.TEXT, DataType.NULL), "ip or string"),
Map.entry(Set.copyOf(Arrays.asList(representableTypes())), "representable"),
Map.entry(Set.copyOf(Arrays.asList(representableNonSpatialTypes())), "representableNonSpatial"),
Map.entry(
Set.of(
DataType.BOOLEAN,
DataType.DOUBLE,
DataType.INTEGER,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.NULL
),
"boolean or numeric or string"
),
Map.entry(
Set.of(
DataType.DATETIME,
DataType.DOUBLE,
DataType.INTEGER,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.NULL
),
"datetime or numeric or string"
),
// What Add accepts
Map.entry(
Set.of(
DataType.DATE_PERIOD,
DataType.DATETIME,
DataType.DOUBLE,
DataType.INTEGER,
DataType.LONG,
DataType.NULL,
DataType.TIME_DURATION,
DataType.UNSIGNED_LONG
),
"datetime or numeric"
),
Map.entry(
Set.of(
DataType.BOOLEAN,
DataType.DATETIME,
DataType.DOUBLE,
DataType.INTEGER,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.NULL
),
"boolean or datetime or numeric or string"
),
// to_int
Map.entry(
Set.of(
DataType.BOOLEAN,
DataType.COUNTER_INTEGER,
DataType.DATETIME,
DataType.DOUBLE,
DataType.INTEGER,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.NULL
),
"boolean or counter_integer or datetime or numeric or string"
),
// to_long
Map.entry(
Set.of(
DataType.BOOLEAN,
DataType.COUNTER_INTEGER,
DataType.COUNTER_LONG,
DataType.DATETIME,
DataType.DOUBLE,
DataType.INTEGER,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.NULL
),
"boolean or counter_integer or counter_long or datetime or numeric or string"
),
// to_double
Map.entry(
Set.of(
DataType.BOOLEAN,
DataType.COUNTER_DOUBLE,
DataType.COUNTER_INTEGER,
DataType.COUNTER_LONG,
DataType.DATETIME,
DataType.DOUBLE,
DataType.INTEGER,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.NULL
),
"boolean or counter_double or counter_integer or counter_long or datetime or numeric or string"
),
Map.entry(
Set.of(
DataType.BOOLEAN,
DataType.CARTESIAN_POINT,
DataType.DATETIME,
DataType.DOUBLE,
DataType.GEO_POINT,
DataType.INTEGER,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.NULL
),
"boolean or cartesian_point or datetime or geo_point or numeric or string"
),
Map.entry(
Set.of(
DataType.DATETIME,
DataType.DOUBLE,
DataType.INTEGER,
DataType.IP,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.VERSION,
DataType.NULL
),
"datetime, double, integer, ip, keyword, long, text, unsigned_long or version"
),
Map.entry(
Set.of(
DataType.BOOLEAN,
DataType.DATETIME,
DataType.DOUBLE,
DataType.GEO_POINT,
DataType.GEO_SHAPE,
DataType.INTEGER,
DataType.IP,
DataType.KEYWORD,
DataType.LONG,
DataType.TEXT,
DataType.UNSIGNED_LONG,
DataType.VERSION,
DataType.NULL
),
"cartesian_point or datetime or geo_point or numeric or string"
),
Map.entry(Set.of(DataType.GEO_POINT, DataType.KEYWORD, DataType.TEXT, DataType.NULL), "geo_point or string"),
Map.entry(Set.of(DataType.CARTESIAN_POINT, DataType.KEYWORD, DataType.TEXT, DataType.NULL), "cartesian_point or string"),
Map.entry(
Set.of(DataType.GEO_POINT, DataType.GEO_SHAPE, DataType.KEYWORD, DataType.TEXT, DataType.NULL),
"geo_point or geo_shape or string"
),
Map.entry(
Set.of(DataType.CARTESIAN_POINT, DataType.CARTESIAN_SHAPE, DataType.KEYWORD, DataType.TEXT, DataType.NULL),
"cartesian_point or cartesian_shape or string"
),
Map.entry(Set.of(DataType.GEO_POINT, DataType.CARTESIAN_POINT, DataType.NULL), "geo_point or cartesian_point"),
Map.entry(Set.of(DataType.DATE_PERIOD, DataType.TIME_DURATION, DataType.NULL), "dateperiod or timeduration")
);

// TODO: generate this message dynamically, a la AbstractConvertFunction#supportedTypesNames()?
private static String expectedType(Set<DataType> validTypes) {
String named = NAMED_EXPECTED_TYPES.get(validTypes);
if (named == null) {
/*
* Note for anyone who's test lands here - it's likely that you
* don't have a test case covering explicit `null` arguments in
* this position. Generally you can get that with anyNullIsNull.
*/
throw new UnsupportedOperationException(
"can't guess expected types for " + validTypes.stream().sorted(Comparator.comparing(t -> t.typeName())).toList()
);
}
return named;
return ordinal + "argument of [] must be [" + expectedTypeString + "], found value [" + name + "] type [" + name + "]";
}

protected static Stream<DataType> representable() {
Expand Down
2 changes: 1 addition & 1 deletion ...java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/FromBase64Tests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public static Iterable<Object[]> parameters() {
);
}));

return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers);
return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers, (v, p) -> "string");
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToBase64Tests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public static Iterable<Object[]> parameters() {
);
}));

return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers);
return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers, (v, p) -> "string");
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion .../java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToBooleanTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public static Iterable<Object[]> parameters() {
emptyList()
);

return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers);
return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers, (v, p) -> "boolean or numeric or string");
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...rg/elasticsearch/xpack/esql/expression/function/scalar/convert/ToCartesianPointTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public static Iterable<Object[]> parameters() {
);
}

return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers);
return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers, (v, p) -> "cartesian_point or string");
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...rg/elasticsearch/xpack/esql/expression/function/scalar/convert/ToCartesianShapeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public static Iterable<Object[]> parameters() {
);
}

return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers);
return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers, (v, p) -> "cartesian_point or cartesian_shape or string");
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDatetimeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public static Iterable<Object[]> parameters() {
)
);

return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers);
return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers, (v, p) -> "datetime or numeric or string");
}

private static String randomDateString(long from, long to) {
Expand Down
2 changes: 1 addition & 1 deletion .../java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public static Iterable<Object[]> parameters() {
)
);

return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers);
return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers, (v, p) -> "numeric");
}

@Override
Expand Down
6 changes: 5 additions & 1 deletion ...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDoubleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,11 @@ public static Iterable<Object[]> parameters() {
List.of()
);

return parameterSuppliersFromTypedDataWithDefaultChecks(true, suppliers);
return parameterSuppliersFromTypedDataWithDefaultChecks(
true,
suppliers,
(v, p) -> "boolean or counter_double or counter_integer or counter_long or datetime or numeric or string"
);
}

@Override
Expand Down
Loading