[ES|QL] Remove variadic functions' optional args from the output of show functions #106454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…argNames and argTypes for variadic functions
|
Documentation preview: |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
astefan
approved these changes
Mar 19, 2024
There was a problem hiding this comment.
Thank you @fang-xing-esql. It looks much better. And it's a LGTM.
There is one more thing we can improve and make things more consistent: at the moment the naming of all the arguments is inconsistent.
For example, concat has string1 and string2 (which is ok) but other string manipulating functions have str and newStr.
Most of the numerical arguments are called n, but there are functions (min, max...) that receive numerical arguments called field.
There are also a lot of function that have a single argument called v.
Can you please have these consistent in some form? For example:
- those arguments that are strings let's call them
string,newString,string2etc - all the string numerical arguments let's call them
number - all the generic arguments, maybe
field
I am open to suggestions, as long as things are consistent.
dej611
reviewed
Mar 19, 2024
.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java
Outdated
Show resolved
Hide resolved
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java
Show resolved
Hide resolved
This was referenced Mar 25, 2024
2 tasks
drewdaemon
added a commit
to elastic/kibana
that referenced
this pull request
May 24, 2024
…79584) ## Summary Resolve #179634 This PR introduces a script to gather metadata from Elasticsearch and build the function definitions for Kibana's client-side validation. To run the script: `cd packages/kbn-esql-validation-autocomplete && yarn makedefs path/to/elasticsearch/repo` ## Current limitations ### Things we can't (yet) gather from ES #### Things that are currently being filled in on the Kibana side but would be missing if we had to rely completely on ES - function aliases (e.g. `to_str`) - operators and aggs (issues for Elasticsearch elastic/elasticsearch#107219 and elastic/elasticsearch#107220) - Kibana AST parameter settings - `constantOnly` — whether a parameter can be a non-literal or not (`percentile`, `auto_bucket`) - `constantOptions`/`constantSuggestions` — specific information about which constants are accepted or suggested for particular parameters. - `supportsWildcard` — whether a parameter can contain `*` (only `count` at the moment) - `noNestingFunctions` — whether a parameter can contain nested functions (used for all agg functions) - Kibana date constant types - `chrono_literal` - `time_literal` ### Other considerations - Asciidoc links need to be scrubbed (e.g. `Function {wikipedia}/foo/bar[arccosine] returns blah blah`) - As of elastic/elasticsearch#106454 some descriptions reference outdated parameter names and some parameter names are questionable: <img width="600" alt="Screenshot 2024-03-28 at 1 07 16 PM" src="https://github.com/elastic/kibana/assets/315764/3b9ab457-e9fd-4c5f-afb2-23ae769bb44a"> _parameter `n` is now called `number`, but the description still calls it `n`... also, is `number` really the best name for the variable?_ ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [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 --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolve #106346
The optional arguments of variadic functions are removed from optionalArgs, argNames and argTypes in the output of show functions, and the outputs are shown below.
The full signature of the functions are kept unchanged, with the optional arguments included, so that we can find the information of the optional arguments there. If needed the full signature can be changed too.