Skip to content

[ES|QL] Add function log(base, value)#104913

Merged
fang-xing-esql merged 25 commits intoelastic:mainfrom
fang-xing-esql:add-function-log
Feb 6, 2024
Merged

[ES|QL] Add function log(base, value)#104913
fang-xing-esql merged 25 commits intoelastic:mainfrom
fang-xing-esql:add-function-log

Conversation

@fang-xing-esql
Copy link
Copy Markdown
Member

@fang-xing-esql fang-xing-esql commented Jan 30, 2024

A scalar function log(based, value) returns the logarithm of a value for a particular base, as specified in the argument.

The arguments can be integer, long integer, unsigned long integer or double. If the arguments are not double, they are converted to one for processing by the function.

The result of the function is a double number.

The result can be null; if the argument is null, the result is the null value.

fang-xing-esql and others added 12 commits January 17, 2024 18:23
@fang-xing-esql
Allow both string and datetime as the third and fourth inputs to auto…
06d124d 
…_bucket

 Committer: Fang Xing <fang.xing@elastic.co>
@fang-xing-esql
Merge branch 'elastic:main' into main
dea8da4
@fang-xing-esql
Allow both string and datetime as the third and fourth inputs to auto…
f5c25c5 
…_bucket
@fang-xing-esql
Allow both string and datetime as the third and fourth inputs to auto…
57025ef 
…_bucket
@fang-xing-esql
Allow both string and datetime as the third and fourth inputs to auto…
663b5c6 
…_bucket
@fang-xing-esql
Allow both string and datetime as the third and fourth inputs to auto…
6a19d6e 
…_bucket
@fang-xing-esql
Allow both string and datetime as the third and fourth inputs to auto…
5d21491 
…_bucket
@fang-xing-esql
Merge branch 'elastic:main' into main
7f5b96b
@fang-xing-esql
Merge branch 'elastic:main' into main
762d03b
@fang-xing-esql
Merge branch 'elastic:main' into main
b95e792
@fang-xing-esql
Merge branch 'elastic:main' into main
40d6ff6
@fang-xing-esql
add a new scalar function log
e8eedae
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 30, 2024

Documentation preview:

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Jan 30, 2024

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

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 30, 2024
luigidellaquila
luigidellaquila reviewed Jan 30, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

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

Thank you very much @fang-xing-esql, it looks pretty good.
Kudos for the test coverage.
I added a few comments, mostly for possible improvements

...gin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Log.java Outdated
}

