ESQL: Move more test type error testing by nik9000 · Pull Request #119945 · elastic/elasticsearch

nik9000 · 2025-01-10T14:16:18Z

This reduces the number of test cases in ESQL a little more ala #119678. It migrates a few random tests and all of the multivalue functions:

92775 -> 43760
 3m45 -> 4m04

This adds a few more error test cases that were missing to make sure it all lines up well. And it fixes a few error messages in a few functions. That's likely where the extra time goes.

This reduces the number of test cases in ESQL a little more ala elastic#119678. It migrates a few random tests and all of the multivalue functions: ``` 92775 -> 43760 3m45 -> 4m04 ``` This adds a few more error test cases that were missing to make sure it all lines up well. And it fixes a few error messages in a few functions. That's *likely* where the extra time goes.

github-actions · 2025-01-10T14:16:36Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2025-01-10T14:16:44Z

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

not-napoleon

LGTM. I left a few notes about possible follow-up work, but this is a big improvement IMHO.

not-napoleon · 2025-01-15T16:43:32Z

...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java

+        TypeResolution resolution = isType(field, dt -> switch (dt) {
+            case UNSIGNED_LONG, GEO_POINT, GEO_SHAPE, CARTESIAN_POINT, CARTESIAN_SHAPE -> false;
+            default -> true;
+        }, sourceText(), FIRST, "not unsigned long or geo");


Probably out of scope for this PR, but I wonder if DataType should have an isSortable concept that we could use here. It's a little weird that if we add a data type that shouldn't sort here, we need to update this exclusion list. Hopefully the tests will fail, so it's not that big a deal, but might be worth thinking about down the road.

I think that's out of scope, yeah, but it does feel useful.

not-napoleon · 2025-01-15T17:00:40Z

...src/test/java/org/elasticsearch/xpack/esql/expression/function/fulltext/MatchErrorTests.java

+            return typeErrorMessage(true, validPerPosition, signature, positionalErrorMessageSupplier);
+        } catch (IllegalStateException e) {
+            // This means all the positional args were okay, so the expected error is for nulls or from the combination
+            return EsqlBinaryComparison.formatIncompatibleTypesMessage(signature.get(0), signature.get(1), sourceForSignature(signature));


Out of scope here, but maybe we should move formatIncompatibleTypesMessage if we're going to use it for more than just the comparison operators.

not-napoleon · 2025-01-15T17:16:00Z

...va/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvCountErrorTests.java

+             * In general MvCount should support all signatures. While building a
+             * new type you may we to temporarily remove this.
+             */
+            throw new UnsupportedOperationException("all signatures should be supported");


I like this pattern for validating that all types work. Let's add a note in the javadoc for ErrorsForCasesWithoutExamplesTestCase that we want to follow this pattern for "any type" functions. If it comes up enough, might be worth refactoring it too.

not-napoleon · 2025-01-15T17:27:36Z

...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSortTests.java

+            nulls(suppliers, orderType);
+        }
+
+        List<TestCaseSupplier> extra = new ArrayList<>();


This could use a comment about what exactly it's adding cases for.

ivancea

LGTM! Some minor concerns, ignore them at will

ivancea · 2025-01-15T17:27:42Z

...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java

