Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ES|QL] add validation for VALUES and MV_SORT #179874

Merged
merged 29 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
7915807
add values definition
drewdaemon Apr 2, 2024
81da135
Merge branch 'main' into add-values-definition
stratoula Apr 3, 2024
6a70189
add validation for mv_sort
drewdaemon Apr 3, 2024
11485e2
Merge branch 'add-values-definition' of github.com:drewdaemon/kibana …
drewdaemon Apr 3, 2024
ae67251
add validation tests
drewdaemon Apr 3, 2024
8469be6
Merge branch 'main' into add-values-definition
stratoula Apr 5, 2024
881c265
fix i18n string
drewdaemon Apr 5, 2024
f6368c5
Merge branch 'add-values-definition' of github.com:drewdaemon/kibana …
drewdaemon Apr 5, 2024
3247190
Merge branch 'main' into add-values-definition
kibanamachine Apr 8, 2024
9778c7d
move type helpers
drewdaemon Apr 8, 2024
42d3832
add autocomplete suggestions
drewdaemon Apr 8, 2024
10e1c00
account for minParams in autocomplete tests
drewdaemon Apr 8, 2024
954d4b5
fix to_version definition
drewdaemon Apr 8, 2024
9bdde37
leverage existing factory
drewdaemon Apr 8, 2024
4b9feaf
update literal type
drewdaemon Apr 8, 2024
90baa5b
fix typings
drewdaemon Apr 8, 2024
979e40e
Merge branch 'main' into add-values-definition
kibanamachine Apr 8, 2024
e5a2ae7
Merge branch 'main' into add-values-definition
stratoula Apr 9, 2024
e21e5dd
fix value suggestions
drewdaemon Apr 9, 2024
d0d5caa
Merge branch 'add-values-definition' of github.com:drewdaemon/kibana …
drewdaemon Apr 9, 2024
acff5e7
mark type-specific literals as internal
drewdaemon Apr 9, 2024
7de22de
pick => omit
drewdaemon Apr 9, 2024
ce13b6c
move some code into a separate function
drewdaemon Apr 9, 2024
2ac7a3a
Add todo
drewdaemon Apr 9, 2024
e17547e
Merge branch 'main' into add-values-definition
kibanamachine Apr 9, 2024
243bab5
Merge branch 'main' into add-values-definition
kibanamachine Apr 9, 2024
98c6b5f
test with capitalized options
drewdaemon Apr 10, 2024
3d8ab9c
Merge branch 'add-values-definition' of github.com:drewdaemon/kibana …
drewdaemon Apr 10, 2024
4a1e176
Merge branch 'main' into add-values-definition
kibanamachine Apr 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions packages/kbn-esql-ast/src/ast_helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,25 @@ export function createLiteral(
return;
}
const text = node.getText();
return {

const partialLiteral: Omit<ESQLLiteral, 'literalType' | 'value'> = {
type: 'literal',
literalType: type,
text,
name: text,
value: type === 'number' ? Number(text) : text,
location: getPosition(node.symbol),
incomplete: isMissingText(node.getText()),
incomplete: isMissingText(text),
};
if (type === 'number') {
return {
...partialLiteral,
literalType: type,
value: Number(text),
};
}
return {
...partialLiteral,
literalType: type,
value: text,
};
}

Expand Down
34 changes: 31 additions & 3 deletions packages/kbn-esql-ast/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,38 @@ export interface ESQLList extends ESQLAstBaseItem {
values: ESQLLiteral[];
}

