[ES|QL] Improve variable and field name handling #195149

drewdaemon · 2024-10-04T22:29:28Z

Summary

Closes #191111
Closes #191105

This change cleans up the way variables and fields were being stored and checked against. We now populate the field and variable cache with unescaped column names. This means that there are fewer cache misses because of escaped names checked against unescaped/differently-escaped names.

It also introduces a suite of tests for variable type detection. Most of those tests are currently skipped, but they are there to represent what should happen when we resolve #195682

User-facing improvements

Validation used to fail for field names with multiple escaped parts (e.g. `geo`.`dest`)
[ES|QL] columns with special characters are suggested twice #191105
Variables assigned the result of an inline cast used to get the wrong type.
Escaped field/variable suggestions now work with field list autocomplete

Screen.Recording.2024-10-09.at.9.32.23.AM.mov

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

…d-and-variable-representations

drewdaemon · 2024-10-09T14:49:36Z

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

+
+  const visitor = new Visitor()
+    .on('visitLiteralExpression', (ctx) => {
+      // TODO - add these as variables
+    })
+    .on('visitExpression', (_ctx) => {}) // required for the types :shrug:
+    .on('visitRenameExpression', (ctx) => {
+      const [oldArg, newArg] = ctx.node.args;
+      addToVariables(oldArg, newArg, fields, variables);
+    })
+    .on('visitFunctionCallExpression', (ctx) => {
+      if (ctx.node.name === '=') {
+        addVariableFromAssignment(ctx.node, variables, fields);
+      } else {
+        addVariableFromExpression(ctx.node, queryString, variables);
      }
-    }
-    if (command.name === 'enrich') {
-      const commandOptionsWithAssignment = command.args.filter(
-        (arg) => isOptionItem(arg) && arg.name === 'with'
-      ) as ESQLCommandOption[];
-      for (const commandOption of commandOptionsWithAssignment) {
-        // Enrich assignment has some special behaviour, so do not use the version above here...
-        for (const assignFn of commandOption.args) {
+    })
+    .on('visitCommandOption', (ctx) => {
+      if (ctx.node.name === 'by') {
+        return [...ctx.visitArguments()];
+      } else if (ctx.node.name === 'with') {
+        for (const assignFn of ctx.node.args) {
          if (isFunctionItem(assignFn)) {
            const [newArg, oldArg] = assignFn?.args || [];
+            // TODO why is oldArg an array?
            if (Array.isArray(oldArg)) {
              addToVariables(oldArg[0], newArg, fields, variables);
            }
          }
        }
      }
-    }
-    if (command.name === 'rename') {
-      const commandOptionsWithAssignment = command.args.filter(
-        (arg) => isOptionItem(arg) && arg.name === 'as'
-      ) as ESQLCommandOption[];
-      for (const commandOption of commandOptionsWithAssignment) {
-        const [oldArg, newArg] = commandOption.args;
-        addToVariables(oldArg, newArg, fields, variables);
+    })
+    .on('visitCommand', (ctx) => {
+      const ret = [];
+      if (['row', 'eval', 'stats', 'inlinestats', 'metrics', 'rename'].includes(ctx.node.name)) {
+        ret.push(...ctx.visitArgs());
      }
-    }
-  }
+      if (['stats', 'inlinestats', 'enrich'].includes(ctx.node.name)) {
+        // BY and WITH can contain variables
+        ret.push(...ctx.visitOptions());
+      }
+      return ret;
+    })
+    .on('visitQuery', (ctx) => [...ctx.visitCommands()]);
+
+  visitor.visitQuery(ast);


This is just a refactor. No functionality change

drewdaemon · 2024-10-09T14:54:48Z

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

+    if (root.castType === 'int') {
+      return 'integer';
    }
-    if (firstArg.type === 'literal') {
-      return firstArg.literalType;
+    if (root.castType === 'bool') {
+      return 'boolean';
    }


Will be nice to automate keeping this in sync in #186409

…d-and-variable-representations

elasticmachine · 2024-10-09T21:32:06Z

Pinging @elastic/kibana-esql (Team:ESQL)

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

stratoula

This looks ok and in some parts the code has been simplified which is good. There is a test failing though and seems related to the iteam you put on our agenda today (?)

stratoula · 2024-10-10T06:42:12Z

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

@@ -83,8 +83,6 @@ export const compareTypesWithLiterals = (
    return b === 'timeInterval';
  if (b === 'time_literal' || b === 'time_duration' || b === 'date_period')
    return a === 'timeInterval';
-  if (a === 'time_literal') return b === 'time_duration';
-  if (b === 'time_literal') return a === 'time_duration';


Why did we remove this?

It is covered in the 2 branches above, no?

Haha blindness! Correct!

stratoula · 2024-10-10T06:50:38Z

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

+ *
+ * TODO - this function needs a lot of work. For example, it needs to find the best-matching function signature
+ * which it isn't currently doing.
+ * @param root


ℹ️ this function signature here seems to miss some params and the returns type.

Removed in 9a52497

drewdaemon · 2024-10-10T15:35:45Z

@stratoula I don't see a failing test, but I will resolve the merge conflict!

…d-and-variable-representations

drewdaemon · 2024-10-10T21:57:20Z

@elasticmachine merge upstream

elasticmachine · 2024-10-10T23:45:32Z

💚 Build Succeeded

Buildkite Build
Commit: f64b387

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	5950	5958	+8
`unifiedSearch`	348	356	+8
total			+16

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/esql-validation-autocomplete`	192	190	-2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`kbnUiSharedDeps-srcJs`	3.4MB	3.4MB	+154.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/esql-validation-autocomplete`	204	202	-2

References to deprecated APIs

id	before	after	diff
`@kbn/esql-validation-autocomplete`	2	1	-1

stratoula

Thanx for fixing this Drew! LGTM!

kibanamachine · 2024-10-11T14:33:24Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11294248732

## Summary Closes elastic#191111 Closes elastic#191105 This change cleans up the way variables and fields were being stored and checked against. We now populate the field and variable cache with unescaped column names. This means that there are fewer cache misses because of escaped names checked against unescaped/differently-escaped names. It also introduces a [suite of tests](https://github.com/elastic/kibana/pull/195149/files#diff-6e4802e45f72257ca6f765bedd3ad65d2cbb587adb5befadb5f27ad5ab08a5a6R113) for variable type detection. Most of those tests are currently skipped, but they are there to represent what should happen when we resolve elastic#195682 User-facing improvements - Validation used to fail for field names with multiple escaped parts (e.g. `geo`.`dest`) - elastic#191105 - Variables assigned the result of an inline cast used to get the wrong type. - Escaped field/variable suggestions now work with field list autocomplete https://github.com/user-attachments/assets/2162f392-3ac3-4bb4-bf37-c73318c7f751 ### Checklist - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 <[email protected]> (cherry picked from commit 731c4e4)

kibanamachine · 2024-10-11T14:37:53Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…5936) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Improve variable and field name handling (#195149)](#195149)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Drew Tate <[email protected]>

drewdaemon added 10 commits October 4, 2024 16:28

storing all variables and field names in unescaped form

ac34364

add expression test cases

bb89a6c

add tests for variable type checking

9a3456f

rename file

783a185

fix rename and enrich variable escaping logic

8ad9959

fix grok and dissect

8a28383

Merge branch 'main' of github.com:elastic/kibana into 191111/fix-fiel…

aa6c515

…d-and-variable-representations

refactor variable traversal

89534e3

a bit more simplification

a5138cd

clarify some parameter checking

326075d

drewdaemon added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Oct 8, 2024

drewdaemon added 2 commits October 8, 2024 13:40

getExpressionType is its own function

bca3b86

Fix escaping of variable names

625b765

drewdaemon force-pushed the 191111/fix-field-and-variable-representations branch from 5cf9522 to 625b765 Compare October 9, 2024 14:38

drewdaemon commented Oct 9, 2024

View reviewed changes

drewdaemon mentioned this pull request Oct 9, 2024

ES|QL] Client side validation bugs #192255

Open

12 tasks

drewdaemon added 4 commits October 9, 2024 09:56

partially support escaped fields in autocomplete

a6f6806

fix problem export

9ce6e12

Merge branch 'main' of github.com:elastic/kibana into 191111/fix-fiel…

5b2800d

…d-and-variable-representations

restore old meta tests

648dee5

drewdaemon marked this pull request as ready for review October 9, 2024 21:31

drewdaemon requested a review from a team as a code owner October 9, 2024 21:31

drewdaemon added release_note:fix Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana labels Oct 9, 2024

qn895 reviewed Oct 9, 2024

View reviewed changes

packages/kbn-esql-validation-autocomplete/src/validation/__tests__/fields_and_variables.test.ts Show resolved Hide resolved

add some more todos

c906807

stratoula reviewed Oct 10, 2024

View reviewed changes

drewdaemon added 3 commits October 10, 2024 11:07

Merge branch 'main' of github.com:elastic/kibana into 191111/fix-fiel…

a2a3404

…d-and-variable-representations

support escaped backticks in autocomplete field lists

b291404

fix some docs

9a52497

elastic deleted a comment from elasticmachine Oct 10, 2024

Merge branch 'main' into 191111/fix-field-and-variable-representations

f64b387

elastic deleted a comment from elasticmachine Oct 10, 2024

stratoula approved these changes Oct 11, 2024

View reviewed changes

drewdaemon merged commit 731c4e4 into elastic:main Oct 11, 2024
20 checks passed

kibanamachine added the v9.0.0 label Oct 11, 2024

kibanamachine mentioned this pull request Oct 11, 2024

[8.x] [ES|QL] Improve variable and field name handling (#195149) #195936

Merged

kibanamachine added the v8.16.0 label Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Improve variable and field name handling #195149

[ES|QL] Improve variable and field name handling #195149

drewdaemon commented Oct 4, 2024 •

edited

Loading

drewdaemon Oct 9, 2024

drewdaemon Oct 9, 2024

elasticmachine commented Oct 9, 2024

stratoula left a comment

stratoula Oct 10, 2024

drewdaemon Oct 10, 2024

stratoula Oct 10, 2024

stratoula Oct 10, 2024

drewdaemon Oct 10, 2024

drewdaemon commented Oct 10, 2024

drewdaemon commented Oct 10, 2024

elasticmachine commented Oct 10, 2024

API count

References to deprecated APIs

stratoula left a comment

kibanamachine commented Oct 11, 2024

kibanamachine commented Oct 11, 2024

[ES|QL] Improve variable and field name handling #195149

[ES|QL] Improve variable and field name handling #195149

Conversation

drewdaemon commented Oct 4, 2024 • edited Loading

Summary

Checklist

drewdaemon Oct 9, 2024

Choose a reason for hiding this comment

drewdaemon Oct 9, 2024

Choose a reason for hiding this comment

elasticmachine commented Oct 9, 2024

stratoula left a comment

Choose a reason for hiding this comment

stratoula Oct 10, 2024

Choose a reason for hiding this comment

drewdaemon Oct 10, 2024

Choose a reason for hiding this comment

stratoula Oct 10, 2024

Choose a reason for hiding this comment

stratoula Oct 10, 2024

Choose a reason for hiding this comment

drewdaemon Oct 10, 2024

Choose a reason for hiding this comment

drewdaemon commented Oct 10, 2024

drewdaemon commented Oct 10, 2024

elasticmachine commented Oct 10, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Page load bundle

API count

References to deprecated APIs

stratoula left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 11, 2024

kibanamachine commented Oct 11, 2024

💚 All backports created successfully

Questions ?

drewdaemon commented Oct 4, 2024 •

edited

Loading