Skip to content

ESQL: Better tests to AUTO_BUCKET#107228

Merged
elasticsearchmachine merged 3 commits intoelastic:mainfrom
nik9000:auto_bucket_cases
Apr 9, 2024
Merged

ESQL: Better tests to AUTO_BUCKET#107228
elasticsearchmachine merged 3 commits intoelastic:mainfrom
nik9000:auto_bucket_cases

Conversation

@nik9000
Copy link
Copy Markdown
Member

@nik9000 nik9000 commented Apr 8, 2024

This improves the tests for AUTO_BUCKET marginally, specifically so that it tests all valid combinations of arguments and generates a correct types table. This'll combine nicely with #106782 to generate the signatures that kibana needs for it's editor.

This improves the tests for AUTO_BUCKET marginally, specifically so that
it tests all valid combinations of arguments and generates a correct
types table. This'll combine nicely with elastic#106782 to generate the
signatures that kibana needs for it's editor.
@nik9000 nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL v8.14.0 labels Apr 8, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2024

Documentation preview:

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 8, 2024
// TODO check for a limit 4. is arbitrary.
throw new IllegalArgumentException("would generate too many types");
if (argumentCount > 3) {
throw new IllegalArgumentException("would generate too many combinations");
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

4 is like 80k test cases. Too many I think.

Copy link
Copy Markdown
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

LGTM.
I was looking at renaming this function (#107197) and extend it to take just a span (#107272), temporal or numeric and the testing refactoring is welcome


private static Matcher<Object> numericResultsMatcher(List<TestCaseSupplier.TypedData> typedData, Object value) {
return equalTo(value);
private static TestCaseSupplier.TypedData keywordDateLiteral(String name, DataType type, String date) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Leftover?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Checking

Copy link
Copy Markdown
Member

@fang-xing-esql fang-xing-esql left a comment

Choose a reason for hiding this comment

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

LGTM! Except LocateNoStartEvaluator is not relevant.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not relevant to this PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's something we should have committed before. I figured I'd just land it with whatever PR. It looks like Alex landed it all on it's own though. So it's gone from this one.

@nik9000 nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 9, 2024
@elasticsearchmachine elasticsearchmachine merged commit aba7566 into elastic:main Apr 9, 2024
@nik9000 nik9000 deleted the auto_bucket_cases branch April 9, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.14.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants