From 731c4e4aabbacfdc36ae44bce3e10f438b55a4cd Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Fri, 11 Oct 2024 08:33:04 -0600 Subject: [PATCH] [ES|QL] Improve variable and field name handling (#195149) ## Summary Closes https://github.com/elastic/kibana/issues/191111 Closes https://github.com/elastic/kibana/issues/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 https://github.com/elastic/kibana/issues/195682 User-facing improvements - Validation used to fail for field names with multiple escaped parts (e.g. `geo`.`dest`) - https://github.com/elastic/kibana/issues/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 --- .../kbn-esql-validation-autocomplete/index.ts | 1 - .../autocomplete.command.sort.test.ts | 30 +++ .../src/autocomplete/autocomplete.test.ts | 39 +-- .../src/autocomplete/autocomplete.ts | 8 +- .../src/autocomplete/factories.ts | 2 +- .../src/definitions/types.ts | 2 +- .../src/shared/constants.ts | 2 +- .../src/shared/esql_types.ts | 2 - .../src/shared/helpers.ts | 54 ++--- .../src/shared/variables.ts | 205 ++++++++-------- .../__tests__/fields_and_variables.test.ts | 222 ++++++++++++++++++ .../src/validation/validation.ts | 12 +- 12 files changed, 417 insertions(+), 162 deletions(-) create mode 100644 packages/kbn-esql-validation-autocomplete/src/validation/__tests__/fields_and_variables.test.ts diff --git a/packages/kbn-esql-validation-autocomplete/index.ts b/packages/kbn-esql-validation-autocomplete/index.ts index 0bfc4274fe84d..67fdd72880150 100644 --- 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/packages/kbn-esql-validation-autocomplete/src/autocomplete/__tests__/autocomplete.command.sort.test.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/__tests__/autocomplete.command.sort.test.ts index c934c4d72e35a..a33235dda581b 100644 --- a/packages/kbn-esql-validation-autocomplete/src/autocomplete/__tests__/autocomplete.command.sort.test.ts +++ b/packages/kbn-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 index a0a4a359c5ff6..8524cd4950955 100644 --- 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 index 6f9fb66a8c715..5d885379f1a94 100644 --- 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 index cb08e0f8d4e91..72f5759d0d555 100644 --- 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 index 2b1bd618449c3..9ce286796c971 100644 --- 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; 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 index f97800d6dbc1e..c1942118a41e2 100644 --- 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 index 6fb9725211478..66f985505a43d 100644 --- 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 index fd53d08e67f90..ce9cec58575fc 100644 --- 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(v: T): v is NonNullable { @@ -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 ): 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 ): 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 ) { - 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; diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/variables.ts b/packages/kbn-esql-validation-autocomplete/src/shared/variables.ts index 54e6d38736007..29656d2f581ed 100644 --- a/packages/kbn-esql-validation-autocomplete/src/shared/variables.ts +++ b/packages/kbn-esql-validation-autocomplete/src/shared/variables.ts @@ -7,19 +7,13 @@ * License v3.0 only", or the "Server Side Public License, v 1". */ -import type { ESQLAstItem, ESQLCommand, ESQLCommandOption, ESQLFunction } from '@kbn/esql-ast'; +import type { ESQLAst, ESQLAstItem, ESQLCommand, ESQLFunction } from '@kbn/esql-ast'; +import { Visitor } from '@kbn/esql-ast/src/visitor'; import type { ESQLVariable, ESQLRealField } from '../validation/types'; -import { DOUBLE_BACKTICK, EDITOR_MARKER, SINGLE_BACKTICK } from './constants'; -import { - isColumnItem, - isAssignment, - isExpression, - isOptionItem, - isFunctionItem, - getFunctionDefinition, -} from './helpers'; +import { EDITOR_MARKER } from './constants'; +import { isColumnItem, isFunctionItem, getFunctionDefinition } from './helpers'; -function addToVariableOccurrencies(variables: Map, instance: ESQLVariable) { +function addToVariableOccurrences(variables: Map, instance: ESQLVariable) { if (!variables.has(instance.name)) { variables.set(instance.name, []); } @@ -35,41 +29,73 @@ function addToVariables( ) { if (isColumnItem(oldArg) && isColumnItem(newArg)) { const newVariable: ESQLVariable = { - name: newArg.name, + name: newArg.parts.join('.'), type: 'double' /* fallback to number */, location: newArg.location, }; // Now workout the exact type // it can be a rename of another variable as well - const oldRef = - fields.get(oldArg.name) || variables.get(oldArg.quoted ? oldArg.text : oldArg.name); + const oldRef = fields.get(oldArg.parts.join('.')) || variables.get(oldArg.parts.join('.')); if (oldRef) { - addToVariableOccurrencies(variables, newVariable); + addToVariableOccurrences(variables, newVariable); newVariable.type = Array.isArray(oldRef) ? oldRef[0].type : oldRef.type; } } } -function getAssignRightHandSideType(item: ESQLAstItem, fields: Map) { - if (Array.isArray(item)) { - const firstArg = item[0]; - if (Array.isArray(firstArg) || !firstArg) { - return; +/** + * Determines the type of the expression + * + * TODO - this function needs a lot of work. For example, it needs to find the best-matching function signature + * which it isn't currently doing. See https://github.com/elastic/kibana/issues/195682 + */ +function getExpressionType( + root: ESQLAstItem, + fields: Map, + variables: Map +): string { + const fallback = 'double'; + + if (Array.isArray(root) || !root) { + return fallback; + } + if (root.type === 'literal') { + return root.literalType; + } + if (root.type === 'inlineCast') { + if (root.castType === 'int') { + return 'integer'; } - if (firstArg.type === 'literal') { - return firstArg.literalType; + if (root.castType === 'bool') { + return 'boolean'; } - if (isColumnItem(firstArg)) { - const field = fields.get(firstArg.name); - if (field) { - return field.type; - } + return root.castType; + } + if (isColumnItem(root)) { + const field = fields.get(root.parts.join('.')); + if (field) { + return field.type; } - if (isFunctionItem(firstArg)) { - const fnDefinition = getFunctionDefinition(firstArg.name); - return fnDefinition?.signatures[0].returnType; + const variable = variables.get(root.parts.join('.')); + if (variable) { + return variable[0].type; } - return firstArg.type; + } + if (isFunctionItem(root)) { + const fnDefinition = getFunctionDefinition(root.name); + return fnDefinition?.signatures[0].returnType ?? fallback; + } + return fallback; +} + +function getAssignRightHandSideType( + item: ESQLAstItem, + fields: Map, + variables: Map +) { + if (Array.isArray(item)) { + const firstArg = item[0]; + return getExpressionType(firstArg, fields, variables); } } @@ -90,25 +116,20 @@ export function excludeVariablesFromCurrentCommand( return resultVariables; } -function extractExpressionAsQuotedVariable( - originalQuery: string, - location: { min: number; max: number } -) { - const extractExpressionText = originalQuery.substring(location.min, location.max + 1); - // now inject quotes and save it as variable - return `\`${extractExpressionText.replaceAll(SINGLE_BACKTICK, DOUBLE_BACKTICK)}\``; -} - function addVariableFromAssignment( assignOperation: ESQLFunction, variables: Map, fields: Map ) { if (isColumnItem(assignOperation.args[0])) { - const rightHandSideArgType = getAssignRightHandSideType(assignOperation.args[1], fields); - addToVariableOccurrencies(variables, { - name: assignOperation.args[0].name, - type: (rightHandSideArgType as string) || 'double' /* fallback to number */, + const rightHandSideArgType = getAssignRightHandSideType( + assignOperation.args[1], + fields, + variables + ); + addToVariableOccurrences(variables, { + name: assignOperation.args[0].parts.join('.'), + type: rightHandSideArgType as string /* fallback to number */, location: assignOperation.args[0].location, }); } @@ -120,79 +141,71 @@ function addVariableFromExpression( variables: Map ) { if (!expressionOperation.text.includes(EDITOR_MARKER)) { - // save the variable in its quoted usable way - // (a bit of forward thinking here to simplyfy lookups later) - const forwardThinkingVariableName = extractExpressionAsQuotedVariable( - queryString, - expressionOperation.location + const expressionText = queryString.substring( + expressionOperation.location.min, + expressionOperation.location.max + 1 ); - const expressionType = 'double'; - addToVariableOccurrencies(variables, { - name: forwardThinkingVariableName, + const expressionType = 'double'; // TODO - use getExpressionType once it actually works + addToVariableOccurrences(variables, { + name: expressionText, type: expressionType, location: expressionOperation.location, }); } } -export const collectVariablesFromList = ( - list: ESQLAstItem[], - fields: Map, - queryString: string, - variables: Map -) => { - for (const arg of list) { - if (isAssignment(arg)) { - addVariableFromAssignment(arg, variables, fields); - } else if (isExpression(arg)) { - addVariableFromExpression(arg, queryString, variables); - } - } -}; - export function collectVariables( - commands: ESQLCommand[], + ast: ESQLAst, fields: Map, queryString: string ): Map { const variables = new Map(); - for (const command of commands) { - if (['row', 'eval', 'stats', 'inlinestats', 'metrics'].includes(command.name)) { - collectVariablesFromList(command.args, fields, queryString, variables); - if (command.name === 'stats' || command.name === 'inlinestats') { - const commandOptionsWithAssignment = command.args.filter( - (arg) => isOptionItem(arg) && arg.name === 'by' - ) as ESQLCommandOption[]; - for (const commandOption of commandOptionsWithAssignment) { - collectVariablesFromList(commandOption.args, fields, queryString, variables); - } + + const visitor = new Visitor() + .on('visitLiteralExpression', (ctx) => { + // TODO - add these as variables + }) + .on('visitExpression', (_ctx) => {}) // required for the types :shrug: + .on('visitRenameExpression', (ctx) => { + const [oldArg, newArg] = ctx.node.args; + addToVariables(oldArg, newArg, fields, variables); + }) + .on('visitFunctionCallExpression', (ctx) => { + if (ctx.node.name === '=') { + addVariableFromAssignment(ctx.node, variables, fields); + } else { + addVariableFromExpression(ctx.node, queryString, variables); } - } - if (command.name === 'enrich') { - const commandOptionsWithAssignment = command.args.filter( - (arg) => isOptionItem(arg) && arg.name === 'with' - ) as ESQLCommandOption[]; - for (const commandOption of commandOptionsWithAssignment) { - // Enrich assignment has some special behaviour, so do not use the version above here... - for (const assignFn of commandOption.args) { + }) + .on('visitCommandOption', (ctx) => { + if (ctx.node.name === 'by') { + return [...ctx.visitArguments()]; + } else if (ctx.node.name === 'with') { + for (const assignFn of ctx.node.args) { if (isFunctionItem(assignFn)) { const [newArg, oldArg] = assignFn?.args || []; + // TODO why is oldArg an array? if (Array.isArray(oldArg)) { addToVariables(oldArg[0], newArg, fields, variables); } } } } - } - if (command.name === 'rename') { - const commandOptionsWithAssignment = command.args.filter( - (arg) => isOptionItem(arg) && arg.name === 'as' - ) as ESQLCommandOption[]; - for (const commandOption of commandOptionsWithAssignment) { - const [oldArg, newArg] = commandOption.args; - addToVariables(oldArg, newArg, fields, variables); + }) + .on('visitCommand', (ctx) => { + const ret = []; + if (['row', 'eval', 'stats', 'inlinestats', 'metrics', 'rename'].includes(ctx.node.name)) { + ret.push(...ctx.visitArgs()); } - } - } + if (['stats', 'inlinestats', 'enrich'].includes(ctx.node.name)) { + // BY and WITH can contain variables + ret.push(...ctx.visitOptions()); + } + return ret; + }) + .on('visitQuery', (ctx) => [...ctx.visitCommands()]); + + visitor.visitQuery(ast); + return variables; } diff --git a/packages/kbn-esql-validation-autocomplete/src/validation/__tests__/fields_and_variables.test.ts b/packages/kbn-esql-validation-autocomplete/src/validation/__tests__/fields_and_variables.test.ts new file mode 100644 index 0000000000000..e0004d603d9c2 --- /dev/null +++ b/packages/kbn-esql-validation-autocomplete/src/validation/__tests__/fields_and_variables.test.ts @@ -0,0 +1,222 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { FunctionParameterType } from '../../definitions/types'; +import { setTestFunctions } from '../../shared/test_functions'; +import { setup } from './helpers'; + +describe('field and variable escaping', () => { + it('recognizes escaped fields', async () => { + const { expectErrors } = await setup(); + // command level + await expectErrors( + 'FROM index | KEEP `kubernetes`.`something`.`something` | EVAL `kubernetes.something.something` + 12', + [] + ); + // function argument + await expectErrors('FROM index | EVAL ABS(`kubernetes`.`something`.`something`)', []); + }); + + it('recognizes field names with spaces and comments', async () => { + const { expectErrors } = await setup(); + // command level + await expectErrors('FROM index | KEEP kubernetes . something . /* gotcha! */ something', []); + // function argument + await expectErrors( + 'FROM index | EVAL ABS(kubernetes . something . /* gotcha! */ something)', + [] + ); + }); + + it('recognizes escaped variables', async () => { + const { expectErrors } = await setup(); + // command level + await expectErrors('ROW `var$iable` = 1 | EVAL `var$iable`', []); + + // command level, different escaping in declaration + await expectErrors( + 'ROW variable.`wi#th`.separator = "lolz" | EVAL `variable`.`wi#th`.`separator`', + [] + ); + + // function arguments + await expectErrors( + 'ROW `var$iable` = 1, variable.`wi#th`.separator = "lolz" | EVAL ABS(`var$iable`), TRIM(variable.`wi#th`.`separator`)', + [] + ); + + // expression variable + await expectErrors('FROM index | EVAL doubleField + 20 | EVAL `doubleField + 20`', []); + await expectErrors('ROW 21 + 20 | STATS AVG(`21 + 20`)', []); + }); + + it('recognizes variables with spaces and comments', async () => { + const { expectErrors } = await setup(); + // command level + await expectErrors( + 'ROW variable.`wi#th`.separator = "lolz" | RENAME variable . /* lolz */ `wi#th` . separator AS foo', + [] + ); + // function argument + await expectErrors( + 'ROW variable.`wi#th`.separator = "lolz" | EVAL TRIM(variable . /* lolz */ `wi#th` . separator)', + [] + ); + }); + + describe('as part of various commands', () => { + const cases = [ + { name: 'ROW', command: 'ROW `var$iable` = 1, variable.`wi#th`.separator = "lolz"' }, + { + name: 'DISSECT', + command: 'ROW `funky`.`stri#$ng` = "lolz" | DISSECT `funky`.`stri#$ng` "%{WORD:firstWord}"', + }, + { name: 'DROP', command: 'FROM index | DROP kubernetes.`something`.`something`' }, + { + name: 'ENRICH', + command: + 'FROM index | ENRICH policy WITH `new`.name1 = `otherField`, `new.name2` = `yetAnotherField`', + }, + { name: 'EVAL', command: 'FROM index | EVAL kubernetes.`something`.`something` + 12' }, + { + name: 'GROK', + command: 'ROW `funky`.`stri#$ng` = "lolz" | GROK `funky`.`stri#$ng` "%{WORD:firstWord}"', + }, + { name: 'KEEP', command: 'FROM index | KEEP kubernetes.`something`.`something`' }, + { + name: 'RENAME', + command: 'FROM index | RENAME kubernetes.`something`.`something` as foobar', + }, + { name: 'SORT', command: 'FROM index | SORT kubernetes.`something`.`something` DESC' }, + { + name: 'STATS ... BY', + command: + 'FROM index | STATS AVG(kubernetes.`something`.`something`) BY `kubernetes`.`something`.`something`', + }, + { name: 'WHERE', command: 'FROM index | WHERE kubernetes.`something`.`something` == 12' }, + ]; + + it.each(cases)('$name accepts escaped fields', async ({ command }) => { + const { expectErrors } = await setup(); + await expectErrors(command, []); + }); + }); +}); + +describe('variable support', () => { + describe('variable data type detection', () => { + // most of these tests are aspirational (and skipped) because we don't have + // a good way to compute the type of an expression yet. + beforeAll(() => { + setTestFunctions([ + // this test function is just used to test the type of the variable + { + type: 'eval', + description: 'Test function', + supportedCommands: ['eval'], + name: 'test', + signatures: [ + { params: [{ name: 'arg', type: 'cartesian_point' }], returnType: 'cartesian_point' }, + ], + }, + // this test function is used to check that the correct return type is used + // when determining variable types + { + type: 'eval', + description: 'Test function', + supportedCommands: ['eval'], + name: 'return_value', + signatures: [ + { params: [{ name: 'arg', type: 'text' }], returnType: 'text' }, + { params: [{ name: 'arg', type: 'double' }], returnType: 'double' }, + { + params: [ + { name: 'arg', type: 'double' }, + { name: 'arg', type: 'text' }, + ], + returnType: 'long', + }, + ], + }, + ]); + }); + + afterAll(() => { + setTestFunctions([]); + }); + + const expectType = (type: FunctionParameterType) => + `Argument of [test] must be [cartesian_point], found value [var] type [${type}]`; + + // @todo unskip after https://github.com/elastic/kibana/issues/195682 + test.skip('literals', async () => { + const { expectErrors } = await setup(); + // literal assignment + await expectErrors('FROM index | EVAL var = 1, TEST(var)', [expectType('integer')]); + // literal expression + await expectErrors('FROM index | EVAL 1, TEST(`1`)', [expectType('integer')]); + }); + + test('fields', async () => { + const { expectErrors } = await setup(); + // field assignment + await expectErrors('FROM index | EVAL var = textField, TEST(var)', [expectType('text')]); + }); + + // @todo unskip after https://github.com/elastic/kibana/issues/195682 + test.skip('variables', async () => { + const { expectErrors } = await setup(); + await expectErrors('FROM index | EVAL var = textField, var2 = var, TEST(var2)', [ + `Argument of [test] must be [cartesian_point], found value [var2] type [text]`, + ]); + }); + + // @todo unskip after https://github.com/elastic/kibana/issues/195682 + test.skip('inline casting', async () => { + const { expectErrors } = await setup(); + // inline cast assignment + await expectErrors('FROM index | EVAL var = doubleField::long, TEST(var)', [ + expectType('long'), + ]); + // inline cast expression + await expectErrors('FROM index | EVAL doubleField::long, TEST(`doubleField::long`)', [ + expectType('long'), + ]); + }); + + // @todo unskip after https://github.com/elastic/kibana/issues/195682 + test.skip('function results', async () => { + const { expectErrors } = await setup(); + // function assignment + await expectErrors('FROM index | EVAL var = RETURN_VALUE(doubleField), TEST(var)', [ + expectType('double'), + ]); + await expectErrors('FROM index | EVAL var = RETURN_VALUE(textField), TEST(var)', [ + expectType('text'), + ]); + await expectErrors( + 'FROM index | EVAL var = RETURN_VALUE(doubleField, textField), TEST(var)', + [expectType('long')] + ); + // function expression + await expectErrors( + 'FROM index | EVAL RETURN_VALUE(doubleField), TEST(`RETURN_VALUE(doubleField)`)', + [expectType('double')] + ); + await expectErrors( + 'FROM index | EVAL RETURN_VALUE(textField), TEST(`RETURN_VALUE(textField)`)', + [expectType('text')] + ); + await expectErrors( + 'FROM index | EVAL RETURN_VALUE(doubleField, textField), TEST(`RETURN_VALUE(doubleField, textField)`)', + [expectType('long')] + ); + }); + }); +}); diff --git a/packages/kbn-esql-validation-autocomplete/src/validation/validation.ts b/packages/kbn-esql-validation-autocomplete/src/validation/validation.ts index fdfe03852fc48..23508eeedd234 100644 --- a/packages/kbn-esql-validation-autocomplete/src/validation/validation.ts +++ b/packages/kbn-esql-validation-autocomplete/src/validation/validation.ts @@ -76,7 +76,7 @@ import { import { collapseWrongArgumentTypeMessages, getMaxMinNumberOfParams } from './helpers'; import { getParamAtPosition } from '../autocomplete/helper'; import { METADATA_FIELDS } from '../shared/constants'; -import { isStringType } from '../shared/esql_types'; +import { compareTypesWithLiterals } from '../shared/esql_types'; function validateFunctionLiteralArg( astFunction: ESQLFunction, @@ -870,10 +870,12 @@ function validateColumnForCommand( const columnRef = getColumnForASTNode(column, references)!; if (columnParamsWithInnerTypes.length) { - const hasSomeWrongInnerTypes = columnParamsWithInnerTypes.every(({ innerTypes }) => { - if (innerTypes?.includes('string') && isStringType(columnRef.type)) return false; - return innerTypes && !innerTypes.includes('any') && !innerTypes.includes(columnRef.type); - }); + const hasSomeWrongInnerTypes = columnParamsWithInnerTypes.every( + ({ innerTypes }) => + innerTypes && + !innerTypes.includes('any') && + !innerTypes.some((type) => compareTypesWithLiterals(type, columnRef.type)) + ); if (hasSomeWrongInnerTypes) { const supportedTypes: string[] = columnParamsWithInnerTypes .map(({ innerTypes }) => innerTypes)