[ES|QL] Improve handling of functions for variadic signatures with mi…

…nParams (elastic#177165) ## Summary Fix an issue with functions with the `minParams` configuration (`concat`, `case`, `cidr_match`). Validation fix: * now validation engine understands the minimum number of args and provide a better message based on the function signature * if the function has a single exact signature, then make it explicit the `exact` nature <img width="442" alt="Screenshot 2024-02-19 at 11 46 59" src="https://github.com/elastic/kibana/assets/924948/14aa9cb4-bce2-404b-936e-cb92ec966c2d"> <img width="227" alt="Screenshot 2024-02-19 at 11 45 18" src="https://github.com/elastic/kibana/assets/924948/94b2a051-5cd2-4f60-b65a-c3ac77c17b85"> * if the function has some optional args in the signature, then make it explicit that there are too few or too many args <img width="441" alt="Screenshot 2024-02-19 at 11 48 00" src="https://github.com/elastic/kibana/assets/924948/55acf5f7-ce6b-452d-ba49-cd38ac05120e"> <img width="443" alt="Screenshot 2024-02-19 at 11 45 03" src="https://github.com/elastic/kibana/assets/924948/653f62d3-7ee5-44d2-91e9-aae812f08394"> * if the function has a minParams configuration, then it should make it explicit that there are too few args: <img width="467" alt="Screenshot 2024-02-19 at 11 41 46" src="https://github.com/elastic/kibana/assets/924948/8a4030e0-317d-4371-abd0-11b333ad26d9"> <img width="441" alt="Screenshot 2024-02-19 at 11 41 16" src="https://github.com/elastic/kibana/assets/924948/d8f8048e-edda-44c2-b84e-b908cd7b29a5"> <img width="446" alt="Screenshot 2024-02-19 at 11 41 04" src="https://github.com/elastic/kibana/assets/924948/c6ac58ef-a1b7-48a6-b2ad-a8994ae2b885"> * autocomplete now understand the `minParams` and suggest the right value in the right place ![esql_min_params_autocomplete](https://github.com/elastic/kibana/assets/924948/fd21a819-8310-4f94-88e6-3a2967c7a8bd) * signature hover tooltip now provides a better signature for functions with minParams (arg should not be optional, rather mandatory until the #minParams, optional after) ![esql_min_params_signature](https://github.com/elastic/kibana/assets/924948/421f22f3-8d00-4acb-a65c-495a05b8d400) ### 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
e40pud · Feb 21, 2024 · eb708f5 · eb708f5
1 parent 41f5b45
commit eb708f5
Show file tree

Hide file tree

Showing 9 changed files with 277 additions and 102 deletions.
diff --git a/packages/kbn-monaco/src/esql/lib/ast/autocomplete/autocomplete.test.ts b/packages/kbn-monaco/src/esql/lib/ast/autocomplete/autocomplete.test.ts
@@ -975,6 +975,39 @@ describe('autocomplete', () => {
       ],
       '('
     );
+    // test that comma is correctly added to the suggestions if minParams is not reached yet
+    testSuggestions('from a | eval a=concat( ', [
+      ...getFieldNamesByType('string').map((v) => `${v},`),
+      ...getFunctionSignaturesByReturnType('eval', 'string', { evalMath: true }, undefined, [
+        'concat',
+      ]).map((v) => `${v},`),
+    ]);
+    testSuggestions('from a | eval a=concat(stringField, ', [
+      ...getFieldNamesByType('string'),
+      ...getFunctionSignaturesByReturnType('eval', 'string', { evalMath: true }, undefined, [
+        'concat',
+      ]),
+    ]);
+    // test that the arg type is correct after minParams
+    testSuggestions('from a | eval a=cidr_match(ipField, stringField,', [
+      ...getFieldNamesByType('string'),
+      ...getFunctionSignaturesByReturnType('eval', 'string', { evalMath: true }, undefined, [
+        'cidr_match',
+      ]),
+    ]);
+    // test that comma is correctly added to the suggestions if minParams is not reached yet
+    testSuggestions('from a | eval a=cidr_match( ', [
+      ...getFieldNamesByType('ip').map((v) => `${v},`),
+      ...getFunctionSignaturesByReturnType('eval', 'ip', { evalMath: true }, undefined, [
+        'cidr_match',
+      ]).map((v) => `${v},`),
+    ]);
+    testSuggestions('from a | eval a=cidr_match(ipField, ', [
+      ...getFieldNamesByType('string'),
+      ...getFunctionSignaturesByReturnType('eval', 'string', { evalMath: true }, undefined, [
+        'cidr_match',
+      ]),
+    ]);
     // test deep function nesting suggestions (and check that the same function is not suggested)
     // round(round(
     // round(round(round(

diff --git a/packages/kbn-monaco/src/esql/lib/ast/autocomplete/autocomplete.ts b/packages/kbn-monaco/src/esql/lib/ast/autocomplete/autocomplete.ts
@@ -173,7 +173,8 @@ export async function suggest(
       unclosedRoundBrackets === 0 &&
       getLastCharFromTrimmed(innerText) !== '_') ||
     (context.triggerCharacter === ' ' &&
-      (isMathFunction(innerText, offset) || isComma(innerText[offset - 2])))
+      (isMathFunction(innerText, offset) ||
+        isComma(innerText.trimEnd()[innerText.trimEnd().length - 1])))
   ) {
     finalText = `${innerText.substring(0, offset)}${EDITOR_MARKER}${innerText.substring(offset)}`;
   }
