[ESQL] Remove Named Expcted Types map from testing infrastructure by not-napoleon · Pull Request #111213 · elastic/elasticsearch

not-napoleon · 2024-07-23T21:24:23Z

This removes the NAMED_EXPECTED_TYPES map from the testing infrastructure. It had become difficult to maintain, and pushed error message text farther way from the code actually testing it. This PR introduces some small functional interfaces to enable scalar function tests to have more fine grained control over their expected error messages.

We could do more here, but I think this is a good step in the right direction.

elasticsearchmachine · 2024-07-23T21:24:48Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

nik9000 · 2024-07-24T13:45:24Z

...st/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java

    }

+    @FunctionalInterface
+    protected interface PositionalErrorMessageSupplier {


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

nik9000 · 2024-07-24T13:45:30Z

...st/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java

        boolean entirelyNullPreservesType,
-        List<TestCaseSupplier> suppliers
+        List<TestCaseSupplier> suppliers,
+        PositionalErrorMessageSupplier positionalErrorMessageSupplier


👍 for giving this a name.

nik9000 · 2024-07-24T13:47:25Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateExtractTests.java

+            (v, p) -> switch (p) {
+                case 0 -> "string";
+                case 1 -> "datetime";
+                default -> "";


should this throw on unknown?

so we get to the default if the code that figures out which position has the bad argument comes up with a number higher than the number of arguments. Like if it says "the fourth argument to + is bad". That should really never happen, and I didn't put a lot of thought into what to do if it did, I just made the switch expression happy.

I tend to make them happy by throwing if I don't expect it. I suppose in this case it's not a big difference.

nik9000 · 2024-07-24T13:48:03Z

...st/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvFirstTests.java

        geoShape(cases, "mv_first", "MvFirst", DataType.GEO_SHAPE, (size, values) -> equalTo(values.findFirst().get()));
        cartesianShape(cases, "mv_first", "MvFirst", DataType.CARTESIAN_SHAPE, (size, values) -> equalTo(values.findFirst().get()));
-        return parameterSuppliersFromTypedDataWithDefaultChecks(false, cases);
+        return parameterSuppliersFromTypedDataWithDefaultChecks(false, cases, (v, p) -> "");


Should this throw? Maybe say something like all types are valid or something.

I mean, this will fail if a type ends up not being valid. The function will generate an error message with the string "representable" here, which will not match the test's expected empty string. Throwing from here doesn't really make that any clearer in my opinion? But I'm open to discuss.

nik9000 · 2024-07-24T13:48:10Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMaxTests.java

        unsignedLongs(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.reduce(BigInteger::max).get()));
        dateTimes(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.max().getAsLong()));
-        return parameterSuppliersFromTypedDataWithDefaultChecks(false, cases);
+        return parameterSuppliersFromTypedDataWithDefaultChecks(false, cases, (v, p) -> "representableNonSpatial");


nik9000 · 2024-07-24T13:49:04Z

...ava/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/LessThanTests.java

+                    o,
+                    v,
+                    t,
+                    (l, p) -> "datetime, double, integer, ip, keyword, long, text, unsigned_long or version"


This might be better off returning a closure. But not sure.

I poked around at it a bit, I didn't think it looked all that much better. Please feel free to submit a follow up that cleans it up if you want.

ivancea

LGTM.

However, I feel like we're missing the possibility to standardize the error messages here. The current solution is far from ideal, but somehow guides you into using an specific format. I wonder if we could go towards some more strict generation, like using the @Param data to generate it.

Just speaking aloud. The only part I don't "like" here is that we're decentralizing the messages, which be make more tedious to undo later, if we want to do that

ivancea · 2024-07-24T13:52:34Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMinTests.java

        unsignedLongs(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.reduce(BigInteger::min).get()));
        dateTimes(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.min().getAsLong()));
-        return parameterSuppliersFromTypedDataWithDefaultChecks(false, cases);
+        return parameterSuppliersFromTypedDataWithDefaultChecks(false, cases, (v, p) -> "representableNonSpatial");


I think this message is wrong (?), shouldn't it be human-readable?

It is wrong, but let's grab it in a follow up, I think.

yes, it should. But again, this is just putting the string into the test, not changing the string that the function currently sends on type error. I do think it's worth spending some time to review our type errors and update them, but that's not the goal of this PR.

nik9000 · 2024-07-24T13:57:23Z

However, I feel like we're missing the possibility to standardize the error messages here.

I think we can get that by making some "canned" error message suppliers or something like that. That way places that aren't standard can use this and that are can provide the closure with a named method call. But I think this is a step in the right direction.

not-napoleon · 2024-07-24T14:03:18Z

decentralizing the messages, which be make more tedious to undo later, if we want to do that

Just to be clear, the code that's being removed was never involved in generating the actual error messages. It was just being used to guess the error message for the test to validate. The error messages were always generated within the type checking infrastructure, on each function. See, for example, MvMax#resolveFieldType

* main: (39 commits) Update README.asciidoc (elastic#111244) ESQL: INLINESTATS (elastic#109583) ESQL: Document a little of `DataType` (elastic#111250) Relax assertions in segment level field stats (elastic#111243) LogsDB data generator - support nested object field (elastic#111206) Validate `Authorization` header in Azure test fixture (elastic#111242) Fixing HistoryStoreTests.testPut() and testStoreWithHideSecrets() (elastic#111246) [ESQL] Remove Named Expcted Types map from testing infrastructure (elastic#111213) Change visibility of createWriter to allow tests from a different package to override it (elastic#111234) [ES|QL] Remove EsqlDataTypes (elastic#111089) Mute org.elasticsearch.repositories.azure.AzureBlobContainerRetriesTests testReadNonexistentBlobThrowsNoSuchFileException elastic#111233 Abstract codec lookup by name, to make CodecService extensible (elastic#111007) Add HTTPS support to `AzureHttpFixture` (elastic#111228) Unmuting tests related to free_context action being processed in ESSingleNodeTestCase (elastic#111224) Upgrade Azure SDK (elastic#111225) Collapse transport versions for 8.14.0 (elastic#111199) Make sure contender uses logs templates (elastic#111183) unmute HistogramPercentileAggregationTests.testBoxplotHistogram (elastic#111223) Refactor Quality Assurance test infrastructure (elastic#111195) Mute org.elasticsearch.xpack.restart.FullClusterRestartIT testDisableFieldNameField {cluster=UPGRADED} elastic#111222 ... # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

not-napoleon added 5 commits July 23, 2024 13:35

wire up new first layer

b43a286

wire up a bunch of explicit type strings

043e993

binary comparision tests

1c132c4

of course concat is special

6792e6b

remove that blasted map

026ad66

not-napoleon added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL v8.16.0 labels Jul 23, 2024

not-napoleon requested a review from nik9000 July 23, 2024 21:24

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 23, 2024

spotless apply

bb8fbce

nik9000 approved these changes Jul 24, 2024

View reviewed changes

ivancea approved these changes Jul 24, 2024

View reviewed changes

added some javadoc

fedab70

not-napoleon added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jul 24, 2024

elasticsearchmachine merged commit c5be248 into elastic:main Jul 24, 2024

not-napoleon deleted the esql-remove-named-expected-types branch July 24, 2024 16:39

Conversation

not-napoleon commented Jul 23, 2024

Uh oh!

elasticsearchmachine commented Jul 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jul 24, 2024

Uh oh!

not-napoleon commented Jul 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants