Skip to content

Commit

Permalink
[ES|QL] Prettiffying the query (#191269)
Browse files Browse the repository at this point in the history
## Summary

The wrap by pipes CTA doesnt work correctly when pipes are being used
inside functions or in a comment in an ES|QL query.

This PR fixes this bug by using the prettifying api. I was thinking to
change the tooltip text to mention that it doesnt only wrap by pipes but
also prettifies but I decided against it and kept the tooltip as it is
for 2 reasons:

- this is the most important thing that it does
- prettifying means nothing for the majority of our users


![meow](https://github.com/user-attachments/assets/c2189218-4367-4d4a-9f9e-2d6f88f731d2)

### Checklist

- [ ] [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 <elasticmachine@users.noreply.github.com>
  • Loading branch information
stratoula and elasticmachine authored Sep 10, 2024
1 parent f23c899 commit 7cc3eae
Show file tree
Hide file tree
Showing 8 changed files with 66 additions and 48 deletions.
2 changes: 2 additions & 0 deletions packages/kbn-esql-ast/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,5 @@ export { getAstAndSyntaxErrors } from './src/ast_parser';
export { ESQLErrorListener } from './src/antlr_error_listener';

export { Walker, type WalkerOptions, walk } from './src/walker';

export { BasicPrettyPrinter } from './src/pretty_print/basic_pretty_printer';
2 changes: 2 additions & 0 deletions packages/kbn-esql-utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ export {
getTimeFieldFromESQLQuery,
getStartEndParams,
hasStartEndParams,
prettifyQuery,
isQueryWrappedByPipes,
retieveMetadataColumns,
TextBasedLanguages,
} from './src';
Expand Down
2 changes: 2 additions & 0 deletions packages/kbn-esql-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export {
removeDropCommandsFromESQLQuery,
hasTransformationalCommand,
getTimeFieldFromESQLQuery,
prettifyQuery,
isQueryWrappedByPipes,
retieveMetadataColumns,
} from './utils/query_parsing_helpers';
export { appendToESQLQuery, appendWhereClauseToESQLQuery } from './utils/append_to_query';
Expand Down
39 changes: 39 additions & 0 deletions packages/kbn-esql-utils/src/utils/query_parsing_helpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
removeDropCommandsFromESQLQuery,
hasTransformationalCommand,
getTimeFieldFromESQLQuery,
prettifyQuery,
isQueryWrappedByPipes,
retieveMetadataColumns,
} from './query_parsing_helpers';

Expand Down Expand Up @@ -178,6 +180,43 @@ describe('esql query helpers', () => {
});
});

describe('prettifyQuery', function () {
it('should return the code wrapped', function () {
const code = prettifyQuery('FROM index1 | KEEP field1, field2 | SORT field1', false);
expect(code).toEqual('FROM index1\n | KEEP field1, field2\n | SORT field1');
});

it('should return the code unwrapped', function () {
const code = prettifyQuery('FROM index1 \n| KEEP field1, field2 \n| SORT field1', true);
expect(code).toEqual('FROM index1 | KEEP field1, field2 | SORT field1');
});

it('should return the code unwrapped and trimmed', function () {
const code = prettifyQuery(
'FROM index1 \n| KEEP field1, field2 \n| SORT field1',
true
);
expect(code).toEqual('FROM index1 | KEEP field1, field2 | SORT field1');
});
});

describe('isQueryWrappedByPipes', function () {
it('should return false if the query is not wrapped', function () {
const flag = isQueryWrappedByPipes('FROM index1 | KEEP field1, field2 | SORT field1');
expect(flag).toBeFalsy();
});

it('should return true if the query is wrapped', function () {
const flag = isQueryWrappedByPipes('FROM index1 /n| KEEP field1, field2 /n| SORT field1');
expect(flag).toBeTruthy();
});

it('should return true if the query is wrapped and prettified', function () {
const flag = isQueryWrappedByPipes('FROM index1 /n | KEEP field1, field2 /n | SORT field1');
expect(flag).toBeTruthy();
});
});

