-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
ESQL: Fail in AggregateFunction
when LogicPlan
is not an Aggregate
#124446
Conversation
Pinging @elastic/es-analytical-engine (Team:Analytics) |
buildkite test this |
There was a problem hiding this 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!
.../main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AggregateFunction.java
Show resolved
Hide resolved
There was a problem hiding this comment.
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:
Lines 1010 to 1015 in a8be56f
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
buildkite test this |
There was a problem hiding this 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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this 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 !
buildkite test this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
💔 Backport failed
You can use sqren/backport to manually backport by running |
Trigger
Verifier
failure inAggregateFunction
if theLogicPlan
is not anAggregate
.Closes #124311