Skip to content

Commit

Permalink
[ES|QL] only suggest pipe at the end of the field list (elastic#195679)
Browse files Browse the repository at this point in the history 
## Summary

Close elastic#191100

### Improvements

1. You no longer get a comma suggestion when you're out of fields...


https://github.com/user-attachments/assets/3ed3617b-99e2-44a5-917e-294b98f16ef4

2. Fixed elastic#191100


### Checklist

- [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

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
@drewdaemon @elasticmachine
drewdaemon and elasticmachine authored Oct 10, 2024
1 parent ed75ef6 commit dffe0b5
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 33 deletions.
83 changes: 66 additions & 17 deletions packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,23 @@ describe('autocomplete', () => {
'```````````````````````````````round(doubleField) + 1```````````````` + 1```````` + 1```` + 1`` + 1`',
]
);

it('should not suggest already-used fields and variables', async () => {
const { suggest: suggestTest } = await setup();
const getSuggestions = async (query: string) =>
(await suggestTest(query)).map((value) => value.text);

expect(await getSuggestions('from a_index | EVAL foo = 1 | KEEP /')).toContain('foo');
expect(await getSuggestions('from a_index | EVAL foo = 1 | KEEP foo, /')).not.toContain(
'foo'
);
expect(await getSuggestions('from a_index | EVAL foo = 1 | KEEP /')).toContain(
'doubleField'
);
expect(
await getSuggestions('from a_index | EVAL foo = 1 | KEEP doubleField, /')
).not.toContain('doubleField');
});
});
}

Expand Down Expand Up @@ -1111,11 +1128,14 @@ describe('autocomplete', () => {
]);
});