describe('retieveMetadataColumns', () => {
it('should return metadata columns if they exist', () => {
expect(retieveMetadataColumns('from a metadata _id, _ignored | eval b = 1')).toStrictEqual([
Expand Down
14 changes: 13 additions & 1 deletion packages/kbn-esql-utils/src/utils/query_parsing_helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* 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 { getAstAndSyntaxErrors, Walker, walk, BasicPrettyPrinter } from '@kbn/esql-ast';

import type {
ESQLSource,
Expand All @@ -14,7 +15,6 @@ import type {
ESQLSingleAstItem,
ESQLCommandOption,
} from '@kbn/esql-ast';
import { getAstAndSyntaxErrors, Walker, walk } from '@kbn/esql-ast';

const DEFAULT_ESQL_LIMIT = 1000;

Expand Down Expand Up @@ -114,6 +114,18 @@ export const getTimeFieldFromESQLQuery = (esql: string) => {
return column?.name;
};

export const isQueryWrappedByPipes = (query: string): boolean => {
const { ast } = getAstAndSyntaxErrors(query);
const numberOfCommands = ast.length;
const pipesWithNewLine = query.split('\n |');
return numberOfCommands === pipesWithNewLine?.length;
};

export const prettifyQuery = (query: string, isWrapped: boolean): string => {
const { ast } = getAstAndSyntaxErrors(query);
return BasicPrettyPrinter.print(ast, { multiline: !isWrapped });
};

export const retieveMetadataColumns = (esql: string): string[] => {
const { ast } = getAstAndSyntaxErrors(esql);
const options: ESQLCommandOption[] = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import React, { useMemo } from 'react';
import { i18n } from '@kbn/i18n';
import { EuiFlexItem, EuiToolTip, EuiButtonIcon } from '@elastic/eui';
import { getWrappedInPipesCode } from '../helpers';
import { prettifyQuery, isQueryWrappedByPipes } from '@kbn/esql-utils';

export function QueryWrapComponent({
code,
Expand All @@ -19,18 +19,16 @@ export function QueryWrapComponent({
code: string;
updateQuery: (qs: string) => void;
}) {
const isWrappedInPipes = useMemo(() => {
const pipes = code.split('|');
const pipesWithNewLine = code?.split('\n|');
return pipes?.length === pipesWithNewLine?.length;
const isWrappedByPipes = useMemo(() => {
return isQueryWrappedByPipes(code);
}, [code]);

return (
<EuiFlexItem grow={false}>
<EuiToolTip
position="top"
content={
isWrappedInPipes
isWrappedByPipes
? i18n.translate(
'textBasedEditor.query.textBasedLanguagesEditor.disableWordWrapLabel',
{
Expand All @@ -43,12 +41,12 @@ export function QueryWrapComponent({
}
>
<EuiButtonIcon
iconType={isWrappedInPipes ? 'pipeNoBreaks' : 'pipeBreaks'}
iconType={isWrappedByPipes ? 'pipeNoBreaks' : 'pipeBreaks'}
color="text"
size="xs"
data-test-subj="TextBasedLangEditor-toggleWordWrap"
aria-label={
isWrappedInPipes
isWrappedByPipes
? i18n.translate(
'textBasedEditor.query.textBasedLanguagesEditor.disableWordWrapLabel',
{
Expand All @@ -63,7 +61,7 @@ export function QueryWrapComponent({
)
}
onClick={() => {
const updatedCode = getWrappedInPipesCode(code, isWrappedInPipes);
const updatedCode = prettifyQuery(code, isWrappedByPipes);
if (code !== updatedCode) {
updateQuery(updatedCode);
}
Expand Down
31 changes: 1 addition & 30 deletions packages/kbn-text-based-editor/src/helpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,7 @@
*/

import { dataViewPluginMocks } from '@kbn/data-views-plugin/public/mocks';
import {
parseErrors,
parseWarning,
getWrappedInPipesCode,
getIndicesList,
getRemoteIndicesList,
} from './helpers';
import { parseErrors, parseWarning, getIndicesList, getRemoteIndicesList } from './helpers';

describe('helpers', function () {
describe('parseErrors', function () {
Expand Down Expand Up @@ -210,29 +204,6 @@ describe('helpers', function () {
});
});

describe('getWrappedInPipesCode', function () {
it('should return the code wrapped', function () {
const code = getWrappedInPipesCode('FROM index1 | keep field1, field2 | order field1', false);
expect(code).toEqual('FROM index1\n| keep field1, field2\n| order field1');
});

it('should return the code unwrapped', function () {
const code = getWrappedInPipesCode(
'FROM index1 \n| keep field1, field2 \n| order field1',
true
);
expect(code).toEqual('FROM index1 | keep field1, field2 | order field1');
});

it('should return the code unwrapped and trimmed', function () {
const code = getWrappedInPipesCode(
'FROM index1 \n| keep field1, field2 \n| order field1',
true
);
expect(code).toEqual('FROM index1 | keep field1, field2 | order field1');
});
});

describe('getIndicesList', function () {
it('should return also system indices with hidden flag on', async function () {
const dataViewsMock = dataViewPluginMocks.createStartContract();
Expand Down
8 changes: 0 additions & 8 deletions packages/kbn-text-based-editor/src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,14 +196,6 @@ export const getDocumentationSections = async (language: string) => {
}
};

export const getWrappedInPipesCode = (code: string, isWrapped: boolean): string => {
const pipes = code?.split('|');
const codeNoLines = pipes?.map((pipe) => {
return pipe.replaceAll('\n', '').trim();
});
return codeNoLines.join(isWrapped ? ' | ' : '\n| ');
};

export const getIndicesList = async (dataViews: DataViewsPublicPluginStart) => {
const indices = await dataViews.getIndices({
showAllIndices: false,
Expand Down

0 comments on commit 7cc3eae

Please sign in to comment.