-        TypeResolution resolution = isType(field, DataType::isRepresentable, sourceText(), FIRST, "representable");
+        TypeResolution resolution = isType(field, dt -> switch (dt) {
+            case UNSIGNED_LONG, GEO_POINT, GEO_SHAPE, CARTESIAN_POINT, CARTESIAN_SHAPE -> false;
+            default -> true;


Are there other non-representable cases we want to avoid here?

Suggested change

default -> true;

default -> dt.isRepresentable();

ivancea · 2025-01-15T17:47:45Z

...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java

        }

-        return isString(order, sourceText(), SECOND);
+        return isNotNull(order, sourceText(), SECOND).and(isString(order, sourceText(), SECOND));


This is already checked in the PostOptimizationVerificationAware interface, implemented here.

Given that most TypeResolutions consider null to be "ok", I would probably leave it as-is, and check it in the other interface?

Unless there's another reason to add this change. It really doesn't matter I think, just to avoid doing it "sometimes"

Got it. I'll see if I can incorportate that bit.

Adds tests for requests that would fill up the heap and crash elasticsearch but for our circuit breakers. In an ideal world we'd stream these results back to and this wouldn't crash anything. But we don't have that at the moment.

…ove_error_cases3

nik9000

I've reverted the troublesome changes - MvPercentile and MvSort. Those we'll grab in another PR.

This reduces the number of test cases in ESQL a little more ala elastic#119678. It migrates a few random tests and all of the multivalue functions: ``` 92775 -> 43760 3m45 -> 4m04 ``` This adds a few more error test cases that were missing to make sure it all lines up well. And it fixes a few error messages in a few functions. That's *likely* where the extra time goes.

nik9000 · 2025-01-16T20:44:52Z

Backport: #120324

This reduces the number of test cases in ESQL a little more ala #119678. It migrates a few random tests and all of the multivalue functions: ``` 92775 -> 43760 3m45 -> 4m04 ``` This adds a few more error test cases that were missing to make sure it all lines up well. And it fixes a few error messages in a few functions. That's *likely* where the extra time goes.

This reduces the number of test cases in ESQL a little more ala elastic#119678. It migrates a few random tests and all of the multivalue functions: ``` 92775 -> 43760 3m45 -> 4m04 ``` This adds a few more error test cases that were missing to make sure it all lines up well. And it fixes a few error messages in a few functions. That's *likely* where the extra time goes.

Following in the same vein as #119678 and #119945, this PR moves the errors tests in the numeric package to their own class. 43,303 tests -> 34,521 tests 6m 30s -> 5m 50s

Following in the same vein as elastic#119678 and elastic#119945, this PR moves the errors tests in the numeric package to their own class. 43,303 tests -> 34,521 tests 6m 30s -> 5m 50s

Following in the same vein as #119678 and #119945, this PR moves the errors tests in the numeric package to their own class. 43,303 tests -> 34,521 tests 6m 30s -> 5m 50s

nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Jan 10, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 10, 2025

Merge branch 'main' into move_error_cases3

324c026

not-napoleon approved these changes Jan 15, 2025

View reviewed changes

ivancea approved these changes Jan 15, 2025

View reviewed changes

nik9000 added 9 commits January 15, 2025 13:22

Examples

46244b5

ESQL: Heap attack tests

1a90c82

Adds tests for requests that would fill up the heap and crash elasticsearch but for our circuit breakers. In an ideal world we'd stream these results back to and this wouldn't crash anything. But we don't have that at the moment.

Merge branch 'main' into esql_lookup_heap_attack

c127529

Merge branch 'main' into move_error_cases3

c8282ad

Merge remote-tracking branch 'nik9000/esql_lookup_heap_attack' into m…

9b559a9

…ove_error_cases3

REvert more

4d8d39a

How'd this get in there?

4eb5758

Revert harder

06d3f4c

Revert proper

b68f1f4

nik9000 commented Jan 16, 2025

View reviewed changes

nik9000 enabled auto-merge (squash) January 16, 2025 19:25

nik9000 merged commit 1c13465 into elastic:main Jan 16, 2025

GalLalouche mentioned this pull request Jan 27, 2025

ESQL: Yet more test type error testing movements #120962

Merged

GalLalouche mentioned this pull request Jan 30, 2025

ESQL: Even more test type error testing movements #121321

Merged

Conversation

nik9000 commented Jan 10, 2025

Uh oh!

github-actions bot commented Jan 10, 2025

Uh oh!

elasticsearchmachine commented Jan 10, 2025

Uh oh!

not-napoleon 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jan 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants