[ES|QL] Improve variable and field name handling (elastic#195149)

## Summary Closes elastic#191111 Closes elastic#191105 This change cleans up the way variables and fields were being stored and checked against. We now populate the field and variable cache with unescaped column names. This means that there are fewer cache misses because of escaped names checked against unescaped/differently-escaped names. It also introduces a [suite of tests](https://github.com/elastic/kibana/pull/195149/files#diff-6e4802e45f72257ca6f765bedd3ad65d2cbb587adb5befadb5f27ad5ab08a5a6R113) for variable type detection. Most of those tests are currently skipped, but they are there to represent what should happen when we resolve elastic#195682 User-facing improvements - Validation used to fail for field names with multiple escaped parts (e.g. `geo`.`dest`) - elastic#191105 - Variables assigned the result of an inline cast used to get the wrong type. - Escaped field/variable suggestions now work with field list autocomplete https://github.com/user-attachments/assets/2162f392-3ac3-4bb4-bf37-c73318c7f751 ### Checklist - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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: Elastic Machine <[email protected]> (cherry picked from commit 731c4e4)
kibanamachine · Oct 11, 2024 · 5d473e2 · 5d473e2
1 parent d360231
commit 5d473e2
Show file tree

Hide file tree

Showing 12 changed files with 417 additions and 162 deletions.
diff --git a/packages/kbn-esql-validation-autocomplete/index.ts b/packages/kbn-esql-validation-autocomplete/index.ts
@@ -62,7 +62,6 @@ export {
   isLiteralItem,
   isTimeIntervalItem,
   isAssignment,
-  isExpression,
   isAssignmentComplete,
   isSingleItem,
 } from './src/shared/helpers';

diff --git a/...esql-validation-autocomplete/src/autocomplete/__tests__/autocomplete.command.sort.test.ts b/...esql-validation-autocomplete/src/autocomplete/__tests__/autocomplete.command.sort.test.ts
@@ -62,6 +62,36 @@ describe('autocomplete.suggest', () => {
             },
           ].map(attachTriggerCommand)
         );
