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

ES|QL: add tests for NaN on BUCKET function #110380

Merged

Conversation

luigidellaquila
Copy link
Contributor

Closes #105166

Adding tests that verify that BUCKET (previously AUTO_BUCKET) function does not return NaN when an invalid number of buckets is provided (eg. 0, -1 or a very large integer)

@luigidellaquila luigidellaquila added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL labels Jul 2, 2024
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0 labels Jul 2, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@luigidellaquila luigidellaquila changed the title ES|QL add tests for NaN on BUCKET function ES|QL: add tests for NaN on BUCKET function Jul 2, 2024
Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a comment about the solution itself, and if this is really the behaviour we want.
Apart of that, looks good

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead validate the params in the validate() method? They are required to be foldable.
It would be a breaking change I guess, so asking to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking Ivan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on this.
It doesn't seem a critical change (it's more a corner case than a real feature) but it could be considered a breaking one.

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Since we are adding tests to bucket, please also add foldable expressions tests:

FROM employees | WHERE hire_date >= "1985-01-01T00:00:00Z" AND hire_date < "1986-01-01T00:00:00Z" | EVAL c = concat("2","0")::int | STATS hires_per_month = COUNT(*) BY month = BUCKET(hire_date, c, "1985-01-01T00:00:00Z", "1986-01-01T00:00:00Z") | SORT month

and

FROM employees | WHERE hire_date >= "1985-01-01T00:00:00Z" AND hire_date < "1986-01-01T00:00:00Z" | STATS hires_per_month = COUNT(*) BY month = BUCKET(hire_date, concat("2","0")::int, "1985-01-01T00:00:00Z", "1986-01-01T00:00:00Z") | SORT month

@luigidellaquila luigidellaquila added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jul 2, 2024
@elasticsearchmachine elasticsearchmachine merged commit afbd568 into elastic:main Jul 3, 2024
15 checks passed
@luigidellaquila luigidellaquila deleted the esql/tests_for_105166 branch July 3, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESQL: AUTO_BUCKET() can return NaN
4 participants