Skip to content

Conversation

@astefan
Copy link
Contributor

@astefan astefan commented Aug 19, 2025

Addresses #124731

@astefan astefan marked this pull request as ready for review September 2, 2025 14:18
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Sep 2, 2025
@astefan astefan added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Sep 2, 2025
@astefan astefan requested a review from bpintea September 2, 2025 14:20
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 2, 2025
@elasticsearchmachine
Copy link
Collaborator

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

- class: org.elasticsearch.xpack.ml.integration.InferenceIT
method: testInferClassificationModel
issue: https://github.com/elastic/elasticsearch/issues/133448
- class: org.elasticsearch.xpack.esql.qa.single_node.GenerativeForkIT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discovered in one of the previous runs of this PR while in Draft. Reduced to something that reproduces without inlinestats.

;


inlineStatsUnionGroup-Ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were moved to the inlinestats test file.

@astefan astefan force-pushed the inlinestats_with_union_types branch from c1c89b8 to 94210ab Compare September 2, 2025 14:51
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.

Nice and broad set of tests, thanks.
I'd maybe add (at least) one where the union type is explicitly overwritten before the INLINESTATS. (There are some implicit EVALs before, extracted out of the STATS).

muted-tests.yml Outdated
method: testInferClassificationModel
issue: https://github.com/elastic/elasticsearch/issues/133448
- class: org.elasticsearch.xpack.esql.qa.single_node.GenerativeForkIT
method: test {csv-spec:inlinestats.MultiIndexInlinestats OfMultiTypedField}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
method: test {csv-spec:inlinestats.MultiIndexInlinestats OfMultiTypedField}
method: test {csv-spec:inlinestats.MultiIndexInlinestatsOfMultiTypedField}

But why not "-Ignore" it in the CSV spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this one fails only in the presence of fork and as part of the GenerativeForkIT suite which automatically gathers csv-spec tests and adds | FORK (WHERE true) (WHERE true) to the end of the tests. This test runs fine in other IT/unit test setups.

required_capability: inlinestats_v11

FROM sample_data
| EVAL client_ip = client_ip::ip
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you leave the "nop" conversion here intentionally? To maybe "prepare" for the next query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are a "mirror" of what we already have for stats + union types.

null |172.21.2.113 |2764889 |Connected to 10.1.0.2|2 |14
;

multiIndexIpInlinestats PushableCount-Ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the following (9? tests) -Ignore'd? Most (all?) should work fine.
Only tested some, but if some don't, we should leave a comment and/or issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. Thank you for mentioning this, I forgot about them being ignored. Updated.

- added comments to some of the csv-spec tests
- de-Ignored few csv-spec tests
- renamed (removed the white spaces) csv-spec tests
- more tests
@astefan astefan added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 3, 2025
@astefan astefan force-pushed the inlinestats_with_union_types branch from f1e29f4 to 98ae6b8 Compare September 4, 2025 15:29
@elasticsearchmachine elasticsearchmachine merged commit 049599c into elastic:main Sep 4, 2025
33 checks passed
@astefan astefan deleted the inlinestats_with_union_types branch September 4, 2025 19:45
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 v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants