elasticsearch icon indicating copy to clipboard operation
elasticsearch copied to clipboard

ESQL: extend Validatable support

Open ivancea opened this issue 1 year ago • 3 comments

  • Moved TopList agg foldable validations to Validatable. Same for other functions doing the same.
  • Add Validatable support in function tests
  • Add Validatable to functions documentation
  • Fixed Bucket Validatable implementation to not fail test cases with null parameters

Do not merge

Waiting for https://github.com/elastic/elasticsearch/issues/100634

ivancea avatar Jun 27 '24 15:06 ivancea

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

elasticsearchmachine avatar Jun 27 '24 15:06 elasticsearchmachine

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

ivancea avatar Jun 28 '24 11:06 ivancea

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 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.

astefan avatar Jun 28 '24 11:06 astefan