Skip to content

Commit

Permalink
[8.x] [ES|QL] Create expression type evaluator (#195989) (#196922)
Browse files Browse the repository at this point in the history
# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] Create expression type evaluator
(#195989)](#195989)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Drew
Tate","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-18T16:15:11Z","message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","Feature:ES|QL","Team:ESQL","v8.16.0","backport:version","v8.17.0"],"title":"[ES|QL]
Create expression type
evaluator","number":195989,"url":"https://github.com/elastic/kibana/pull/195989","mergeCommit":{"message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24"}},"sourceBranch":"main","suggestedTargetBranches":["8.16","8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195989","number":195989,"mergeCommit":{"message":"[ES|QL]
Create expression type evaluator (#195989)\n\n## Summary\r\n\r\nClose
https://github.com/elastic/kibana/issues/195682\r\nClose
https://github.com/elastic/kibana/issues/195430\r\n\r\nIntroduces
`getExpressionType`, the ES|QL expression type evaluator to\r\nrule them
all!\r\n\r\nAlso, fixes several validation bugs related to the faulty
logic that\r\nexisted before with variable type detection (some noted
in\r\nhttps://github.com//issues/192255#issuecomment-2394613881).\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"2173af79fde374008b181ca42cf98a7137a7bb24"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Drew Tate <[email protected]>
  • Loading branch information
kibanamachine and drewdaemon authored Oct 18, 2024
1 parent 125e7fe commit 070ea62
Show file tree
Hide file tree
Showing 30 changed files with 609 additions and 270 deletions.
4 changes: 2 additions & 2 deletions packages/kbn-esql-ast/src/parser/__tests__/literal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('literal expression', () => {
});
});

it('decimals vs integers', () => {
it('doubles vs integers', () => {
const text = 'ROW a(1.0, 1)';
const { ast } = parse(text);

Expand All @@ -36,7 +36,7 @@ describe('literal expression', () => {
args: [
{
type: 'literal',
literalType: 'decimal',
literalType: 'double',
},
{
type: 'literal',
Expand Down
9 changes: 5 additions & 4 deletions packages/kbn-esql-ast/src/parser/factories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import type {
FunctionSubtype,
ESQLNumericLiteral,
ESQLOrderExpression,
InlineCastingType,
} from '../types';
import { parseIdentifier, getPosition } from './helpers';
import { Builder, type AstNodeParserFields } from '../builder';
Expand Down Expand Up @@ -72,7 +73,7 @@ export const createCommand = (name: string, ctx: ParserRuleContext) =>

export const createInlineCast = (ctx: InlineCastContext, value: ESQLInlineCast['value']) =>
Builder.expression.inlineCast(
{ castType: ctx.dataType().getText(), value },
{ castType: ctx.dataType().getText().toLowerCase() as InlineCastingType, value },
createParserFields(ctx)
);

Expand Down Expand Up @@ -107,7 +108,7 @@ export function createLiteralString(token: Token): ESQLLiteral {
const text = token.text!;
return {
type: 'literal',
literalType: 'string',
literalType: 'keyword',
text,
name: text,
value: text,
Expand Down Expand Up @@ -149,13 +150,13 @@ export function createLiteral(
location: getPosition(node.symbol),
incomplete: isMissingText(text),
};
if (type === 'decimal' || type === 'integer') {
if (type === 'double' || type === 'integer') {
return {
...partialLiteral,
literalType: type,
value: Number(text),
paramType: 'number',
} as ESQLNumericLiteral<'decimal'> | ESQLNumericLiteral<'integer'>;
} as ESQLNumericLiteral<'double'> | ESQLNumericLiteral<'integer'>;
} else if (type === 'param') {
throw new Error('Should never happen');
}
Expand Down
14 changes: 7 additions & 7 deletions packages/kbn-esql-ast/src/parser/walkers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ function getConstant(ctx: ConstantContext): ESQLAstItem {

// Decimal type covers multiple ES|QL types: long, double, etc.
if (ctx instanceof DecimalLiteralContext) {
return createNumericLiteral(ctx.decimalValue(), 'decimal');
return createNumericLiteral(ctx.decimalValue(), 'double');
}

// Integer type encompasses integer
Expand All @@ -358,7 +358,7 @@ function getConstant(ctx: ConstantContext): ESQLAstItem {
}
if (ctx instanceof StringLiteralContext) {
// String literal covers multiple ES|QL types: text and keyword types
return createLiteral('string', ctx.string_().QUOTED_STRING());
return createLiteral('keyword', ctx.string_().QUOTED_STRING());
}
if (
ctx instanceof NumericArrayLiteralContext ||
Expand All @@ -371,14 +371,14 @@ function getConstant(ctx: ConstantContext): ESQLAstItem {
const isDecimal =
numericValue.decimalValue() !== null && numericValue.decimalValue() !== undefined;
const value = numericValue.decimalValue() || numericValue.integerValue();
values.push(createNumericLiteral(value!, isDecimal ? 'decimal' : 'integer'));
values.push(createNumericLiteral(value!, isDecimal ? 'double' : 'integer'));
}
for (const booleanValue of ctx.getTypedRuleContexts(BooleanValueContext)) {
values.push(getBooleanValue(booleanValue)!);
}
for (const string of ctx.getTypedRuleContexts(StringContext)) {
// String literal covers multiple ES|QL types: text and keyword types
const literal = createLiteral('string', string.QUOTED_STRING());
const literal = createLiteral('keyword', string.QUOTED_STRING());
if (literal) {
values.push(literal);
}
Expand Down Expand Up @@ -534,7 +534,7 @@ function collectRegexExpression(ctx: BooleanExpressionContext): ESQLFunction[] {
const arg = visitValueExpression(regex.valueExpression());
if (arg) {
fn.args.push(arg);
const literal = createLiteral('string', regex._pattern.QUOTED_STRING());
const literal = createLiteral('keyword', regex._pattern.QUOTED_STRING());
if (literal) {
fn.args.push(literal);
}
Expand Down Expand Up @@ -672,7 +672,7 @@ export function visitDissect(ctx: DissectCommandContext) {
return [
visitPrimaryExpression(ctx.primaryExpression()),
...(pattern && textExistsAndIsValid(pattern.getText())
? [createLiteral('string', pattern), ...visitDissectOptions(ctx.commandOptions())]
? [createLiteral('keyword', pattern), ...visitDissectOptions(ctx.commandOptions())]
: []),
].filter(nonNullable);
}
Expand All @@ -682,7 +682,7 @@ export function visitGrok(ctx: GrokCommandContext) {
return [
visitPrimaryExpression(ctx.primaryExpression()),
...(pattern && textExistsAndIsValid(pattern.getText())
? [createLiteral('string', pattern)]
? [createLiteral('keyword', pattern)]
: []),
].filter(nonNullable);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ ROW
// 2
/* 3 */
// 4
/* 5 */ /* 6 */ 1::INTEGER /* 7 */ /* 8 */ // 9`);
/* 5 */ /* 6 */ 1::integer /* 7 */ /* 8 */ // 9`);
});
});

Expand Down
4 changes: 2 additions & 2 deletions packages/kbn-esql-ast/src/pretty_print/leaf_printer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ export const LeafPrinter = {
return '?';
}
}
case 'string': {
case 'keyword': {
return String(node.value);
}
case 'decimal': {
case 'double': {
const isRounded = node.value % 1 === 0;

if (isRounded) {
Expand Down
31 changes: 27 additions & 4 deletions packages/kbn-esql-ast/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,33 @@ export type BinaryExpressionAssignmentOperator = '=';
export type BinaryExpressionComparisonOperator = '==' | '=~' | '!=' | '<' | '<=' | '>' | '>=';
export type BinaryExpressionRegexOperator = 'like' | 'not_like' | 'rlike' | 'not_rlike';

// from https://github.com/elastic/elasticsearch/blob/122e7288200ee03e9087c98dff6cebbc94e774aa/docs/reference/esql/functions/kibana/inline_cast.json
export type InlineCastingType =
| 'bool'
| 'boolean'
| 'cartesian_point'
| 'cartesian_shape'
| 'date_nanos'
| 'date_period'
| 'datetime'
| 'double'
| 'geo_point'
| 'geo_shape'
| 'int'
| 'integer'
| 'ip'
| 'keyword'
| 'long'
| 'string'
| 'text'
| 'time_duration'
| 'unsigned_long'
| 'version';

export interface ESQLInlineCast<ValueType = ESQLAstItem> extends ESQLAstBaseItem {
type: 'inlineCast';
value: ValueType;
castType: string;
castType: InlineCastingType;
}

/**
Expand Down Expand Up @@ -270,7 +293,7 @@ export interface ESQLList extends ESQLAstBaseItem {
values: ESQLLiteral[];
}

export type ESQLNumericLiteralType = 'decimal' | 'integer';
export type ESQLNumericLiteralType = 'double' | 'integer';

export type ESQLLiteral =
| ESQLDecimalLiteral
Expand All @@ -290,7 +313,7 @@ export interface ESQLNumericLiteral<T extends ESQLNumericLiteralType> extends ES
}
// We cast anything as decimal (e.g. 32.12) as generic decimal numeric type here
// @internal
export type ESQLDecimalLiteral = ESQLNumericLiteral<'decimal'>;
export type ESQLDecimalLiteral = ESQLNumericLiteral<'double'>;

// @internal
export type ESQLIntegerLiteral = ESQLNumericLiteral<'integer'>;
Expand All @@ -312,7 +335,7 @@ export interface ESQLNullLiteral extends ESQLAstBaseItem {
// @internal
export interface ESQLStringLiteral extends ESQLAstBaseItem {
type: 'literal';
literalType: 'string';
literalType: 'keyword';
value: string;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-esql-ast/src/visitor/contexts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ export class LimitCommandVisitorContext<
if (
arg &&
arg.type === 'literal' &&
(arg.literalType === 'integer' || arg.literalType === 'decimal')
(arg.literalType === 'integer' || arg.literalType === 'double')
) {
return arg;
}
Expand Down
20 changes: 10 additions & 10 deletions packages/kbn-esql-ast/src/walker/walker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ describe('structurally can walk all nodes', () => {
},
{
type: 'literal',
literalType: 'string',
literalType: 'keyword',
name: '"foo"',
},
{
Expand Down Expand Up @@ -375,7 +375,7 @@ describe('structurally can walk all nodes', () => {
},
{
type: 'literal',
literalType: 'string',
literalType: 'keyword',
name: '"2"',
},
{
Expand All @@ -390,7 +390,7 @@ describe('structurally can walk all nodes', () => {
},
{
type: 'literal',
literalType: 'decimal',
literalType: 'double',
name: '3.14',
},
]);
Expand Down Expand Up @@ -473,7 +473,7 @@ describe('structurally can walk all nodes', () => {
values: [
{
type: 'literal',
literalType: 'decimal',
literalType: 'double',
name: '3.3',
},
],
Expand All @@ -492,7 +492,7 @@ describe('structurally can walk all nodes', () => {
},
{
type: 'literal',
literalType: 'decimal',
literalType: 'double',
name: '3.3',
},
]);
Expand Down Expand Up @@ -600,27 +600,27 @@ describe('structurally can walk all nodes', () => {
expect(literals).toMatchObject([
{
type: 'literal',
literalType: 'string',
literalType: 'keyword',
name: '"a"',
},
{
type: 'literal',
literalType: 'string',
literalType: 'keyword',
name: '"b"',
},
{
type: 'literal',
literalType: 'string',
literalType: 'keyword',
name: '"c"',
},
{
type: 'literal',
literalType: 'string',
literalType: 'keyword',
name: '"d"',
},
{
type: 'literal',
literalType: 'string',
literalType: 'keyword',
name: '"e"',
},
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const extraFunctions: FunctionDefinition[] = [
{ name: 'value', type: 'any' },
],
minParams: 2,
returnType: 'any',
returnType: 'unknown',
},
],
examples: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,6 @@ describe('autocomplete.suggest', () => {
...getFieldNamesByType('integer'),
...getFieldNamesByType('double'),
...getFieldNamesByType('long'),
'`avg(b)`',
...getFunctionSignaturesByReturnType('eval', ['integer', 'double', 'long'], {
scalar: true,
}),
Expand All @@ -284,11 +283,19 @@ describe('autocomplete.suggest', () => {
const { assertSuggestions } = await setup();

await assertSuggestions('from a | stats avg(b) by doubleField % 2 /', [',', '| ']);
await assertSuggestions('from a | stats avg(b) by doubleField % 2 /', [',', '| '], {
triggerCharacter: ' ',
});

await assertSuggestions(
'from a | stats var0 = AVG(doubleField) BY var1 = BUCKET(dateField, 1 day)/',
[',', '| ', '+ $0', '- $0']
);
await assertSuggestions(
'from a | stats var0 = AVG(doubleField) BY var1 = BUCKET(dateField, 1 day) /',
[',', '| ', '+ $0', '- $0'],
{ triggerCharacter: ' ' }
);
});
test('on space within bucket()', async () => {
const { assertSuggestions } = await setup();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1780,10 +1780,15 @@ async function getOptionArgsSuggestions(
innerText,
command,
option,
{ type: argDef?.type || 'any' },
{ type: argDef?.type || 'unknown' },
nodeArg,
nodeArgType as string,
references,
{
fields: references.fields,
// you can't use a variable defined
// in the stats command in the by clause
variables: new Map(),
},
getFieldsByType
))
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ export const getBuiltinCompatibleFunctionDefinition = (
({ name, supportedCommands, supportedOptions, signatures, ignoreAsSuggestion }) =>
(command === 'where' && commandsToInclude ? commandsToInclude.indexOf(name) > -1 : true) &&
!ignoreAsSuggestion &&
!/not_/.test(name) &&
(!skipAssign || name !== '=') &&
(option ? supportedOptions?.includes(option) : supportedCommands.includes(command)) &&
signatures.some(
Expand All @@ -79,7 +78,10 @@ export const getBuiltinCompatibleFunctionDefinition = (
return compatibleFunctions
.filter((mathDefinition) =>
mathDefinition.signatures.some(
(signature) => returnTypes[0] === 'any' || returnTypes.includes(signature.returnType)
(signature) =>
returnTypes[0] === 'unknown' ||
returnTypes[0] === 'any' ||
returnTypes.includes(signature.returnType)
)
)
.map(getSuggestionBuiltinDefinition);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,23 +52,6 @@ export function getFunctionsToIgnoreForStats(command: ESQLCommand, argIndex: num
return isFunctionItem(arg) ? getFnContent(arg) : [];
}

/**
* Given a function signature, returns the parameter at the given position.
*
* Takes into account variadic functions (minParams), returning the last
* parameter if the position is greater than the number of parameters.
*
* @param signature
* @param position
* @returns
*/
export function getParamAtPosition(
{ params, minParams }: FunctionDefinition['signatures'][number],
position: number
) {
return params.length > position ? params[position] : minParams ? params[params.length - 1] : null;
}

/**
* Given a function signature, returns the parameter at the given position, even if it's undefined or null
*
Expand Down
Loading

0 comments on commit 070ea62

Please sign in to comment.