Skip to content

Commit

Permalink
[ES|QL] Add autocomplete and validation to support MATCH and QSRT (el…
Browse files Browse the repository at this point in the history
…astic#199032)

## Summary

Closes elastic#196995. This PR adds
autocomplete and validation to support MATCH and QSRT



https://github.com/user-attachments/assets/f6be7108-cc6c-480f-b7cf-8c7953d06e7a



https://github.com/user-attachments/assets/0549e044-90d6-4619-a00b-c9a2c8f94c04



### Checklist

Delete any items that are not applicable to this PR.

- [ ] 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)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature
release.

When forming the risk matrix, consider some of the following examples
and how they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces—unexpected behavior in non-default Kibana Space.
| Low | High | Integration tests will verify that all features are still
supported in non-default Kibana Space and when user switches between
spaces. |
| Multiple nodes—Elasticsearch polling might have race conditions
when multiple Kibana nodes are polling for the same tasks. | High | Low
| Tasks are idempotent, so executing them multiple times will not result
in logical error, but will degrade performance. To test for this case we
add plenty of unit tests around this logic and document manual testing
procedure. |
| Code should gracefully handle cases when feature X or plugin Y are
disabled. | Medium | High | Unit tests will verify that any feature flag
or plugin combination still results in our service operational. |
| [See more potential risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)
- [ ] This will appear in the **Release Notes** and follow the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
  • Loading branch information
qn895 and stratoula authored Nov 14, 2024
1 parent 86b3738 commit f4af267
Show file tree
Hide file tree
Showing 14 changed files with 422 additions and 58 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { join } from 'path';
import _ from 'lodash';
import type { RecursivePartial } from '@kbn/utility-types';
import { FunctionDefinition } from '../src/definitions/types';