describe('KEEP <field>', () => {
describe.each(['KEEP', 'DROP'])('%s <field>', (commandName) => {
// KEEP field
testSuggestions('FROM a | KEEP /', getFieldNamesByType('any').map(attachTriggerCommand));
testSuggestions(
'FROM a | KEEP d/',
`FROM a | ${commandName} /`,
getFieldNamesByType('any').map(attachTriggerCommand)
);
testSuggestions(
`FROM a | ${commandName} d/`,
getFieldNamesByType('any')
.map<PartialSuggestionWithText>((text) => ({
text,
Expand All @@ -1124,11 +1144,11 @@ describe('autocomplete', () => {
.map(attachTriggerCommand)
);
testSuggestions(
'FROM a | KEEP doubleFiel/',
`FROM a | ${commandName} doubleFiel/`,
getFieldNamesByType('any').map(attachTriggerCommand)
);
testSuggestions(
'FROM a | KEEP doubleField/',
`FROM a | ${commandName} doubleField/`,
['doubleField, ', 'doubleField | ']
.map((text) => ({
text,
Expand All @@ -1141,7 +1161,7 @@ describe('autocomplete', () => {

// Let's get funky with the field names
testSuggestions(
'FROM a | KEEP @timestamp/',
`FROM a | ${commandName} @timestamp/`,
['@timestamp, ', '@timestamp | ']
.map((text) => ({
text,
Expand All @@ -1150,10 +1170,15 @@ describe('autocomplete', () => {
}))
.map(attachTriggerCommand),
undefined,
[[{ name: '@timestamp', type: 'date' }]]
[
[
{ name: '@timestamp', type: 'date' },
{ name: 'utc_stamp', type: 'date' },
],
]
);
testSuggestions(
'FROM a | KEEP foo.bar/',
`FROM a | ${commandName} foo.bar/`,
['foo.bar, ', 'foo.bar | ']
.map((text) => ({
text,
Expand All @@ -1162,39 +1187,63 @@ describe('autocomplete', () => {
}))
.map(attachTriggerCommand),
undefined,
[[{ name: 'foo.bar', type: 'double' }]]
[
[
{ name: 'foo.bar', type: 'double' },
{ name: 'baz', type: 'date' },
],
]
);

describe('escaped field names', () => {
// This isn't actually the behavior we want, but this test is here
// to make sure no weird suggestions start cropping up in this case.
testSuggestions('FROM a | KEEP `foo.bar`/', ['foo.bar'], undefined, [
testSuggestions(`FROM a | ${commandName} \`foo.bar\`/`, ['foo.bar'], undefined, [
[{ name: 'foo.bar', type: 'double' }],
]);
// @todo re-enable these tests when we can use AST to support this case
testSuggestions.skip('FROM a | KEEP `foo.bar`/', ['foo.bar, ', 'foo.bar | '], undefined, [
[{ name: 'foo.bar', type: 'double' }],
]);
testSuggestions.skip(
'FROM a | KEEP `foo`.`bar`/',
`FROM a | ${commandName} \`foo.bar\`/`,
['foo.bar, ', 'foo.bar | '],
undefined,
[[{ name: 'foo.bar', type: 'double' }]]
);
testSuggestions.skip('FROM a | KEEP `any#Char$Field`/', [
testSuggestions.skip(
`FROM a | ${commandName} \`foo\`.\`bar\`/`,
['foo.bar, ', 'foo.bar | '],
undefined,
[[{ name: 'foo.bar', type: 'double' }]]
);
testSuggestions.skip(`FROM a | ${commandName} \`any#Char$Field\`/`, [
'`any#Char$Field`, ',
'`any#Char$Field` | ',
]);
});

// Subsequent fields
testSuggestions(
'FROM a | KEEP doubleField, dateFiel/',
`FROM a | ${commandName} doubleField, dateFiel/`,
getFieldNamesByType('any')
.filter((s) => s !== 'doubleField')
.map(attachTriggerCommand)
);
testSuggestions('FROM a | KEEP doubleField, dateField/', ['dateField, ', 'dateField | ']);
testSuggestions(`FROM a | ${commandName} doubleField, dateField/`, [
'dateField, ',
'dateField | ',
]);

// out of fields
testSuggestions(
`FROM a | ${commandName} doubleField, dateField/`,
['dateField | '],
undefined,
[
[
{ name: 'doubleField', type: 'double' },
{ name: 'dateField', type: 'date' },
],
]
);
});
});
});
Expand Down
38 changes: 22 additions & 16 deletions packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ async function getExpressionSuggestionsByType(
literals: argDef.constantOnly,
},
{
ignoreFields: isNewExpression
ignoreColumns: isNewExpression
? command.args.filter(isColumnItem).map(({ name }) => name)
: [],
}
Expand Down Expand Up @@ -656,10 +656,15 @@ async function getExpressionSuggestionsByType(
}));
}

return [
{ ...pipeCompleteItem, text: ' | ' },
{ ...commaCompleteItem, text: ', ' },
].map<SuggestionRawDefinition>((s) => ({
const finalSuggestions = [{ ...pipeCompleteItem, text: ' | ' }];
if (fieldSuggestions.length > 1)
// when we fix the editor marker, this should probably be checked against 0 instead of 1
// this is because the last field in the AST is currently getting removed (because it contains
// the editor marker) so it is not included in the ignored list which is used to filter out
// existing fields above.
finalSuggestions.push({ ...commaCompleteItem, text: ', ' });

return finalSuggestions.map<SuggestionRawDefinition>((s) => ({
...s,
filterText: fragment,
text: fragment + s.text,
Expand Down Expand Up @@ -1176,15 +1181,15 @@ async function getFieldsOrFunctionsSuggestions(
},
{
ignoreFn = [],
ignoreFields = [],
ignoreColumns = [],
}: {
ignoreFn?: string[];
ignoreFields?: string[];
ignoreColumns?: string[];
} = {}
): Promise<SuggestionRawDefinition[]> {
const filteredFieldsByType = pushItUpInTheList(
(await (fields
? getFieldsByType(types, ignoreFields, {
? getFieldsByType(types, ignoreColumns, {
advanceCursor: commandName === 'sort',
openSuggestions: commandName === 'sort',
})
Expand All @@ -1195,7 +1200,10 @@ async function getFieldsOrFunctionsSuggestions(
const filteredVariablesByType: string[] = [];
if (variables) {
for (const variable of variables.values()) {
if (types.includes('any') || types.includes(variable[0].type)) {
if (
(types.includes('any') || types.includes(variable[0].type)) &&
!ignoreColumns.includes(variable[0].name)
) {
filteredVariablesByType.push(variable[0].name);
}
}
Expand Down Expand Up @@ -1515,7 +1523,7 @@ async function getListArgsSuggestions(
fields: true,
variables: anyVariables,
},
{ ignoreFields: [firstArg.name, ...otherArgs.map(({ name }) => name)] }
{ ignoreColumns: [firstArg.name, ...otherArgs.map(({ name }) => name)] }
))
);
}
Expand Down Expand Up @@ -1875,18 +1883,16 @@ async function getOptionArgsSuggestions(
* for a given fragment of text in a generic way. A good example is
* a field name.
*
* When typing a field name, there are three scenarios
* When typing a field name, there are 2 scenarios
*
* 1. user hasn't begun typing
* 1. field name is incomplete (includes the empty string)
* KEEP /
*
* 2. user is typing a partial field name
* KEEP fie/
*
* 3. user has typed a complete field name
* 2. field name is complete
* KEEP field/
*
* This function provides a framework for handling all three scenarios in a clean way.
* This function provides a framework for detecting and handling both scenarios in a clean way.
*
* @param innerText - the query text before the current cursor position
* @param isFragmentComplete — return true if the fragment is complete
Expand Down

0 comments on commit dffe0b5

Please sign in to comment.