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

ESQL: Fail in AggregateFunction when LogicPlan is not an Aggregate #124446

Merged

Conversation

kanoshiou
Copy link
Contributor

Trigger Verifier failure in AggregateFunction if the LogicPlan is not an Aggregate.

Closes #124311

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Mar 9, 2025
@javanna javanna added :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Mar 14, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 14, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@ivancea ivancea self-requested a review March 17, 2025 11:24
@ivancea ivancea self-assigned this Mar 17, 2025
@ivancea ivancea added the >bug label Mar 17, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@ivancea
Copy link
Contributor

ivancea commented Mar 17, 2025

buildkite test this

Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! It looks good!
About the RRF command (First comment 👀), I'll ping @ioanatia in case she sees something I don't, as she knows the command far better, and so they know about this change.
Otherwise, after it's changed to work with RRF and the tests pass, it should be fine!

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add another test with multiple aggs in the same WHERE/EVAL, or both in the same query with an agg each?
You can test it like here:

public void testNotFoundFieldInNestedFunction() {
assertEquals("""
1:30: Unknown column [missing]
line 1:43: Unknown column [not_found]
line 1:23: Unknown column [avg]""", error("from test | stats c = avg by missing + 1, not_found"));
}

I'm asking this because the RRF test sends many repeated errors (Because each "subblan" of the RRF has the same source). It shouldn't happen here at all, but just as a double-check

@kanoshiou
Copy link
Contributor Author

Thank you for reviewing @ivancea @ioanatia !😊

The branch has been updated. Please take a look when you have a moment.

@ivancea
Copy link
Contributor

ivancea commented Mar 18, 2025

buildkite test this

Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Thanks a lot @kanoshiou ! This LGTM. I only have a final test suggestion that I think we should implement before merging this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add test cases where an aggregate function ends up in the ROW command. The following currently gives 500:

row language_code = count(2)

While supremely paranoid, we could also throw in test cases for dissect and grok, like row x = 1 | dissect avg(1) "foo" and similarly with grok. (This doesn't give 500 on current main.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing! I’ve added the tests in c7d867c.

…-outside-STATS

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Thanks @kanoshiou , let's :shipit: !

@ivancea ivancea added v8.19.0 v9.0.1 auto-backport Automatically create backport pull requests when merged labels Mar 21, 2025
@ivancea
Copy link
Contributor

ivancea commented Mar 21, 2025

buildkite test this

Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

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

🚀

@ivancea ivancea merged commit 1d6a77c into elastic:main Mar 21, 2025
18 of 19 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts
9.0 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 124446

ivancea pushed a commit to ivancea/elasticsearch that referenced this pull request Mar 21, 2025
ivancea pushed a commit to ivancea/elasticsearch that referenced this pull request Mar 21, 2025
ivancea added a commit that referenced this pull request Mar 21, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…regate` (#125400)

Manual 8.x backport of #124446, with support for Metrics aggs and removed Dedup support
ivancea added a commit that referenced this pull request Mar 21, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…regate` (#125402)

Manual 9.0 backport of #124446, with support for Metrics aggs and removed Dedup support
smalyshev pushed a commit to smalyshev/elasticsearch that referenced this pull request Mar 21, 2025

Verified

This commit was signed with the committer’s verified signature.
smalyshev Stanislav Malyshev
…te` (elastic#124446)
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-backport Automatically create backport pull requests when merged >bug external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.0.1 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESQL: Verifier failure to catch aggregation functions outside STATS command
6 participants