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: FoldNull folding aggs into literals, raising an "unknown agg" error #110257

Open
ivancea opened this issue Jun 28, 2024 · 6 comments
Open
Assignees
Labels
:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@ivancea
Copy link
Contributor

ivancea commented Jun 28, 2024

When an aggregation is folded by FoldNull, it's converted to something like STATS null, which leads to an error.

Reproduced by using adding a null param to any agg that won't surrogate based on that param being foldable.
Example:

ROW a = 5
| STATS percentile(a, NULL)

Result:

{
  "type": "esql_illegal_argument_exception",
  "reason": "unknown agg: class org.elasticsearch.xpack.esql.core.expression.Literal: null"
}
@ivancea ivancea added >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Jun 28, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@ivancea
Copy link
Contributor Author

ivancea commented Jun 28, 2024

Related with #100634
I'm closing as duplicate. It's not exactly the same issue I think, but it's part of it for sure

@ivancea ivancea closed this as not planned Won't fix, can't repro, duplicate, stale Jun 28, 2024
@luigidellaquila
Copy link
Contributor

I'm not sure we should close this, #100634 is a request for enhancements while this one is an actual bug (error 500).
Waiting for #100634 to be fully supported, we should at least add some validation for these cases.

Interestingly, median(null) works just fine, while percentile(null, 50) throws an error

@alex-spies
Copy link
Contributor

median(const) is already implemented, while percentile still needs a corresponding MV_... function to act as surrogate.

Not sure it's worth adding a validation for this, as implementing something like MV_PERCENTILE (but not exposing it to users, internal usage only) may not be a whole lot more work.

@ivancea
Copy link
Contributor Author

ivancea commented Jun 28, 2024

I'm 50/50 here really. The aggs that don't fail, is because they have the surrogate, indeed. However, any agg with multiple parameters, that have a null in one of them but a non-foldable in the other, will fail.

For example, percentile can't really be fixed. We can't convert PERCENTILE(field, null) to MV_PERCENTILE, as field is not foldable (At least not with the current methods, afaik). And it fails too.

So, we can reopen this if you think it's interesting

@alex-spies
Copy link
Contributor

Ah, I was thinking about percentile(null, 50), but this is indeed a different problem than percentile(some_field, null). The latter indeed should end up with an error thrown at validation time.

Re-opening, but I think we need to make the wording of this issue more precise.

@alex-spies alex-spies reopened this Jun 28, 2024
@astefan astefan self-assigned this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

No branches or pull requests

5 participants