@@ -1078,17 +1079,23 @@ async function getFunctionArgsSuggestions(
     if (signature.params.length > argIndex) {
       return signature.params[argIndex].type;
     }
-    if (signature.infiniteParams) {
-      return signature.params[0].type;
+    if (signature.infiniteParams || signature.minParams) {
+      return signature.params[signature.params.length - 1].type;
     }
     return [];
   });
 
   const arg = node.args[argIndex];
 
+  // the first signature is used as reference
+  const refSignature = fnDefinition.signatures[0];
+
   const hasMoreMandatoryArgs =
-    fnDefinition.signatures[0].params.filter(({ optional }, index) => !optional && index > argIndex)
-      .length > argIndex;
+    refSignature.params.filter(({ optional }, index) => !optional && index > argIndex).length >
+      argIndex ||
+    ('minParams' in refSignature && refSignature.minParams
+      ? refSignature.minParams - 1 > argIndex
+      : false);
 
   const suggestions = [];
   const noArgDefined = !arg;

diff --git a/packages/kbn-monaco/src/esql/lib/ast/definitions/helpers.ts b/packages/kbn-monaco/src/esql/lib/ast/definitions/helpers.ts
@@ -12,12 +12,44 @@ export function getFunctionSignatures(
   { name, signatures }: FunctionDefinition,
   { withTypes }: { withTypes: boolean } = { withTypes: true }
 ) {
-  return signatures.map(({ params, returnType, infiniteParams, examples }) => ({
-    declaration: `${name}(${params.map((arg) => printArguments(arg, withTypes)).join(', ')}${
-      infiniteParams ? ` ,[... ${params.map((arg) => printArguments(arg, withTypes))}]` : ''
-    })${withTypes ? `: ${returnType}` : ''}`,
-    examples,
-  }));
+  return signatures.map(({ params, returnType, infiniteParams, minParams, examples }) => {
+    // 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 hasMoreOptionalArgs = !!infiniteParams || !!minParams;
+    const extraArg = Array(minParamsToAdd || 1).fill(params[Math.max(params.length - 1, 0)]);
+    return {
+      declaration: `${name}(${params
+        .map((arg) => printArguments(arg, withTypes))
+        .join(', ')}${handleAdditionalArgs(
+        minParamsToAdd > 0,
+        extraArg,
+        withTypes,
+        false
+      )}${handleAdditionalArgs(hasMoreOptionalArgs, extraArg, withTypes)})${
+        withTypes ? `: ${returnType}` : ''
+      }`,
+      examples,
+    };
+  });
+}
+
+function handleAdditionalArgs(
+  criteria: boolean,
+  additionalArgs: Array<{
+    name: string;
+    type: string | string[];
+    optional?: boolean;
+    reference?: string;
+  }>,
+  withTypes: boolean,
+  optionalArg: boolean = true
+) {
+  return criteria
+    ? `${withTypes && optionalArg ? ' ,[... ' : ', '}${additionalArgs
+        .map((arg) => printArguments(arg, withTypes))
+        .join(', ')}${withTypes && optionalArg ? ']' : ''}`
+    : '';
 }
 
 export function getCommandSignature(

diff --git a/packages/kbn-monaco/src/esql/lib/ast/shared/helpers.ts b/packages/kbn-monaco/src/esql/lib/ast/shared/helpers.ts
@@ -91,7 +91,7 @@ export function isIncompleteItem(arg: ESQLAstItem): boolean {
 }
 
 export function isMathFunction(query: string, offset: number) {
-  const queryTrimmed = query.substring(0, offset).trimEnd();
+  const queryTrimmed = query.trimEnd();
   // try to get the full operation token (e.g. "+", "in", "like", etc...) but it requires the token
   // to be spaced out from a field/function (e.g. "field + ") so it is subject to issues
   const [opString] = queryTrimmed.split(' ').reverse();

diff --git a/packages/kbn-monaco/src/esql/lib/ast/validation/errors.ts b/packages/kbn-monaco/src/esql/lib/ast/validation/errors.ts
@@ -57,14 +57,39 @@ function getMessageAndTypeFromId<K extends ErrorTypes>({
       };
     case 'wrongArgumentNumber':
       return {
-        message: i18n.translate('monaco.esql.validation.wrongArgumentNumber', {
+        message: i18n.translate('monaco.esql.validation.wrongArgumentExactNumber', {
           defaultMessage:
-            'Error building [{fn}]: expects {canHaveMoreArgs, plural, =0 {exactly } other {}}{numArgs, plural, one {one argument} other {{numArgs} arguments}}, passed {passedArgs} instead.',
+            'Error building [{fn}]: expects exactly {numArgs, plural, one {one argument} other {{numArgs} arguments}}, passed {passedArgs} instead.',
           values: {
             fn: out.fn,
             numArgs: out.numArgs,
             passedArgs: out.passedArgs,
-            canHaveMoreArgs: out.exactly,
+          },
+        }),
+      };
+    case 'wrongArgumentNumberTooMany':
+      return {
+        message: i18n.translate('monaco.esql.validation.wrongArgumentTooManyNumber', {
+          defaultMessage:
+            'Error building [{fn}]: expects {extraArgs, plural, =0 {} other {no more than }}{numArgs, plural, one {one argument} other {{numArgs} arguments}}, passed {passedArgs} instead.',
+          values: {
+            fn: out.fn,
+            numArgs: out.numArgs,
+            passedArgs: out.passedArgs,
+            extraArgs: out.extraArgs,
+          },
+        }),
+      };
+    case 'wrongArgumentNumberTooFew':
+      return {
+        message: i18n.translate('monaco.esql.validation.wrongArgumentTooFewNumber', {
+          defaultMessage:
+            'Error building [{fn}]: expects {missingArgs, plural, =0 {} other {at least }}{numArgs, plural, one {one argument} other {{numArgs} arguments}}, passed {passedArgs} instead.',
+          values: {
+            fn: out.fn,
+            numArgs: out.numArgs,
+            passedArgs: out.passedArgs,
+            missingArgs: out.missingArgs,
           },
         }),
       };

diff --git a/packages/kbn-monaco/src/esql/lib/ast/validation/esql_validation_meta_tests.json b/packages/kbn-monaco/src/esql/lib/ast/validation/esql_validation_meta_tests.json
@@ -519,11 +519,11 @@
       "error": false
     },
     {
-      "query": "row var = case(true, \"a\")",
+      "query": "row var = case(true, \"a\", \"a\", \"a\")",
       "error": false
     },
     {
-      "query": "row case(true, \"a\")",
+      "query": "row case(true, \"a\", \"a\", \"a\")",
       "error": false
     },
     {
@@ -543,19 +543,19 @@
       "error": true
     },
     {
-      "query": "row var = cidr_match(to_ip(\"127.0.0.1\"), \"a\")",
+      "query": "row var = cidr_match(to_ip(\"127.0.0.1\"), \"a\", \"a\")",
       "error": false
     },
     {
-      "query": "row cidr_match(to_ip(\"127.0.0.1\"), \"a\")",
+      "query": "row cidr_match(to_ip(\"127.0.0.1\"), \"a\", \"a\")",
       "error": false
     },
     {
-      "query": "row var = cidr_match(to_ip(\"a\"), to_string(\"a\"))",
+      "query": "row var = cidr_match(to_ip(\"a\"), to_string(\"a\"), to_string(\"a\"))",
       "error": false
     },
     {
-      "query": "row var = cidr_match(\"a\", 5)",
+      "query": "row var = cidr_match(\"a\", 5, 5)",
       "error": true
     },
     {
@@ -567,19 +567,19 @@
       "error": false
     },
     {
-      "query": "row var = concat(\"a\")",
+      "query": "row var = concat(\"a\", \"a\", \"a\")",
       "error": false
     },
     {
-      "query": "row concat(\"a\")",
+      "query": "row concat(\"a\", \"a\", \"a\")",
       "error": false
     },
     {
-      "query": "row var = concat(to_string(\"a\"))",
+      "query": "row var = concat(to_string(\"a\"), to_string(\"a\"), to_string(\"a\"))",
       "error": false
     },
     {
-      "query": "row var = concat(5)",
+      "query": "row var = concat(5, 5, 5)",
       "error": true
     },
     {
@@ -3535,11 +3535,11 @@
       "error": true
     },
     {
-      "query": "from a_index | where length(concat(stringField)) > 0",
+      "query": "from a_index | where length(concat(stringField, stringField, stringField)) > 0",
       "error": false
     },
     {
-      "query": "from a_index | where length(concat(numberField)) > 0",
+      "query": "from a_index | where length(concat(numberField, numberField, numberField)) > 0",
       "error": true
     },
     {
@@ -4567,11 +4567,11 @@
       "error": true
     },
     {
-      "query": "from a_index | eval var = case(booleanField, stringField)",
+      "query": "from a_index | eval var = case(booleanField, stringField, stringField, stringField)",
       "error": false
     },
     {
-      "query": "from a_index | eval case(booleanField, stringField)",
+      "query": "from a_index | eval case(booleanField, stringField, stringField, stringField)",
       "error": false
     },
     {
@@ -4599,19 +4599,19 @@
       "error": true
     },
     {
-      "query": "from a_index | eval var = cidr_match(ipField, stringField)",
+      "query": "from a_index | eval var = cidr_match(ipField, stringField, stringField)",
       "error": false
     },
     {
-      "query": "from a_index | eval cidr_match(ipField, stringField)",
+      "query": "from a_index | eval cidr_match(ipField, stringField, stringField)",
       "error": false
     },
     {
-      "query": "from a_index | eval var = cidr_match(to_ip(stringField), to_string(stringField))",
+      "query": "from a_index | eval var = cidr_match(to_ip(stringField), to_string(stringField), to_string(stringField))",
       "error": false
     },
     {
-      "query": "from a_index | eval cidr_match(stringField, numberField)",
+      "query": "from a_index | eval cidr_match(stringField, numberField, numberField)",
       "error": true
     },
     {
@@ -4627,23 +4627,19 @@
       "error": true
     },
     {
-      "query": "from a_index | eval var = concat(stringField)",
+      "query": "from a_index | eval var = concat(stringField, stringField, stringField)",
       "error": false
     },
     {
-      "query": "from a_index | eval concat(stringField)",
+      "query": "from a_index | eval concat(stringField, stringField, stringField)",
       "error": false
     },
     {
-      "query": "from a_index | eval var = concat(to_string(stringField))",
+      "query": "from a_index | eval var = concat(to_string(stringField), to_string(stringField), to_string(stringField))",
       "error": false
     },
     {
-      "query": "from a_index | eval concat(numberField)",
-      "error": true
-    },
-    {
-      "query": "from a_index | eval var = concat(*)",
+      "query": "from a_index | eval concat(numberField, numberField, numberField)",
       "error": true
     },
     {

diff --git a/packages/kbn-monaco/src/esql/lib/ast/validation/types.ts b/packages/kbn-monaco/src/esql/lib/ast/validation/types.ts
@@ -47,7 +47,29 @@ export interface ValidationErrors {
   };
   wrongArgumentNumber: {
     message: string;
-    type: { fn: string; numArgs: number; passedArgs: number; exactly: number };
+    type: {
+      fn: string;
+      numArgs: number;
+      passedArgs: number;
+    };
+  };
+  wrongArgumentNumberTooMany: {
+    message: string;
+    type: {
+      fn: string;
+      numArgs: number;
+      passedArgs: number;
+      extraArgs: number;
+    };
+  };
+  wrongArgumentNumberTooFew: {
+    message: string;
+    type: {
+      fn: string;
+      numArgs: number;
+      passedArgs: number;
+      missingArgs: number;
+    };
   };
   unknownColumn: {
     message: string;