+
+        await assertSuggestions(
+          'from a | sort `keywordField`/',
+          [
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField`, ',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` | ',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` ASC',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` DESC',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` NULLS FIRST',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` NULLS LAST',
+            },
+          ].map(attachTriggerCommand)
+        );
       });
       it('suggests subsequent column after comma', async () => {
         const { assertSuggestions } = await setup();

diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts
@@ -1196,28 +1196,39 @@ describe('autocomplete', () => {
         );
 
         describe('escaped field names', () => {
-          // This isn't actually the behavior we want, but this test is here
-          // to make sure no weird suggestions start cropping up in this case.
-          testSuggestions(`FROM a | ${commandName} \`foo.bar\`/`, ['foo.bar'], undefined, [
-            [{ name: 'foo.bar', type: 'double' }],
-          ]);
-          // @todo re-enable these tests when we can use AST to support this case
-          testSuggestions.skip(
+          testSuggestions(
             `FROM a | ${commandName} \`foo.bar\`/`,
-            ['foo.bar, ', 'foo.bar | '],
+            ['`foo.bar`, ', '`foo.bar` | '],
             undefined,
-            [[{ name: 'foo.bar', type: 'double' }]]
+            [
+              [
+                { name: 'foo.bar', type: 'double' },
+                { name: 'baz', type: 'date' }, // added so that we get a comma suggestion
+              ],
+            ]
           );
-          testSuggestions.skip(
-            `FROM a | ${commandName} \`foo\`.\`bar\`/`,
-            ['foo.bar, ', 'foo.bar | '],
+          testSuggestions(
+            `FROM a | ${commandName} \`foo\`\`\`\`bar\`\`baz\`/`,
+            ['`foo````bar``baz`, ', '`foo````bar``baz` | '],
             undefined,
-            [[{ name: 'foo.bar', type: 'double' }]]
+            [
+              [
+                { name: 'foo``bar`baz', type: 'double' },
+                { name: 'baz', type: 'date' }, // added so that we get a comma suggestion
+              ],
+            ]
           );
-          testSuggestions.skip(`FROM a | ${commandName} \`any#Char$Field\`/`, [
+          testSuggestions(`FROM a | ${commandName} \`any#Char$Field\`/`, [
             '`any#Char$Field`, ',
             '`any#Char$Field` | ',
           ]);
+          // @todo enable this test when we can use AST to support this case
+          testSuggestions.skip(
+            `FROM a | ${commandName} \`foo\`.\`bar\`/`,
+            ['`foo`.`bar`, ', '`foo`.`bar` | '],
+            undefined,
+            [[{ name: 'foo.bar', type: 'double' }]]
+          );
         });
 
         // Subsequent fields

diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts
@@ -83,7 +83,7 @@ import {
   TRIGGER_SUGGESTION_COMMAND,
   getAddDateHistogramSnippet,
 } from './factories';
-import { EDITOR_MARKER, SINGLE_BACKTICK, METADATA_FIELDS } from '../shared/constants';
+import { EDITOR_MARKER, METADATA_FIELDS } from '../shared/constants';
 import { getAstContext, removeMarkerArgFromArgsList } from '../shared/context';
 import {
   buildQueryUntilPreviousCommand,
@@ -1215,11 +1215,7 @@ async function getFieldsOrFunctionsSuggestions(
       filteredVariablesByType.some((v) => ALPHANUMERIC_REGEXP.test(v))
     ) {
       for (const variable of filteredVariablesByType) {
-        // remove backticks if present
-        const sanitizedVariable = variable.startsWith(SINGLE_BACKTICK)
-          ? variable.slice(1, variable.length - 1)
-          : variable;
-        const underscoredName = sanitizedVariable.replace(ALPHANUMERIC_REGEXP, '_');
+        const underscoredName = variable.replace(ALPHANUMERIC_REGEXP, '_');
         const index = filteredFieldsByType.findIndex(
           ({ label }) => underscoredName === label || `_${underscoredName}_` === label
         );

diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/factories.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/factories.ts
@@ -171,7 +171,7 @@ export const buildFieldsDefinitions = (fields: string[]): SuggestionRawDefinitio
 export const buildVariablesDefinitions = (variables: string[]): SuggestionRawDefinition[] =>
   variables.map((label) => ({
     label,
-    text: label,
+    text: getSafeInsertText(label),
     kind: 'Variable',
     detail: i18n.translate(
       'kbn-esql-validation-autocomplete.esql.autocomplete.variableDefinition',

diff --git a/packages/kbn-esql-validation-autocomplete/src/definitions/types.ts b/packages/kbn-esql-validation-autocomplete/src/definitions/types.ts
@@ -172,7 +172,7 @@ export interface CommandBaseDefinition {
       name: string;
       type: string;
       optional?: boolean;
-      innerTypes?: string[];
+      innerTypes?: Array<SupportedDataType | 'any' | 'policy'>;
       values?: string[];
       valueDescriptions?: string[];
       constantOnly?: boolean;

diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/constants.ts b/packages/kbn-esql-validation-autocomplete/src/shared/constants.ts
@@ -12,7 +12,7 @@ export const EDITOR_MARKER = 'marker_esql_editor';
 export const TICKS_REGEX = /^`{1}|`{1}$/g;
 export const DOUBLE_TICKS_REGEX = /``/g;
 export const SINGLE_TICK_REGEX = /`/g;
-export const SINGLE_BACKTICK = '`';
 export const DOUBLE_BACKTICK = '``';
+export const SINGLE_BACKTICK = '`';
 
 export const METADATA_FIELDS = ['_version', '_id', '_index', '_source', '_ignored'];
diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/esql_types.ts b/packages/kbn-esql-validation-autocomplete/src/shared/esql_types.ts
@@ -83,8 +83,6 @@ export const compareTypesWithLiterals = (
     return b === 'timeInterval';
   if (b === 'time_literal' || b === 'time_duration' || b === 'date_period')
     return a === 'timeInterval';
-  if (a === 'time_literal') return b === 'time_duration';
-  if (b === 'time_literal') return a === 'time_duration';
 
   return false;
 };
diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts b/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts
@@ -48,7 +48,7 @@ import type { ESQLRealField, ESQLVariable, ReferenceMaps } from '../validation/t
 import { removeMarkerArgFromArgsList } from './context';
 import { isNumericDecimalType } from './esql_types';
 import type { ReasonTypes } from './types';
-import { EDITOR_MARKER } from './constants';
+import { DOUBLE_TICKS_REGEX, EDITOR_MARKER, SINGLE_BACKTICK } from './constants';
 import type { EditorContext } from '../autocomplete/types';
 
 export function nonNullable<T>(v: T): v is NonNullable<T> {
@@ -99,10 +99,6 @@ export function isAssignmentComplete(node: ESQLFunction | undefined) {
   return Boolean(assignExpression && Array.isArray(assignExpression) && assignExpression.length);
 }
 
-export function isExpression(arg: ESQLAstItem): arg is ESQLFunction {
-  return isFunctionItem(arg) && arg.name !== '=';
-}
-
 export function isIncompleteItem(arg: ESQLAstItem): boolean {
   return !arg || (!Array.isArray(arg) && arg.incomplete);
 }
@@ -228,7 +224,7 @@ export function getCommandOption(optionName: CommandOptionsDefinition['name']) {
   );
 }
 
-function compareLiteralType(argType: string, item: ESQLLiteral) {
+function doesLiteralMatchParameterType(argType: FunctionParameterType, item: ESQLLiteral) {
   if (item.literalType === 'null') {
     return true;
   }
@@ -249,7 +245,7 @@ function compareLiteralType(argType: string, item: ESQLLiteral) {
   }
 
   // date-type parameters accept string literals because of ES auto-casting
-  return ['string', 'date', 'date', 'date_period'].includes(argType);
+  return ['string', 'date', 'date_period'].includes(argType);
 }
 
 /**
@@ -259,13 +255,7 @@ export function getColumnForASTNode(
   column: ESQLColumn,
   { fields, variables }: Pick<ReferenceMaps, 'fields' | 'variables'>
 ): ESQLRealField | ESQLVariable | undefined {
-  const columnName = getQuotedColumnName(column);
-  return (
-    getColumnByName(columnName, { fields, variables }) ||
-    // It's possible columnName has backticks "`fieldName`"
-    // so we need to access the original name as well
-    getColumnByName(column.name, { fields, variables })
-  );
+  return getColumnByName(column.parts.join('.'), { fields, variables });
 }
 
 /**
@@ -275,6 +265,11 @@ export function getColumnByName(
   columnName: string,
   { fields, variables }: Pick<ReferenceMaps, 'fields' | 'variables'>
 ): ESQLRealField | ESQLVariable | undefined {
+  // TODO this doesn't cover all escaping scenarios... the best thing to do would be
+  // to use the AST column node parts array, but in some cases the AST node isn't available.
+  if (columnName.startsWith(SINGLE_BACKTICK) && columnName.endsWith(SINGLE_BACKTICK)) {
+    columnName = columnName.slice(1, -1).replace(DOUBLE_TICKS_REGEX, SINGLE_BACKTICK);
+  }
   return fields.get(columnName) || variables.get(columnName)?.[0];
 }
 
@@ -445,7 +440,7 @@ export function checkFunctionArgMatchesDefinition(
     return true;
   }
   if (arg.type === 'literal') {
-    const matched = compareLiteralType(argType as string, arg);
+    const matched = doesLiteralMatchParameterType(argType, arg);
     return matched;
   }
   if (arg.type === 'function') {
@@ -549,16 +544,6 @@ export function isVariable(
   return Boolean(column && 'location' in column);
 }
 
-/**
- * This will return the name without any quotes.
- *
- * E.g. "`bytes`" will become "bytes"
- *
- * @param column
- * @returns
- */
-export const getUnquotedColumnName = (column: ESQLColumn) => column.name;
-
 /**
  * This returns the name with any quotes that were present.
  *
@@ -577,17 +562,16 @@ export function getColumnExists(
   column: ESQLColumn,
   { fields, variables }: Pick<ReferenceMaps, 'fields' | 'variables'>
 ) {
-  const namesToCheck = [getUnquotedColumnName(column), getQuotedColumnName(column)];
-
-  for (const name of namesToCheck) {
-    if (fields.has(name) || variables.has(name)) {
-      return true;
-    }
+  const columnName = column.parts.join('.');
+  if (fields.has(columnName) || variables.has(columnName)) {
+    return true;
+  }
 
-    // TODO — I don't see this fuzzy searching in lookupColumn... should it be there?
-    if (Boolean(fuzzySearch(name, fields.keys()) || fuzzySearch(name, variables.keys()))) {
-      return true;
-    }
+  // TODO — I don't see this fuzzy searching in lookupColumn... should it be there?
+  if (
+    Boolean(fuzzySearch(columnName, fields.keys()) || fuzzySearch(columnName, variables.keys()))
+  ) {
+    return true;
   }
 
   return false;