ESQL: Improve verifier error for incorrect agg declaration #100650
Conversation
Add specific check for aliased function declarations in stats Fix elastic#100634
costin
added
>bug
auto-merge
costin
requested review from
nik9000,
astefan,
bpintea,
alex-spies and
luigidellaquila
and removed request for
nik9000
elasticsearchmachine
added
the
Team:QL (Deprecated)
|
Hi @costin, I've created a changelog YAML for you. |
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
costin
added
ES|QL-ui
|
Hi @costin, I've updated the changelog YAML for you. |
There was a problem hiding this comment.
LGTM even though in some cases it could be misleading, eg ... | stats a = foo will result in incomplete aggregate function declaration; add parenthesis to [foo], and when the user tries to add the parentheses ... | stats a = foo() he will just get another error.
Maybe we could be more generic and just say invalid stats declaration; [{}] is not an aggregate function.
As a future evolution, we could also use the FunctionRegistry to suggest alternatives, like did you mean xyz()?
There was a problem hiding this comment.
Imho, I believe we are too picky with the error messages improvements. Yes, we can do better in some situations but there will always be users who will complain about the message.
And @luigidellaquila's example is very good in pointing out another of these edge situations where we could do better.
Another one is stats c = avg(, where one could argue that the language should have a better error message than no viable alternative at input 'avg(' (ie "why isn't the language telling me that I should add another parenthesis?").
In any case, the improvement in this PR is welcomed, BUT I suggest incorporating the fix that @luigidellaquila mentioned in this PR not in a new one.
| @@ -89,6 +91,19 @@ Collection<Failure> verify(LogicalPlan plan) { | |||
| else if (p.resolved()) { | |||
| return; | |||
| } | |||
| // handle aggregate first to disambiguate between missing fields or incorrect function declaration | |||
| if (p instanceof Aggregate aggregate) { | |||
| for (NamedExpression agg : aggregate.aggregates()) { | |||
There was a problem hiding this comment.
Should we maybe consolidate this with the verification happening below in lines 125/140? Now we're verifying aggs in two places.
There was a problem hiding this comment.
There are different types of verifications - the first one is around unknown fields the other around resolved fields but incorrectly declared.
…00650) Add specific check for aliased function declarations in stats Fix elastic#100634
💚 Backport successful
|
Add specific check for aliased function declarations in stats
Fix #100641