Skip to content

Conversation

@luigidellaquila
Copy link
Contributor

@luigidellaquila luigidellaquila commented Apr 30, 2025

Refactoring of how field names are extracted from the output of the previous query, taking into consideration known issues.

Fixes: #127536

@luigidellaquila luigidellaquila added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL labels Apr 30, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 30, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@luigidellaquila luigidellaquila changed the title Esql/fix generative tests 20250430 ES|QL: fix generative tests 20250430 Apr 30, 2025
@luigidellaquila luigidellaquila changed the title ES|QL: fix generative tests 20250430 ES|QL: fix generative tests Apr 30, 2025
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

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

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

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

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

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.

@luigidellaquila luigidellaquila requested a review from astefan May 5, 2025 08:27
Copy link
Contributor

@astefan astefan 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 enabled auto-merge (squash) May 9, 2025 07:47
@luigidellaquila luigidellaquila merged commit a0b0aca into elastic:main May 9, 2025
16 of 17 checks passed
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request May 12, 2025
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 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] GenerativeIT test failing

3 participants