Skip to content

ES|QL: fix generative tests #127560

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
merged 8 commits into from
May 9, 2025
Merged

ES|QL: fix generative tests #127560

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fa6845b
ES|QL: fix generative tests
luigidellaquila Apr 30, 2025
ec65d0b
Merge branch 'main' into esql/fix_generative_tests_20250430
luigidellaquila Apr 30, 2025
915bac1
Refactoring for valid fields
luigidellaquila May 2, 2025
bc46368
Merge branch 'main' into esql/fix_generative_tests_20250430
luigidellaquila May 2, 2025
246caeb
Merge branch 'main' into esql/fix_generative_tests_20250430
luigidellaquila May 5, 2025
e80b881
Remove condition for fixed bug
luigidellaquila May 5, 2025
95b2876
Fix
luigidellaquila May 5, 2025
55b61d1
Merge branch 'main' into esql/fix_generative_tests_20250430
luigidellaquila May 9, 2025
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
3 changes: 0 additions & 3 deletions muted-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,9 +432,6 @@ tests:
- class: org.elasticsearch.xpack.esql.qa.single_node.PushQueriesIT
method: testPushCaseInsensitiveEqualityOnDefaults
issue: https://github.com/elastic/elasticsearch/issues/127431
- class: org.elasticsearch.xpack.esql.qa.single_node.GenerativeIT
method: test
issue: https://github.com/elastic/elasticsearch/issues/127536
- class: org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT
method: test {union_types.MultiIndexSortIpStringEval ASYNC}
issue: https://github.com/elastic/elasticsearch/issues/127537
Expand Down
2 changes: 2 additions & 0 deletions ...ver/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/EsqlQueryGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ private static String keep(List<Column> previousOutput) {
private static String randomName(List<Column> previousOutput) {
String result = randomRawName(previousOutput);
if (result.isEmpty() // bug https://github.com/elastic/elasticsearch/issues/125870, we'll manage it as an error later
|| result.contains("<")
Copy link
Contributor

@astefan astefan Apr 30, 2025

Choose a reason for hiding this comment

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

Have you considered excluding <no-fields> from randomRawName instead?
And maybe create a static variable that excludes these two and be called in multiple places in this class; I see randomGroupableName and randomRawName excludes <all-fields-projected>.

Copy link
Contributor

@astefan astefan Apr 30, 2025

Choose a reason for hiding this comment

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

Actually, <all-fields-projected> is found in 8 places in this file...

Copy link
Contributor Author

@luigidellaquila luigidellaquila Apr 30, 2025

Choose a reason for hiding this comment

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

That's a good point.
From a purely functional point of view, <no-fields> is there and you can use it, so having it in the tests would make sense.
On the other hand, quoting all the <something> fields would probably make the test less effective in discovering other bugs.
I think I'll go with your suggestion and just add <no-fields> to a blacklist

Copy link
Contributor Author

@luigidellaquila luigidellaquila Apr 30, 2025

Choose a reason for hiding this comment

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

Oh... I just tested it with

from no_* | limit 1 | keep `<no-fields>`

Unknown column [<no-fields>], did you mean [<no-fields>]?

Copy link
Contributor Author

@luigidellaquila luigidellaquila May 5, 2025

Choose a reason for hiding this comment

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

I did some refactoring, now the management of special cases for field names is centralized.

|| result.contains(">")
|| (randomBoolean() && result.contains("*") == false)) {
result = "`" + result + "`";
}
Expand Down