export interface ESQLLiteral extends ESQLAstBaseItem {
export type ESQLLiteral =
| ESQLNumberLiteral
| ESQLBooleanLiteral
| ESQLNullLiteral
| ESQLStringLiteral;

// @internal
export interface ESQLNumberLiteral extends ESQLAstBaseItem {
type: 'literal';
literalType: 'string' | 'number' | 'boolean' | 'null';
value: string | number;
literalType: 'number';
value: number;
}

// @internal
export interface ESQLBooleanLiteral extends ESQLAstBaseItem {
type: 'literal';
literalType: 'boolean';
value: string;
}

// @internal
export interface ESQLNullLiteral extends ESQLAstBaseItem {
type: 'literal';
literalType: 'null';
value: string;
}

// @internal
export interface ESQLStringLiteral extends ESQLAstBaseItem {
type: 'literal';
literalType: 'string';
value: string;
}

export interface ESQLMessage {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ describe('autocomplete', () => {
// simulate the editor behaviour for sorting suggestions
.sort((a, b) => (a.sortText || '').localeCompare(b.sortText || ''));
for (const [index, receivedSuggestion] of suggestionInertTextSorted.entries()) {
if (typeof expected[index] === 'string') {
if (typeof expected[index] !== 'object') {
expect(receivedSuggestion.text).toEqual(expected[index]);
} else {
// check all properties that are defined in the expected suggestion
Expand Down Expand Up @@ -1054,38 +1054,47 @@ describe('autocomplete', () => {
if (fn.name !== 'auto_bucket') {
for (const signature of fn.signatures) {
signature.params.forEach((param, i) => {
if (i < signature.params.length - 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests were skipping the final parameter of each call signature. Updated to test all parameters of each function.

if (i < signature.params.length) {
const canHaveMoreArgs =
i + 1 < (signature.minParams ?? 0) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concat test then started failing because these weren't adding a comma to the expected results based on the presence of minParams = 2. Fixed now.

signature.params.filter(({ optional }, j) => !optional && j > i).length > i;
testSuggestions(
`from a | eval ${fn.name}(${Array(i).fill('field').join(', ')}${i ? ',' : ''} )`,
[
...getFieldNamesByType(param.type).map((f) => (canHaveMoreArgs ? `${f},` : f)),
...getFunctionSignaturesByReturnType(
'eval',
param.type,
{ evalMath: true },
undefined,
[fn.name]
).map((l) => (canHaveMoreArgs ? `${l},` : l)),
...getLiteralsByType(param.type).map((d) => (canHaveMoreArgs ? `${d},` : d)),
]
param.literalOptions?.length
? param.literalOptions.map((option) => `"${option}"${canHaveMoreArgs ? ',' : ''}`)
: [
...getFieldNamesByType(param.type).map((f) =>
canHaveMoreArgs ? `${f},` : f
),
...getFunctionSignaturesByReturnType(
'eval',
param.type,
{ evalMath: true },
undefined,
[fn.name]
).map((l) => (canHaveMoreArgs ? `${l},` : l)),
...getLiteralsByType(param.type).map((d) => (canHaveMoreArgs ? `${d},` : d)),
]
);
testSuggestions(
`from a | eval var0 = ${fn.name}(${Array(i).fill('field').join(', ')}${
i ? ',' : ''
} )`,
[
...getFieldNamesByType(param.type).map((f) => (canHaveMoreArgs ? `${f},` : f)),
...getFunctionSignaturesByReturnType(
'eval',
param.type,
{ evalMath: true },
undefined,
[fn.name]
).map((l) => (canHaveMoreArgs ? `${l},` : l)),
...getLiteralsByType(param.type).map((d) => (canHaveMoreArgs ? `${d},` : d)),
]
param.literalOptions?.length
? param.literalOptions.map((option) => `"${option}"${canHaveMoreArgs ? ',' : ''}`)
: [
...getFieldNamesByType(param.type).map((f) =>
canHaveMoreArgs ? `${f},` : f
),
...getFunctionSignaturesByReturnType(
'eval',
param.type,
{ evalMath: true },
undefined,
[fn.name]
).map((l) => (canHaveMoreArgs ? `${l},` : l)),
...getLiteralsByType(param.type).map((d) => (canHaveMoreArgs ? `${d},` : d)),
]
);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import {
buildVariablesDefinitions,
buildOptionDefinition,
buildSettingDefinitions,
buildValueDefinitions,
} from './factories';
import { EDITOR_MARKER, SINGLE_BACKTICK } from '../shared/constants';
import { getAstContext, removeMarkerArgFromArgsList } from '../shared/context';
Expand Down Expand Up @@ -1082,6 +1083,15 @@ async function getFunctionArgsSuggestions(
return [];
});

const literalOptions = fnDefinition.signatures.reduce<string[]>((acc, signature) => {
const literalOptionsForThisParameter = signature.params[argIndex]?.literalOptions;
return literalOptionsForThisParameter ? acc.concat(literalOptionsForThisParameter) : acc;
}, [] as string[]);

if (literalOptions.length) {
return buildValueDefinitions(literalOptions);
}

const arg = node.args[argIndex];

// the first signature is used as reference
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,16 @@ export const buildConstantsDefinitions = (
sortText: 'A',
}));

export const buildValueDefinitions = (values: string[]): SuggestionRawDefinition[] =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow up of this PR I would probably reuse this for chrono_literals and time_literals as well

values.map((value) => ({
label: `"${value}"`,
text: `"${value}"`,
detail: i18n.translate('kbn-esql-validation-autocomplete.esql.autocomplete.valueDefinition', {
defaultMessage: 'Literal value',
}),
kind: 'Value',
}));

export const buildNewVarDefinition = (label: string): SuggestionRawDefinition => {
return {
label,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,4 +175,22 @@ export const statsAggregationFunctionDefinitions: FunctionDefinition[] = [
},
],
},
{
name: 'values',
type: 'agg',
description: i18n.translate('kbn-esql-validation-autocomplete.esql.definitions.values', {
defaultMessage: 'Returns all values in a group as an array.',
}),
supportedCommands: ['stats'],
signatures: [
{
params: [{ name: 'expression', type: 'any', noNestingFunctions: true }],
returnType: 'any',
examples: [
'from index | stats all_agents=values(agents.keyword)',
'from index | stats all_sorted_agents=mv_sort(values(agents.keyword))',
],
},
],
},
]);
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ export const evalFunctionsDefinitions: FunctionDefinition[] = [
],
validate: validateLogFunctions,
},

{
name: 'log',
description: i18n.translate('kbn-esql-validation-autocomplete.esql.definitions.logDoc', {
Expand Down Expand Up @@ -481,14 +480,9 @@ export const evalFunctionsDefinitions: FunctionDefinition[] = [
signatures: [
{
params: [{ name: 'field', type: 'string' }],
returnType: 'version',
returnType: 'string',
examples: [`from index | EVAL version = to_version(stringField)`],
},
{
params: [{ name: 'field', type: 'version' }],
returnType: 'version',
examples: [`from index | EVAL version = to_version(versionField)`],
},
],
Comment on lines 480 to 486
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't using Elasticsearch types yet (look at #174710). So, version can just be string and that makes everything work again.

},
{
Expand Down Expand Up @@ -924,6 +918,30 @@ export const evalFunctionsDefinitions: FunctionDefinition[] = [
},
],
},
{
name: 'mv_sort',
description: i18n.translate('kbn-esql-validation-autocomplete.esql.definitions.mvSortDoc', {
defaultMessage: 'Sorts a multivalue expression in lexicographical order.',
}),
signatures: [
{
params: [
{ name: 'field', type: 'any' },
{
name: 'order',
type: 'string',
optional: true,
drewdaemon marked this conversation as resolved.
Show resolved Hide resolved
literalOptions: ['asc', 'desc'],
},
],
returnType: 'any',
examples: [
'row a = [4, 2, -3, 2] | eval sorted = mv_sort(a)',
'row a = ["b", "c", "a"] | sorted = mv_sort(a, "DESC")',
],
},
],
},
{
name: 'mv_avg',
description: i18n.translate('kbn-esql-validation-autocomplete.esql.definitions.mvAvgDoc', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,22 @@ export interface FunctionDefinition {
optional?: boolean;
noNestingFunctions?: boolean;
supportsWildcard?: boolean;
/**
* if set this indicates that the value must be a literal
* but can be any literal of the correct type
*/
literalOnly?: boolean;
/**
* if provided this means that the value must be one
* of the options in the array iff the value is a literal.
*
* String values are case insensitive.
*
* If the value is not a literal, this field is ignored because
* we can't check the return value of a function to see if it
* matches one of the options prior to runtime.
*/
literalOptions?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we call it values to align with the CommandDefinition thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify a bit? I'm thinking options better describes what this is. "value" implies what the user actually chooses, whereas "options" says to me a set of values they are allowed to choose.

}>;
minParams?: number;
returnType: string;
Expand Down Expand Up @@ -87,3 +102,5 @@ export type SignatureType =
| FunctionDefinition['signatures'][number]
| CommandOptionsDefinition['signature'];
export type SignatureArgType = SignatureType['params'][number];

export type FunctionArgSignature = FunctionDefinition['signatures'][number]['params'][number];
43 changes: 33 additions & 10 deletions packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
import type {
CommandDefinition,
CommandOptionsDefinition,
FunctionArgSignature,
FunctionDefinition,
SignatureArgType,
} from '../definitions/types';
Expand Down Expand Up @@ -179,6 +180,8 @@ export function getFunctionDefinition(name: string) {
return buildFunctionLookup().get(name.toLowerCase());
}

const unwrapStringLiteralQuotes = (value: string) => value.slice(1, -1);

function buildCommandLookup() {
if (!commandLookups) {
commandLookups = commandDefinitions.reduce((memo, def) => {
Expand Down Expand Up @@ -337,8 +340,28 @@ export function inKnownTimeInterval(item: ESQLTimeInterval): boolean {
return timeLiterals.some(({ name }) => name === item.unit.toLowerCase());
}

/**
* Checks if this argument is one of the possible options
* if they are defined on the arg definition.
*
* TODO - Consider merging with isEqualType to create a unified arg validation function
*/
export function isValidLiteralOption(arg: ESQLLiteral, argDef: FunctionArgSignature) {
return (
arg.literalType === 'string' &&
argDef.literalOptions &&
!argDef.literalOptions
.map((option) => option.toLowerCase())
.includes(unwrapStringLiteralQuotes(arg.value).toLowerCase())
);
}

/**
* Checks if an AST argument is of the correct type
* given the definition.
*/
export function isEqualType(
item: ESQLSingleAstItem,
arg: ESQLSingleAstItem,
argDef: SignatureArgType,
references: ReferenceMaps,
parentCommand?: string,
Expand All @@ -348,24 +371,24 @@ export function isEqualType(
if (argType === 'any') {
return true;
}
if (item.type === 'literal') {
return compareLiteralType(argType, item);
if (arg.type === 'literal') {
return compareLiteralType(argType, arg);
}
if (item.type === 'function') {
if (isSupportedFunction(item.name, parentCommand).supported) {
const fnDef = buildFunctionLookup().get(item.name)!;
if (arg.type === 'function') {
if (isSupportedFunction(arg.name, parentCommand).supported) {
const fnDef = buildFunctionLookup().get(arg.name)!;
return fnDef.signatures.some((signature) => argType === signature.returnType);
}
}
if (item.type === 'timeInterval') {
return argType === 'time_literal' && inKnownTimeInterval(item);
if (arg.type === 'timeInterval') {
return argType === 'time_literal' && inKnownTimeInterval(arg);
}
if (item.type === 'column') {
if (arg.type === 'column') {
if (argType === 'column') {
// anything goes, so avoid any effort here
return true;
}
const hit = getColumnHit(nameHit ?? item.name, references);
const hit = getColumnHit(nameHit ?? arg.name, references);
const validHit = hit;
if (!validHit) {
return false;
Expand Down
Loading