elasticsearch
elasticsearch copied to clipboard
ESQL: extend Validatable support
- Moved
TopListagg foldable validations toValidatable. Same for other functions doing the same. - Add
Validatablesupport in function tests - Add
Validatableto functions documentation - Fixed
BucketValidatableimplementation to not fail test cases withnullparameters
Do not merge
Waiting for https://github.com/elastic/elasticsearch/issues/100634
Pinging @elastic/es-analytical-engine (Team:Analytics)
I don't think the changes in this PR should be done without also considering the rest of the hypothetical work in this area #100634. Instead I would add this task of extending the aggregations testing infrastructure to the issue above and to be done after the other items are finished.
@astefan Indeed, looks like something that complements that issue. However, I would still go with this, for some reasons:
- validate() was an existing thing, currently used in other 2 functions. So I consider this adding missing testing support to an existing feature
- I've seen this other PR that could benefit from testing validations. So the sooner we add this, the sooner we will be able to have such tests "standardized"
- This PR is removing the possibility to check foldability in
resolveType(). So we won't have to migrate more things in the future - I don't see a clear order requirement with that issue. It would make sense to have made this at the end, but also I don't see a reason to not just merge this now and have it ready
I don't think the changes in this PR should be done without also considering the rest of the hypothetical work in this area #100634. Instead I would add this task of extending the aggregations testing infrastructure to the issue above and to be done after the other items are finished.
@astefan Indeed, looks like something that complements that issue. However, I would still go with this, for some reasons:
validate() was an existing thing, currently used in other 2 functions. So I consider this adding missing testing support to an existing feature
- I've seen this other PR that could benefit from testing validations. So the sooner we add this, the sooner we will be able to have such tests "standardized"
This PR is removing the possibility to check foldability in
resolveType(). So we won't have to migrate more things in the futureI don't see a clear order requirement with that issue. It would make sense to have made this at the end, but also I don't see a reason to not just merge this now and have it ready
I don't feel comfortable adding code (this PR) for something that's not yet fixed in its entirety (the whole story of constants support in aggregations, with adjusting the optimization rule(s) if needed, implementing this support for all of them). This PR just assumes the future will look in a certain way and it's getting ahead of it. It's a more unitary work that doesn't let room for unnecessary/unused code if the future turns out to be different.