-
Notifications
You must be signed in to change notification settings - Fork 25.7k
ESQL: add scalb function #127696
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
ESQL: add scalb function #127696
Conversation
|
💚 CLA has been signed |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
luigidellaquila
left a comment
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 @shmuelhanoch
I'm adding here some notes about what we discussed off-line
x-pack/plugin/esql/qa/testFixtures/src/main/resources/floats.csv-spec
Outdated
Show resolved
Hide resolved
...n/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java
Show resolved
Hide resolved
...n/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Scalb.java
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/floats.csv-spec
Outdated
Show resolved
Hide resolved
...n/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Scalb.java
Outdated
Show resolved
Hide resolved
|
Please also add some tests for pathological cases, like very large numbers and null values. |
d195b7a to
f0509dc
Compare
1b913c2 to
0ba09bf
Compare
5b8b482 to
e9a5411
Compare
e9a5411 to
fe3b312
Compare
fe3b312 to
65d1212
Compare
741461d to
96896de
Compare
luigidellaquila
left a comment
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 @shmuelhanoch
One more thing about docs: please add the function also to
https://github.com/elastic/elasticsearch/blob/main/docs/reference/query-languages/esql/_snippets/lists/math-functions.md
and
https://github.com/elastic/elasticsearch/blob/main/docs/reference/query-languages/esql/functions-operators/math-functions.md
@craigtaverner we need both with the new docs, right...?
96896de to
5f64606
Compare
5f64606 to
f1f939a
Compare
luigidellaquila
left a comment
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 @shmuelhanoch, I think we are almost there. Just one small thing about a csv test
x-pack/plugin/esql/qa/testFixtures/src/main/resources/floats.csv-spec
Outdated
Show resolved
Hide resolved
f1f939a to
e68b48e
Compare
e68b48e to
26448d4
Compare
luigidellaquila
left a comment
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, thanks!
|
Hi @luigidellaquila @shmuelhanoch can this be backported to 8.19.0? |
|
@tylerperk I'll take care of it |
Co-authored-by: Shmuel Hanoch <[email protected]>
|
Manual backport here #128815 |
Co-authored-by: shmuelhanoch <[email protected]> Co-authored-by: Shmuel Hanoch <[email protected]>
|
@shmuelhanoch @luigidellaquila don't forget that nowadays we need to add applies_to tags for all docs changes (example), because we don't publish on separate minor branches. Can you confirm:
I'll open PR to fix :-) |
* Add applies to to ScalB function in elastic#127696 * Add applies_to to categorize, follow up to elastic#129398 * Add version info, following elastic#127629 * SAMPLE is new + GA in 9.1 elastic#127629 * add applies to for 9.2 option (cherry picked from commit 5d565b5) # Conflicts: # docs/reference/query-languages/esql/_snippets/functions/parameters/categorize.md # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Categorize.java
* Add applies to to ScalB function in #127696 * Add applies_to to categorize, follow up to #129398 * Add version info, following #127629 * SAMPLE is new + GA in 9.1 #127629 * add applies to for 9.2 option (cherry picked from commit 5d565b5) # Conflicts: # docs/reference/query-languages/esql/_snippets/functions/parameters/categorize.md # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Categorize.java
* Add applies to to ScalB function in elastic#127696 * Add applies_to to categorize, follow up to elastic#129398 * Add version info, following elastic#127629 * SAMPLE is new + GA in 9.1 elastic#127629 * add applies to for 9.2 option
Part of #98545.