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

ES|QL: Switch to regex warnings for some CSV tests #110088

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

luigidellaquila
Copy link
Contributor

Some CSV tests randomly fail on environments with multiple nodes (eg. Serverless) because the number of warnings emitted is not completely predictable.
This PR switches to regex warnings, that allow multiple matches for the same warning pattern.

@luigidellaquila luigidellaquila added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL labels Jun 24, 2024
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0 labels Jun 24, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@@ -165,8 +165,8 @@ notLessThanMultivalue
required_capability: mv_warn

from employees | where not(salary_change < 1) | keep emp_no, salary_change | sort emp_no | limit 5;
warning:Line 1:24: evaluation of [not(salary_change < 1)] failed, treating result as null. Only first 20 failures recorded.#[Emulated:Line 1:28: evaluation of [salary_change < 1] failed, treating result as null. Only first 20 failures recorded.]
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we still need this Emulated functionality. We should maybe add an issue to remove it, if after this PR there's no instances of it left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, with regex we have a way to manage these situations.
As a follow-up I'll double-check and in case I'll remove it. In the meantime I opened this #110092

Copy link
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

@luigidellaquila luigidellaquila merged commit 28d7743 into elastic:main Jun 24, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants