[ES|QL] Implement OrderExpression for SORT command arguments (ela…

…stic#189959) ## Summary Closes elastic#189491 - Adds *order expression* AST nodes, which are minted from `SORT` command. - Improves SORT command autocomplete suggestions. Shows fields on first space: <img width="791" alt="image" src="https://github.com/user-attachments/assets/3fec96b4-4e61-4212-a856-ace7a33d9755"> It now shows `NULLS FIRST` and `NULLS LAST`, even before `ASC` or `DESC` was entered, as `ASC` and `DESC` are optional: <img width="871" alt="image" src="https://github.com/user-attachments/assets/4b6d6c28-a7b0-4ac0-bafc-133df1207d54"> Once `ASC` or `DESC` is entered, shows only nulls options: <img width="911" alt="image" src="https://github.com/user-attachments/assets/5b27bd3d-ccdc-4bd0-b09f-fe65e5975e28"> It also now suggests partial modifier, if the in-progress text that user is typing matches it: <img width="504" alt="image" src="https://github.com/user-attachments/assets/9a047c40-b49b-4694-8477-7270cb9c0886"> (However, we are not triggering autocomplete in those cases in UI, so no way to see it in UI right now.) ### Checklist Delete any items that are not applicable to this PR. - [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 ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
hop-dev · Sep 19, 2024 · 2efd0f0 · 2efd0f0
1 parent 6f4be61
commit 2efd0f0
Show file tree

Hide file tree

Showing 18 changed files with 616 additions and 136 deletions.
diff --git a/packages/kbn-esql-ast/src/__tests__/ast_parser.sort.test.ts b/packages/kbn-esql-ast/src/__tests__/ast_parser.sort.test.ts
@@ -11,34 +11,107 @@ import { getAstAndSyntaxErrors as parse } from '../ast_parser';
 
 describe('SORT', () => {
  describe('correctly formatted', () => {
- // Un-skip one https://github.com/elastic/kibana/issues/189491 fixed.
- it.skip('example from documentation', () => {
+ it('sorting order without modifiers', () => {
+ const text = `FROM employees | SORT height`;
+ const { ast, errors } = parse(text);
+
+ expect(errors.length).toBe(0);
+ expect(ast).toMatchObject([
+ {},
+ {
+ type: 'command',
+ name: 'sort',
+ args: [
+ {
+ type: 'column',
+ name: 'height',
+ },
+ ],
+ },
+ ]);
+ });
+
+ it('sort expression is a function call', () => {
+ const text = `from a_index | sort values(textField)`;
+ const { ast, errors } = parse(text);
+
+ expect(errors.length).toBe(0);
+ expect(ast).toMatchObject([
+ {},
+ {
+ type: 'command',
+ name: 'sort',
+ args: [
+ {
+ type: 'function',
+ name: 'values',
+ },
+ ],
+ },
+ ]);
+ });
+
+ it('with order modifier "DESC"', () => {
  const text = `
  FROM employees
- | KEEP first_name, last_name, height
  | SORT height DESC
  `;
  const { ast, errors } = parse(text);
 
  expect(errors.length).toBe(0);
  expect(ast).toMatchObject([
  {},
+ {
+ type: 'command',
+ name: 'sort',
+ args: [
+ {
+ type: 'order',
+ order: 'DESC',
+ nulls: '',
+ args: [
+ {
+ type: 'column',
+ name: 'height',
+ },
+ ],
+ },
+ ],
+ },
+ ]);
+ });
+
+ it('with nulls modifier "NULLS LAST"', () => {
+ const text = `
+ FROM employees
+ | SORT height NULLS LAST
+ `;
+ const { ast, errors } = parse(text);
+
+ expect(errors.length).toBe(0);
+ expect(ast).toMatchObject([
  {},
  {
  type: 'command',
  name: 'sort',
  args: [
  {
- type: 'column',
- name: 'height',
+ type: 'order',
+ order: '',
+ nulls: 'NULLS LAST',
+ args: [
+ {
+ type: 'column',
+ name: 'height',
+ },
+ ],
  },
  ],
  },
  ]);
  });
 
- // Un-skip once https://github.com/elastic/kibana/issues/189491 fixed.
- it.skip('can parse various sorting columns with options', () => {
+ it('can parse various sorting columns with options', () => {
  const text =
  'FROM a | SORT a, b ASC, c DESC, d NULLS FIRST, e NULLS LAST, f ASC NULLS FIRST, g DESC NULLS LAST';
  const { ast, errors } = parse(text);
@@ -55,28 +128,58 @@ describe('SORT', () => {
  name: 'a',
  },
  {
- type: 'column',
- name: 'b',
+ order: 'ASC',
+ nulls: '',
+ args: [
+ {
+ name: 'b',
+ },
+ ],
  },
  {
- type: 'column',
- name: 'c',
+ order: 'DESC',
+ nulls: '',
+ args: [
+ {
+ name: 'c',
+ },
+ ],
  },
  {
- type: 'column',
- name: 'd',
+ order: '',
+ nulls: 'NULLS FIRST',
+ args: [
+ {
+ name: 'd',
+ },
+ ],
  },
  {
- type: 'column',
- name: 'e',
+ order: '',
+ nulls: 'NULLS LAST',
+ args: [
+ {
+ name: 'e',
+ },
+ ],
  },
  {
- type: 'column',
- name: 'f',
+ order: 'ASC',
+ nulls: 'NULLS FIRST',
+ args: [
+ {
+ name: 'f',
+ },
+ ],
  },
  {
- type: 'column',
- name: 'g',
+ order: 'DESC',
+ nulls: 'NULLS LAST',
+ args: [
+ {
+ name: 'g',
+ },
+ ],
  },
  ],
  },

diff --git a/packages/kbn-esql-ast/src/ast_factory.ts b/packages/kbn-esql-ast/src/ast_factory.ts
@@ -53,7 +53,7 @@ import {
  visitDissect,
  visitGrok,
  collectBooleanExpression,
- visitOrderExpression,
+ visitOrderExpressions,
  getPolicyName,
  getMatchField,
  getEnrichClauses,
@@ -238,7 +238,7 @@ export class AstListener implements ESQLParserListener {
  exitSortCommand(ctx: SortCommandContext) {
  const command = createCommand('sort', ctx);
  this.ast.push(command);
- command.args.push(...visitOrderExpression(ctx.orderExpression_list()));
+ command.args.push(...visitOrderExpressions(ctx.orderExpression_list()));
  }
 
  /**

diff --git a/packages/kbn-esql-ast/src/ast_helpers.ts b/packages/kbn-esql-ast/src/ast_helpers.ts
@@ -42,6 +42,7 @@ import type {
  ESQLNumericLiteralType,
  FunctionSubtype,
  ESQLNumericLiteral,
+ ESQLOrderExpression,
 } from './types';
 import { parseIdentifier } from './parser/helpers';
 
@@ -222,6 +223,26 @@ export function createFunction<Subtype extends FunctionSubtype>(
  return node;
 }
 
+export const createOrderExpression = (
+ ctx: ParserRuleContext,
+ arg: ESQLAstItem,
+ order: ESQLOrderExpression['order'],
+ nulls: ESQLOrderExpression['nulls']
+) => {
+ const node: ESQLOrderExpression = {
+ type: 'order',
+ name: '',
+ order,
+ nulls,
+ args: [arg],
+ text: ctx.getText(),
+ location: getPosition(ctx.start, ctx.stop),
+ incomplete: Boolean(ctx.exception),
+ };
+
+ return node;
+};
+
 function walkFunctionStructure(
  args: ESQLAstItem[],
  initialLocation: ESQLLocation,

diff --git a/packages/kbn-esql-ast/src/ast_walker.ts b/packages/kbn-esql-ast/src/ast_walker.ts
@@ -84,6 +84,7 @@ import {
  textExistsAndIsValid,
  createInlineCast,
  createUnknownItem,
+ createOrderExpression,
 } from './ast_helpers';
 import { getPosition } from './ast_position_utils';
 import {
@@ -97,6 +98,7 @@ import {
  ESQLUnnamedParamLiteral,
  ESQLPositionalParamLiteral,
  ESQLNamedParamLiteral,
+ ESQLOrderExpression,
 } from './types';
 
 export function collectAllSourceIdentifiers(ctx: FromCommandContext): ESQLAstItem[] {
@@ -608,34 +610,43 @@ export function visitByOption(
  return [option];
 }
 
-export function visitOrderExpression(ctx: OrderExpressionContext[]) {
- const ast: ESQLAstItem[] = [];
- for (const orderCtx of ctx) {
- const expression = collectBooleanExpression(orderCtx.booleanExpression());
- if (orderCtx._ordering) {
- const terminalNode =
- orderCtx.getToken(esql_parser.ASC, 0) || orderCtx.getToken(esql_parser.DESC, 0);
- const literal = createLiteral('string', terminalNode);
- if (literal) {
- expression.push(literal);
- }
- }
- if (orderCtx.NULLS()) {
- expression.push(createLiteral('string', orderCtx.NULLS()!)!);
- if (orderCtx._nullOrdering && orderCtx._nullOrdering.text !== '<first missing>') {
- const innerTerminalNode =
- orderCtx.getToken(esql_parser.FIRST, 0) || orderCtx.getToken(esql_parser.LAST, 0);
- const literal = createLiteral('string', innerTerminalNode);
- if (literal) {
- expression.push(literal);
- }
- }
- }
+const visitOrderExpression = (ctx: OrderExpressionContext): ESQLOrderExpression | ESQLAstItem => {
+ const arg = collectBooleanExpression(ctx.booleanExpression())[0];
 
- if (expression.length) {
- ast.push(...expression);
- }
+ let order: ESQLOrderExpression['order'] = '';
+ let nulls: ESQLOrderExpression['nulls'] = '';
+
+ const ordering = ctx._ordering?.text?.toUpperCase();
+
+ if (ordering) order = ordering as ESQLOrderExpression['order'];
+
+ const nullOrdering = ctx._nullOrdering?.text?.toUpperCase();
+
+ switch (nullOrdering) {
+ case 'LAST':
+ nulls = 'NULLS LAST';
+ break;
+ case 'FIRST':
+ nulls = 'NULLS FIRST';
+ break;
  }
+
+ if (!order && !nulls) {
+ return arg;
+ }
+
+ return createOrderExpression(ctx, arg, order, nulls);
+};
+
+export function visitOrderExpressions(
+ ctx: OrderExpressionContext[]
+): Array<ESQLOrderExpression | ESQLAstItem> {
+ const ast: Array<ESQLOrderExpression | ESQLAstItem> = [];
+
+ for (const orderCtx of ctx) {
+ ast.push(visitOrderExpression(orderCtx));
+ }
+
  return ast;
 }
 

diff --git a/packages/kbn-esql-ast/src/pretty_print/__tests__/basic_pretty_printer.test.ts b/packages/kbn-esql-ast/src/pretty_print/__tests__/basic_pretty_printer.test.ts
@@ -50,18 +50,22 @@ describe('single line query', () => {
  expect(text).toBe('FROM a | SORT b');
  });
 
- /** @todo Enable once order expressions are supported. */
- test.skip('order expression with ASC modifier', () => {
+ test('order expression with ASC modifier', () => {
  const { text } = reprint('FROM a | SORT b ASC');
 
  expect(text).toBe('FROM a | SORT b ASC');
  });
 
- /** @todo Enable once order expressions are supported. */
- test.skip('order expression with ASC and NULLS FIRST modifier', () => {
- const { text } = reprint('FROM a | SORT b ASC NULLS FIRST');
+ test('order expression with NULLS LAST modifier', () => {
+ const { text } = reprint('FROM a | SORT b NULLS LAST');
 
- expect(text).toBe('FROM a | SORT b ASC NULLS FIRST');
+ expect(text).toBe('FROM a | SORT b NULLS LAST');
+ });
+
+ test('order expression with DESC and NULLS FIRST modifier', () => {
+ const { text } = reprint('FROM a | SORT b DESC NULLS FIRST');
+
+ expect(text).toBe('FROM a | SORT b DESC NULLS FIRST');
  });
  });