[8.x] [ES|QL] only suggest pipe at the end of the field list (#195679)

packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts

@@ -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');
+      });
     });
   }
 
@@ -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,
@@ -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,
@@ -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,
@@ -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,
@@ -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' },
+            ],
+          ]
+        );
       });
     });
   });

packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts

@@ -627,7 +627,7 @@ async function getExpressionSuggestionsByType(
             literals: argDef.constantOnly,
           },
           {
-            ignoreFields: isNewExpression
+            ignoreColumns: isNewExpression
               ? command.args.filter(isColumnItem).map(({ name }) => name)
               : [],
           }
@@ -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,
@@ -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',
         })
@@ -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);
       }
     }
@@ -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)] }
           ))
         );
       }
@@ -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