-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
ES|QL: add tests for NaN on BUCKET function #110380
Conversation
Pinging @elastic/es-analytical-engine (Team:Analytics) |
There was a problem hiding this 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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking Ivan.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
Closes #105166
Adding tests that verify that
BUCKET
(previouslyAUTO_BUCKET
) function does not returnNaN
when an invalid number of buckets is provided (eg. 0, -1 or a very large integer)