Skip to content

Commit

Permalink
[ES|QL] capitalize function and operator signatures (elastic#189109)
Browse files Browse the repository at this point in the history
## Summary

This is a follow-up to elastic#186340. I
forgot to capitalize the labels for functions and operators. Fixed now.

**Before**
<img width="1321" alt="Screenshot 2024-07-24 at 10 26 52 AM"
src="https://github.com/user-attachments/assets/05102828-0469-4b14-a2f9-34a160edbd18">

**After**
<img width="1286" alt="Screenshot 2024-07-24 at 10 23 59 AM"
src="https://github.com/user-attachments/assets/dd01ccde-ec12-4ad3-98f7-561e5d233e52">

Also, this PR changes the validation error messages to actually reflect
what the user has typed:

**Before**
<img width="697" alt="Screenshot 2024-07-24 at 11 43 54 AM"
src="https://github.com/user-attachments/assets/28cbbd4c-1b3e-4d22-a1af-33b2f17af597">

**After**
<img width="709" alt="Screenshot 2024-07-24 at 11 40 48 AM"
src="https://github.com/user-attachments/assets/dffd4a29-d348-44d0-ba58-00a0a70d04b3">
<img width="663" alt="Screenshot 2024-07-24 at 11 39 54 AM"
src="https://github.com/user-attachments/assets/dff0c726-d028-4826-a1cd-030e733987a7">

### Checklist

Delete any items that are not applicable to this PR.

- [x] 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

(cherry picked from commit 05b6d7e)

# Conflicts:
#	packages/kbn-esql-validation-autocomplete/src/autocomplete/__tests__/autocomplete.command.stats.test.ts
#	packages/kbn-esql-validation-autocomplete/src/autocomplete/__tests__/helpers.ts
  • Loading branch information
drewdaemon committed Jul 25, 2024
1 parent 6fa139b commit 218d058
Show file tree
Hide file tree
Showing 7 changed files with 129 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@ describe('autocomplete', () => {
...getFieldNamesByType('string').map((v) => `${v},`),
...getFunctionSignaturesByReturnType('eval', 'string', { evalMath: true }, undefined, [
'concat',
]).map((v) => `${v},`),
]).map((v) => ({ ...v, text: `${v.text},` })),
]);
testSuggestions('from a | eval a=concat(stringField, ', [
...getFieldNamesByType('string'),
Expand All @@ -1124,7 +1124,7 @@ describe('autocomplete', () => {
...getFieldNamesByType('ip').map((v) => `${v},`),
...getFunctionSignaturesByReturnType('eval', 'ip', { evalMath: true }, undefined, [
'cidr_match',
]).map((v) => `${v},`),
]).map((v) => ({ ...v, text: `${v.text},` })),
]);
testSuggestions('from a | eval a=cidr_match(ipField, ', [
...getFieldNamesByType('string'),
Expand Down Expand Up @@ -1289,7 +1289,7 @@ describe('autocomplete', () => {
...getLiteralsByType('time_literal').map((t) => `${t},`),
...getFunctionSignaturesByReturnType('eval', 'date', { evalMath: true }, undefined, [
'date_trunc',
]).map((t) => `${t},`),
]).map((t) => ({ ...t, text: `${t.text},` })),
...getFieldNamesByType('date').map((t) => `${t},`),
],
'('
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function getSafeInsertSourceText(text: string) {
}

export function getSuggestionFunctionDefinition(fn: FunctionDefinition): SuggestionRawDefinition {
const fullSignatures = getFunctionSignatures(fn);
const fullSignatures = getFunctionSignatures(fn, { capitalize: true, withTypes: true });
return {
label: fullSignatures[0].declaration,
text: `${fn.name.toUpperCase()}($0)`,
Expand All @@ -66,7 +66,7 @@ export function getSuggestionFunctionDefinition(fn: FunctionDefinition): Suggest
export function getSuggestionBuiltinDefinition(fn: FunctionDefinition): SuggestionRawDefinition {
const hasArgs = fn.signatures.some(({ params }) => params.length > 1);
return {
label: fn.name,
label: fn.name.toUpperCase(),
text: hasArgs ? `${fn.name.toUpperCase()} $0` : fn.name.toUpperCase(),
asSnippet: hasArgs,
kind: 'Operator',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,18 @@ import type { CommandDefinition, FunctionDefinition } from './types';
*/
export function getFunctionSignatures(
{ name, signatures }: FunctionDefinition,
{ withTypes }: { withTypes: boolean } = { withTypes: true }
{ withTypes, capitalize }: { withTypes: boolean; capitalize?: boolean } = {
withTypes: true,
capitalize: false,
}
) {
return signatures.map(({ params, returnType, minParams }) => {
// for functions with a minimum number of args, repeat the last arg multiple times
// just make sure to compute the right number of args to add
const minParamsToAdd = Math.max((minParams || 0) - params.length, 0);
const extraArg = Array(minParamsToAdd || 1).fill(params[Math.max(params.length - 1, 0)]);
return {
declaration: `${name}(${params
declaration: `${capitalize ? name.toUpperCase() : name}(${params
.map((arg) => printArguments(arg, withTypes))
.join(', ')}${handleAdditionalArgs(minParamsToAdd > 0, extraArg, withTypes)})${
withTypes ? `: ${returnType}` : ''
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,13 +290,12 @@ export function areFieldAndVariableTypesCompatible(
return fieldType === variableType;
}

export function printFunctionSignature(arg: ESQLFunction, useCaps = true): string {
export function printFunctionSignature(arg: ESQLFunction): string {
const fnDef = getFunctionDefinition(arg.name);
if (fnDef) {
const signature = getFunctionSignatures(
{
...fnDef,
name: useCaps ? fnDef.name.toUpperCase() : fnDef.name,
signatures: [
{
...fnDef?.signatures[0],
Expand All @@ -313,7 +312,7 @@ export function printFunctionSignature(arg: ESQLFunction, useCaps = true): strin
},
],
},
{ withTypes: false }
{ withTypes: false, capitalize: true }
);
return signature[0].declaration;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9187,8 +9187,8 @@
{
"query": "from a_index | eval date_diff(\"year\", concat(\"20\", \"22\"), concat(\"20\", \"22\"))",
"error": [
"Argument of [date_diff] must be [date], found value [concat(\"20\", \"22\")] type [string]",
"Argument of [date_diff] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [date_diff] must be [date], found value [concat(\"20\",\"22\")] type [string]",
"Argument of [date_diff] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand Down Expand Up @@ -11080,7 +11080,7 @@
{
"query": "from a_index | eval date_extract(\"ALIGNED_DAY_OF_WEEK_IN_MONTH\", concat(\"20\", \"22\"))",
"error": [
"Argument of [date_extract] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [date_extract] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand Down Expand Up @@ -11167,7 +11167,7 @@
{
"query": "from a_index | eval date_format(stringField, concat(\"20\", \"22\"))",
"error": [
"Argument of [date_format] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [date_format] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand Down Expand Up @@ -11384,7 +11384,7 @@
{
"query": "from a_index | eval date_trunc(1 year, concat(\"20\", \"22\"))",
"error": [
"Argument of [date_trunc] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [date_trunc] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand All @@ -11396,8 +11396,8 @@
{
"query": "from a_index | eval date_trunc(concat(\"20\", \"22\"), concat(\"20\", \"22\"))",
"error": [
"Argument of [date_trunc] must be [time_literal], found value [concat(\"20\", \"22\")] type [string]",
"Argument of [date_trunc] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [date_trunc] must be [time_literal], found value [concat(\"20\",\"22\")] type [string]",
"Argument of [date_trunc] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand Down Expand Up @@ -24139,7 +24139,7 @@
{
"query": "from a_index | stats max(concat(\"20\", \"22\"))",
"error": [
"Argument of [max] must be [number], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [max] must be [number], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand Down Expand Up @@ -24412,7 +24412,7 @@
{
"query": "from a_index | stats min(concat(\"20\", \"22\"))",
"error": [
"Argument of [min] must be [number], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [min] must be [number], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand Down Expand Up @@ -24970,14 +24970,14 @@
{
"query": "from a_index | stats bucket(concat(\"20\", \"22\"), 1 year)",
"error": [
"Argument of [bucket] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [bucket] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
{
"query": "from a_index | stats by bucket(concat(\"20\", \"22\"), 1 year)",
"error": [
"Argument of [bucket] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [bucket] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand All @@ -24989,7 +24989,7 @@
{
"query": "from a_index | stats bucket(concat(\"20\", \"22\"), 5, \"a\", \"a\")",
"error": [
"Argument of [bucket] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [bucket] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand All @@ -25001,7 +25001,7 @@
{
"query": "from a_index | stats bucket(concat(\"20\", \"22\"), 5, concat(\"20\", \"22\"), concat(\"20\", \"22\"))",
"error": [
"Argument of [bucket] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [bucket] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand All @@ -25013,7 +25013,7 @@
{
"query": "from a_index | stats bucket(concat(\"20\", \"22\"), 5, \"a\", concat(\"20\", \"22\"))",
"error": [
"Argument of [bucket] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [bucket] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand All @@ -25025,7 +25025,7 @@
{
"query": "from a_index | stats bucket(concat(\"20\", \"22\"), 5, concat(\"20\", \"22\"), \"a\")",
"error": [
"Argument of [bucket] must be [date], found value [concat(\"20\", \"22\")] type [string]"
"Argument of [bucket] must be [date], found value [concat(\"20\",\"22\")] type [string]"
],
"warning": []
},
Expand Down Expand Up @@ -26368,6 +26368,91 @@
"error": [],
"warning": []
},
{
"query": "row var = exp(5)",
"error": [],
"warning": []
},
{
"query": "row exp(5)",
"error": [],
"warning": []
},
{
"query": "row var = exp(to_integer(true))",
"error": [],
"warning": []
},
{
"query": "row var = exp(true)",
"error": [
"Argument of [exp] must be [number], found value [true] type [boolean]"
],
"warning": []
},
{
"query": "from a_index | where exp(numberField) > 0",
"error": [],
"warning": []
},
{
"query": "from a_index | where exp(booleanField) > 0",
"error": [
"Argument of [exp] must be [number], found value [booleanField] type [boolean]"
],
"warning": []
},
{
"query": "from a_index | eval var = exp(numberField)",
"error": [],
"warning": []
},
{
"query": "from a_index | eval exp(numberField)",
"error": [],
"warning": []
},
{
"query": "from a_index | eval var = exp(to_integer(booleanField))",
"error": [],
"warning": []
},
{
"query": "from a_index | eval exp(booleanField)",
"error": [
"Argument of [exp] must be [number], found value [booleanField] type [boolean]"
],
"warning": []
},
{
"query": "from a_index | eval var = exp(*)",
"error": [
"Using wildcards (*) in exp is not allowed"
],
"warning": []
},
{
"query": "from a_index | eval exp(numberField, extraArg)",
"error": [
"Error: [exp] function expects exactly one argument, got 2."
],
"warning": []
},
{
"query": "from a_index | sort exp(numberField)",
"error": [],
"warning": []
},
{
"query": "from a_index | eval exp(null)",
"error": [],
"warning": []
},
{
"query": "row nullVar = null | eval exp(nullVar)",
"error": [],
"warning": []
},
{
"query": "f",
"error": [
Expand Down
Loading

0 comments on commit 218d058

Please sign in to comment.