Skip to content

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

Merged
elasticsearchmachine merged 3 commits intoelastic:mainfrom
luigidellaquila:esql/tests_for_105166
Jul 3, 2024
Merged

ES|QL: add tests for NaN on BUCKET function#110380
elasticsearchmachine merged 3 commits intoelastic:mainfrom
luigidellaquila:esql/tests_for_105166

Conversation

@luigidellaquila
Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
Member 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
Copy Markdown
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
@luigidellaquila luigidellaquila deleted the esql/tests_for_105166 branch July 3, 2024 07:19
tvernum pushed a commit that referenced this pull request Feb 25, 2025
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)
tvernum pushed a commit that referenced this pull request Feb 25, 2025
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)
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