@Evaluator(warnExceptions = { ArithmeticException.class })
static double process(double base, double value) {
Copy link
Copy Markdown
Member

@luigidellaquila luigidellaquila Jan 30, 2024

Choose a reason for hiding this comment

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

In most of the cases the base will likely be a constant, so probably it makes sense to have a process(double value) method and a dedicated (and optimized) evaluator. See this as an example

Copy link
Copy Markdown
Member Author

@fang-xing-esql fang-xing-esql Jan 31, 2024

Choose a reason for hiding this comment

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

Discussed with Luigi, this enhancement can be deferred to a follow-up PR.

astefan
astefan reviewed Jan 30, 2024
View reviewed changes
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.

Please, add some csv-spec tests that use actual data from indices.
For example:

from employees | EVAL s = LOG(languages, salary) | keep emp_no, salary, s | limit 10
from employees | eval x = salary * 2 | eval s = log(x, x) | keep emp_no, salary, s | limit 10
from employees | rename languages as base | eval s = log(base, salary) | keep emp_no, salary, s | limit 10 | sort s nulls last
from employees | eval base = 2.5 | eval s = log(base, salary) * 0.5 | keep emp_no, salary, s | sort s nulls first | limit 5

@fang-xing-esql fang-xing-esql changed the title Add function log(base, value) [ES|QL] Add function log(base, value) Jan 30, 2024
@fang-xing-esql
Copy link
Copy Markdown
Member Author

fang-xing-esql commented Jan 31, 2024

Please, add some csv-spec tests that use actual data from indices. For example:

from employees | EVAL s = LOG(languages, salary) | keep emp_no, salary, s | limit 10
from employees | eval x = salary * 2 | eval s = log(x, x) | keep emp_no, salary, s | limit 10
from employees | rename languages as base | eval s = log(base, salary) | keep emp_no, salary, s | limit 10 | sort s nulls last
from employees | eval base = 2.5 | eval s = log(base, salary) * 0.5 | keep emp_no, salary, s | sort s nulls first | limit 5

More csv tests on indices are added.

astefan
astefan approved these changes Feb 5, 2024
View reviewed changes
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.

Left a small cosmetic comment, otherwise LGTM. Well done!

luigidellaquila
luigidellaquila approved these changes Feb 5, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dej611 dej611 mentioned this pull request Feb 6, 2024
Merged
1 task
@fang-xing-esql fang-xing-esql merged commit 0fb5dee into elastic:main Feb 6, 2024
dej611 added a commit to elastic/kibana that referenced this pull request Feb 9, 2024
@dej611
[ES|QL] New sync with ES changes (#176283)
7bbea56 
## Summary

Sync with elastic/elasticsearch#104958 for
support of builtin fn in STATS
  * validation ✅ 
  * autocomplete ✅ 
  * also fixed `STATS BY <field>` syntax


![new_stats](https://github.com/elastic/kibana/assets/924948/735f9842-b1d3-4aa0-9d51-4b2f9b136ed3)


Sync with elastic/elasticsearch#104913 for new
`log` function
  * validation ✅  - also warning for negative values
  * autocomplete ✅ 

![add_log](https://github.com/elastic/kibana/assets/924948/146b945d-a23b-45ec-9df2-2d2b291e883b)

Sync with elastic/elasticsearch#105064 for
removal of `PROJECT` command
  * validation ✅  (both new and legacy syntax supported)
  * autocomplete ✅  (will only suggest new syntax)


![remove_project](https://github.com/elastic/kibana/assets/924948/b6f40afe-a26d-4917-b7a1-d8ae97c5368b)

Sync with elastic/elasticsearch#105221 for
removal of mandatory brackets for `METADATA` command option
* validation ✅ (added warning deprecation message when using brackets)
  * autocomplete ✅ 

![fix_metadata](https://github.com/elastic/kibana/assets/924948/c65db176-dd94-45f3-9524-45453e62f51a)


Sync with elastic/elasticsearch#105224 for
change of syntax for ENRICH ccq mode
  * validation ✅ 
* autocomplete ✅ (not directly promoted, the user has to type `_` to
trigger it)
  * hover ✅ 
  * code actions ✅ 

![fix_ccq_enrich](https://github.com/elastic/kibana/assets/924948/0900edd9-a0a7-4ac8-bc12-e39a72359984)

![fix_ccq_enrich_2](https://github.com/elastic/kibana/assets/924948/74b0908f-d385-4723-b3d4-c09108f47a73)


Do not merge until those 5 get merged.

Additional things in this PR:
* Added more tests for `callbacks` not passed scenario
  * covered more cases like those with `dissect`
* Added more tests for signature params number (calling a function with
an extra arg should return an error)
* Cleaned up some more unused code
* Improved messages on too many arguments for functions

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
@fang-xing-esql fang-xing-esql deleted the add-function-log branch February 9, 2024 14:37
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@dej611 @CoenWarmer
[ES|QL] New sync with ES changes (elastic#176283)
731dfe9 
## Summary

Sync with elastic/elasticsearch#104958 for
support of builtin fn in STATS
  * validation ✅ 
  * autocomplete ✅ 
  * also fixed `STATS BY <field>` syntax


![new_stats](https://github.com/elastic/kibana/assets/924948/735f9842-b1d3-4aa0-9d51-4b2f9b136ed3)


Sync with elastic/elasticsearch#104913 for new
`log` function
  * validation ✅  - also warning for negative values
  * autocomplete ✅ 

![add_log](https://github.com/elastic/kibana/assets/924948/146b945d-a23b-45ec-9df2-2d2b291e883b)

Sync with elastic/elasticsearch#105064 for
removal of `PROJECT` command
  * validation ✅  (both new and legacy syntax supported)
  * autocomplete ✅  (will only suggest new syntax)


![remove_project](https://github.com/elastic/kibana/assets/924948/b6f40afe-a26d-4917-b7a1-d8ae97c5368b)

Sync with elastic/elasticsearch#105221 for
removal of mandatory brackets for `METADATA` command option
* validation ✅ (added warning deprecation message when using brackets)
  * autocomplete ✅ 

![fix_metadata](https://github.com/elastic/kibana/assets/924948/c65db176-dd94-45f3-9524-45453e62f51a)


Sync with elastic/elasticsearch#105224 for
change of syntax for ENRICH ccq mode
  * validation ✅ 
* autocomplete ✅ (not directly promoted, the user has to type `_` to
trigger it)
  * hover ✅ 
  * code actions ✅ 

![fix_ccq_enrich](https://github.com/elastic/kibana/assets/924948/0900edd9-a0a7-4ac8-bc12-e39a72359984)

![fix_ccq_enrich_2](https://github.com/elastic/kibana/assets/924948/74b0908f-d385-4723-b3d4-c09108f47a73)


Do not merge until those 5 get merged.

Additional things in this PR:
* Added more tests for `callbacks` not passed scenario
  * covered more cases like those with `dissect`
* Added more tests for signature params number (calling a function with
an extra arg should return an error)
* Cleaned up some more unused code
* Improved messages on too many arguments for functions

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
@dej611 @fkanout
[ES|QL] New sync with ES changes (elastic#176283)
953209e 
## Summary

Sync with elastic/elasticsearch#104958 for
support of builtin fn in STATS
  * validation ✅ 
  * autocomplete ✅ 
  * also fixed `STATS BY <field>` syntax


![new_stats](https://github.com/elastic/kibana/assets/924948/735f9842-b1d3-4aa0-9d51-4b2f9b136ed3)


Sync with elastic/elasticsearch#104913 for new
`log` function
  * validation ✅  - also warning for negative values
  * autocomplete ✅ 

![add_log](https://github.com/elastic/kibana/assets/924948/146b945d-a23b-45ec-9df2-2d2b291e883b)

Sync with elastic/elasticsearch#105064 for
removal of `PROJECT` command
  * validation ✅  (both new and legacy syntax supported)
  * autocomplete ✅  (will only suggest new syntax)


![remove_project](https://github.com/elastic/kibana/assets/924948/b6f40afe-a26d-4917-b7a1-d8ae97c5368b)

Sync with elastic/elasticsearch#105221 for
removal of mandatory brackets for `METADATA` command option
* validation ✅ (added warning deprecation message when using brackets)
  * autocomplete ✅ 

![fix_metadata](https://github.com/elastic/kibana/assets/924948/c65db176-dd94-45f3-9524-45453e62f51a)


Sync with elastic/elasticsearch#105224 for
change of syntax for ENRICH ccq mode
  * validation ✅ 
* autocomplete ✅ (not directly promoted, the user has to type `_` to
trigger it)
  * hover ✅ 
  * code actions ✅ 

![fix_ccq_enrich](https://github.com/elastic/kibana/assets/924948/0900edd9-a0a7-4ac8-bc12-e39a72359984)

![fix_ccq_enrich_2](https://github.com/elastic/kibana/assets/924948/74b0908f-d385-4723-b3d4-c09108f47a73)


Do not merge until those 5 get merged.

Additional things in this PR:
* Added more tests for `callbacks` not passed scenario
  * covered more cases like those with `dissect`
* Added more tests for signature params number (calling a function with
an extra arg should return an error)
* Cleaned up some more unused code
* Improved messages on too many arguments for functions

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@luigidellaquila luigidellaquila luigidellaquila approved these changes

@astefan astefan astefan approved these changes

Assignees

No one assigned

Labels

:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.13.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fang-xing-esql @elasticsearchmachine @luigidellaquila @astefan