import { FULL_TEXT_SEARCH_FUNCTIONS } from '../src/shared/constants';
const aliasTable: Record<string, string[]> = {
to_version: ['to_ver'],
to_unsigned_long: ['to_ul', 'to_ulong'],
Expand Down Expand Up @@ -246,23 +246,40 @@ const convertDateTime = (s: string) => (s === 'datetime' ? 'date' : s);
* @returns
*/
function getFunctionDefinition(ESFunctionDefinition: Record<string, any>): FunctionDefinition {
let supportedCommandsAndOptions: Pick<
FunctionDefinition,
'supportedCommands' | 'supportedOptions'
> =
ESFunctionDefinition.type === 'eval'
? scalarSupportedCommandsAndOptions
: aggregationSupportedCommandsAndOptions;

// MATCH and QSRT has limited supported for where commands only
if (FULL_TEXT_SEARCH_FUNCTIONS.includes(ESFunctionDefinition.name)) {
supportedCommandsAndOptions = {
supportedCommands: ['where'],
supportedOptions: [],
};
}
const ret = {
type: ESFunctionDefinition.type,
name: ESFunctionDefinition.name,
...(ESFunctionDefinition.type === 'eval'
? scalarSupportedCommandsAndOptions
: aggregationSupportedCommandsAndOptions),
...supportedCommandsAndOptions,
description: ESFunctionDefinition.description,
alias: aliasTable[ESFunctionDefinition.name],
ignoreAsSuggestion: ESFunctionDefinition.snapshot_only,
preview: ESFunctionDefinition.preview,
signatures: _.uniqBy(
ESFunctionDefinition.signatures.map((signature: any) => ({
...signature,
params: signature.params.map((param: any) => ({
params: signature.params.map((param: any, idx: number) => ({
...param,
type: convertDateTime(param.type),
description: undefined,
...(idx === 0 && FULL_TEXT_SEARCH_FUNCTIONS.includes(ESFunctionDefinition.name)
? // Default to false. If set to true, this parameter does not accept a function or literal, only fields.
{ fieldsOnly: true }
: {}),
})),
returnType: convertDateTime(signature.returnType),
variadic: undefined, // we don't support variadic property
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('WHERE <expression>', () => {
.map((name) => `${name} `)
.map(attachTriggerCommand),
attachTriggerCommand('var0 '),
...allEvalFns,
...allEvalFns.filter((fn) => fn.label !== 'QSTR'),
],
{
callbacks: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ describe('autocomplete.suggest', () => {
for (const fn of scalarFunctionDefinitions) {
// skip this fn for the moment as it's quite hard to test
// Add match in the text when the autocomplete is ready https://github.com/elastic/kibana/issues/196995
if (!['bucket', 'date_extract', 'date_diff', 'case', 'match'].includes(fn.name)) {
if (!['bucket', 'date_extract', 'date_diff', 'case', 'match', 'qstr'].includes(fn.name)) {
test(`${fn.name}`, async () => {
const testedCases = new Set<string>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export const policies = [
* @returns
*/
export function getFunctionSignaturesByReturnType(
command: string,
command: string | string[],
_expectedReturnType: Readonly<FunctionReturnType | 'any' | Array<FunctionReturnType | 'any'>>,
{
agg,
Expand Down Expand Up @@ -165,12 +165,16 @@ export function getFunctionSignaturesByReturnType(

const deduped = Array.from(new Set(list));

const commands = Array.isArray(command) ? command : [command];
return deduped
.filter(({ signatures, ignoreAsSuggestion, supportedCommands, supportedOptions, name }) => {
if (ignoreAsSuggestion) {
return false;
}
if (!supportedCommands.includes(command) && !supportedOptions?.includes(option || '')) {
if (
!commands.some((c) => supportedCommands.includes(c)) &&
!supportedOptions?.includes(option || '')
) {
return false;
}
const filteredByReturnType = signatures.filter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import { uniq, uniqBy } from 'lodash';
import type {
AstProviderFn,
ESQLAst,
ESQLAstItem,
ESQLCommand,
ESQLCommandOption,
Expand Down Expand Up @@ -151,14 +152,16 @@ export async function suggest(
astProvider: AstProviderFn,
resourceRetriever?: ESQLCallbacks
): Promise<SuggestionRawDefinition[]> {
// Partition out to inner ast / ast context for the latest command
const innerText = fullText.substring(0, offset);

const correctedQuery = correctQuerySyntax(innerText, context);

const { ast } = await astProvider(correctedQuery);

const astContext = getAstContext(innerText, ast, offset);

// But we also need the full ast for the full query
const correctedFullQuery = correctQuerySyntax(fullText, context);
const { ast: fullAst } = await astProvider(correctedFullQuery);

if (astContext.type === 'comment') {
return [];
}
Expand Down Expand Up @@ -216,7 +219,8 @@ export async function suggest(
getFieldsMap,
getPolicies,
getPolicyMetadata,
resourceRetriever?.getPreferences
resourceRetriever?.getPreferences,
fullAst
);
}
if (astContext.type === 'setting') {
Expand Down Expand Up @@ -394,7 +398,8 @@ async function getSuggestionsWithinCommandExpression(
getFieldsMap: GetFieldsMapFn,
getPolicies: GetPoliciesFn,
getPolicyMetadata: GetPolicyMetadataFn,
getPreferences?: () => Promise<{ histogramBarTarget: number } | undefined>
getPreferences?: () => Promise<{ histogramBarTarget: number } | undefined>,
fullAst?: ESQLAst
) {
const commandDef = getCommandDefinition(command.name);

Expand All @@ -413,7 +418,8 @@ async function getSuggestionsWithinCommandExpression(
() => findNewVariable(anyVariables),
(expression: ESQLAstItem | undefined) =>
getExpressionType(expression, references.fields, references.variables),
getPreferences
getPreferences,
fullAst
);
} else {
// The deprecated path.
Expand Down Expand Up @@ -1173,19 +1179,21 @@ async function getFunctionArgsSuggestions(
);

// Functions
suggestions.push(
...getFunctionSuggestions({
command: command.name,
option: option?.name,
returnTypes: canBeBooleanCondition
? ['any']
: (getTypesFromParamDefs(typesToSuggestNext) as string[]),
ignored: fnToIgnore,
}).map((suggestion) => ({
...suggestion,
text: addCommaIf(shouldAddComma, suggestion.text),
}))
);
if (typesToSuggestNext.every((d) => !d.fieldsOnly)) {
suggestions.push(
...getFunctionSuggestions({
command: command.name,
option: option?.name,
returnTypes: canBeBooleanCondition
? ['any']
: (getTypesFromParamDefs(typesToSuggestNext) as string[]),
ignored: fnToIgnore,
}).map((suggestion) => ({
...suggestion,
text: addCommaIf(shouldAddComma, suggestion.text),
}))
);
}
// could also be in stats (bucket) but our autocomplete is not great yet
if (
(getTypesFromParamDefs(typesToSuggestNext).includes('date') &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
type ESQLCommand,
type ESQLSingleAstItem,
type ESQLFunction,
ESQLAst,
} from '@kbn/esql-ast';
import { logicalOperators } from '../../../definitions/builtin';
import { isParameterType, type SupportedDataType } from '../../../definitions/types';
Expand All @@ -27,6 +28,10 @@ import {
import { getOverlapRange, getSuggestionsToRightOfOperatorExpression } from '../../helper';
import { getPosition } from './util';
import { pipeCompleteItem } from '../../complete_items';
import {
UNSUPPORTED_COMMANDS_BEFORE_MATCH,
UNSUPPORTED_COMMANDS_BEFORE_QSTR,
} from '../../../shared/constants';

export async function suggest(
innerText: string,
Expand All @@ -35,7 +40,8 @@ export async function suggest(
_columnExists: (column: string) => boolean,
_getSuggestedVariableName: () => string,
getExpressionType: (expression: ESQLAstItem | undefined) => SupportedDataType | 'unknown',
_getPreferences?: () => Promise<{ histogramBarTarget: number } | undefined>
_getPreferences?: () => Promise<{ histogramBarTarget: number } | undefined>,
fullTextAst?: ESQLAst
): Promise<SuggestionRawDefinition[]> {
const suggestions: SuggestionRawDefinition[] = [];

Expand Down Expand Up @@ -154,11 +160,25 @@ export async function suggest(
break;

case 'empty_expression':
// Don't suggest MATCH or QSTR after unsupported commands
const priorCommands = fullTextAst?.map((a) => a.name) ?? [];
const ignored = [];
if (priorCommands.some((c) => UNSUPPORTED_COMMANDS_BEFORE_MATCH.has(c))) {
ignored.push('match');
}
if (priorCommands.some((c) => UNSUPPORTED_COMMANDS_BEFORE_QSTR.has(c))) {
ignored.push('qstr');
}

const columnSuggestions = await getColumnsByType('any', [], {
advanceCursor: true,
openSuggestions: true,
});
suggestions.push(...columnSuggestions, ...getFunctionSuggestions({ command: 'where' }));

suggestions.push(
...columnSuggestions,
...getFunctionSuggestions({ command: 'where', ignored })
);

break;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type { CodeActionOptions } from './types';
import type { ESQLRealField } from '../validation/types';
import type { FieldType } from '../definitions/types';
import type { ESQLCallbacks, PartialFieldsMetadataClient } from '../shared/types';
import { FULL_TEXT_SEARCH_FUNCTIONS } from '../shared/constants';

function getCallbackMocks(): jest.Mocked<ESQLCallbacks> {
return {
Expand Down Expand Up @@ -285,6 +286,16 @@ describe('quick fixes logic', () => {
{ relaxOnMissingCallbacks: false },
]) {
for (const fn of getAllFunctions({ type: 'eval' })) {
if (FULL_TEXT_SEARCH_FUNCTIONS.includes(fn.name)) {
testQuickFixes(
`FROM index | WHERE ${BROKEN_PREFIX}${fn.name}()`,
[fn.name].map(toFunctionSignature),
{ equalityCheck: 'include', ...options }
);
}
}
for (const fn of getAllFunctions({ type: 'eval' })) {
if (FULL_TEXT_SEARCH_FUNCTIONS.includes(fn.name)) continue;
// add an A to the function name to make it invalid
testQuickFixes(
`FROM index | EVAL ${BROKEN_PREFIX}${fn.name}()`,
Expand Down Expand Up @@ -313,6 +324,8 @@ describe('quick fixes logic', () => {
);
}
for (const fn of getAllFunctions({ type: 'agg' })) {
if (FULL_TEXT_SEARCH_FUNCTIONS.includes(fn.name)) continue;

// add an A to the function name to make it invalid
testQuickFixes(
`FROM index | STATS ${BROKEN_PREFIX}${fn.name}()`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3259,6 +3259,7 @@ const matchDefinition: FunctionDefinition = {
name: 'field',
type: 'keyword',
optional: false,
fieldsOnly: true,
},
{
name: 'query',
Expand All @@ -3274,6 +3275,7 @@ const matchDefinition: FunctionDefinition = {
name: 'field',
type: 'keyword',
optional: false,
fieldsOnly: true,
},
{
name: 'query',
Expand All @@ -3289,6 +3291,7 @@ const matchDefinition: FunctionDefinition = {
name: 'field',
type: 'text',
optional: false,
fieldsOnly: true,
},
{
name: 'query',
Expand All @@ -3304,6 +3307,7 @@ const matchDefinition: FunctionDefinition = {
name: 'field',
type: 'text',
optional: false,
fieldsOnly: true,
},
{
name: 'query',
Expand All @@ -3314,8 +3318,8 @@ const matchDefinition: FunctionDefinition = {
returnType: 'boolean',
},
],
supportedCommands: ['stats', 'inlinestats', 'metrics', 'eval', 'where', 'row', 'sort'],
supportedOptions: ['by'],
supportedCommands: ['where'],
supportedOptions: [],
validate: undefined,
examples: [
'from books \n| where match(author, "Faulkner")\n| keep book_no, author \n| sort book_no \n| limit 5;',
Expand Down Expand Up @@ -5912,6 +5916,7 @@ const qstrDefinition: FunctionDefinition = {
name: 'query',
type: 'keyword',
optional: false,
fieldsOnly: true,
},
],
returnType: 'boolean',
Expand All @@ -5922,13 +5927,14 @@ const qstrDefinition: FunctionDefinition = {
name: 'query',
type: 'text',
optional: false,
fieldsOnly: true,
},
],
returnType: 'boolean',
},
],
supportedCommands: ['stats', 'inlinestats', 'metrics', 'eval', 'where', 'row', 'sort'],
supportedOptions: ['by'],
supportedCommands: ['where'],
supportedOptions: [],
validate: undefined,
examples: [
'from books \n| where qstr("author: Faulkner")\n| keep book_no, author \n| sort book_no \n| limit 5;',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import type {
ESQLAst,
ESQLAstItem,
ESQLCommand,
ESQLCommandOption,
Expand Down Expand Up @@ -136,6 +137,10 @@ export interface FunctionDefinition {
* though a function can be used to create the value. (e.g. now() for dates or concat() for strings)
*/
constantOnly?: boolean;
/**
* Default to false. If set to true, this parameter does not accept a function or literal, only fields.
*/
fieldsOnly?: boolean;
/**
* if provided this means that the value must be one
* of the options in the array iff the value is a literal.
Expand Down Expand Up @@ -181,7 +186,8 @@ export interface CommandBaseDefinition<CommandName extends string> {
columnExists: (column: string) => boolean,
getSuggestedVariableName: () => string,
getExpressionType: (expression: ESQLAstItem | undefined) => SupportedDataType | 'unknown',
getPreferences?: () => Promise<{ histogramBarTarget: number } | undefined>
getPreferences?: () => Promise<{ histogramBarTarget: number } | undefined>,
fullTextAst?: ESQLAst
) => Promise<SuggestionRawDefinition[]>;
/** @deprecated this property will disappear in the future */
signature: {
Expand Down
16 changes: 16 additions & 0 deletions packages/kbn-esql-validation-autocomplete/src/shared/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,19 @@ export const DOUBLE_BACKTICK = '``';
export const SINGLE_BACKTICK = '`';

export const METADATA_FIELDS = ['_version', '_id', '_index', '_source', '_ignored', '_index_mode'];

export const FULL_TEXT_SEARCH_FUNCTIONS = ['match', 'qstr'];
export const UNSUPPORTED_COMMANDS_BEFORE_QSTR = new Set([
'show',
'row',
'dissect',
'enrich',
'eval',
'grok',
'keep',
'mv_expand',
'rename',
'stats',
'limit',
]);
export const UNSUPPORTED_COMMANDS_BEFORE_MATCH = new Set(['limit']);
Loading

0 comments on commit f4af267

Please sign in to comment.