[ES|QL] Fixing the suggestions in WHERE after a variable such as ?value by stratoula · Pull Request #222312 · elastic/kibana

stratoula · 2025-06-03T09:01:08Z

Summary

Closes #221776

Case 1: Suggestions after the system params

Case 2: Suggestions after a random named param

Case 3: Suggestions after field named param

Note: Fixes are being applied to eval too.

Some more technical info

The named params (such as ?value or ??field) were typed as unknown to our code. For are not offering suggestions for this type but named params are important for controls and we should not block the suggestions because the user used a named param.
This PR is changing the logic and treats the named params as of type param allowing the suggestions to follow.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

stratoula · 2025-06-03T12:50:49Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/autocomplete/helper.ts

      // and not directly after the column name or prefix e.g. "colu/"
-      !new RegExp(`${expressionRoot.parts.join('\\.')}$`).test(innerText)
+      // we are escaping the column name here as it may contain special characters such as ??
+      !new RegExp(`${escapeRegExp(expressionRoot.parts.join('\\.'))}$`).test(innerText)


This change is irrelevant with the bug we are trying to solve here but it solves another bug. An error appears in the console when you use the named params with the ?? (such as ... | WHERE ??field)

stratoula · 2025-06-04T06:57:34Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/helpers.ts

    return getExpressionType(root[0], fields, userDefinedColumns);
  }

-  if (isLiteralItem(root) && root.literalType !== 'param') {


This check was returning all the named params as of type unknown. We are filtering out suggestions for type unknown so with this change now we are returning the type param which is not filtered out.

elasticmachine · 2025-06-04T09:20:06Z

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

drewdaemon

You are being pragmatic and obviously we don't want a degraded experience for users who are using controls. I like here that you have scoped the change down to params and retained the existing behavior for invalid expressions (type unknown).

That said, my guess is that we will have to grapple with this again in the future.

For example, we filter function call signatures by the types of the existing parameters.

Say we have the FUNC function with two call signatures: keyword, keyword, and double, double.

In the current system, FUNC(keywordField, /) should only suggest keyword things, not doubles because of the type of the first parameter.

But what if the type of the first argument is "param" (FUNC(??field, /))?

Should we then be suggesting both double and keyword things? I didn't test it, but I doubt this works right now.

drewdaemon · 2025-06-04T19:54:13Z

src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/helpers.ts

+/**
+ * Type guard to check if the type is 'param'
+ */
+export const isParamExpressionType = (type: string): type is 'param' => type === 'param';


nit: what does having a function for this accomplish?

As a comparable: I think most of our isSomething AST helpers only exist because we currently have to make sure that the nodes aren't randomly arrays. I would love to see them disappear and just be able to say if (node.type === 'option') instead of if (isOptionItem(node))for example.

Oh interesting thought. I like these small functions for readability. So I prefer them from something === "param". I had them as that actually and I didnt like it.

I think if your utilities are well organized they can be really helpful for the code readability. Mostly because we have so many different types and terms. I always struggle reading this code and I feel that these small utilities help me.

I hear your point ofc. At the end of the day it is a personal preference. We should have a convention for sure. 👍

stratoula · 2025-06-05T05:58:06Z

@drewdaemon u r right. I did the minimum to make the params work with the most popular cases in WHERE. For the sake of ES|QL controls. There are other cases that are not handled. And they can't be handled in a bug fix PR.

My personal opinion after working on this code is that we need to rewrite this expression logic and have a plan for params. The code is very complex, you navigate from one function to another, there are so many things go on that is not easy to debug or fix or make enhancements. I have a note on looking into this and come with a strategy on how we should improve it.

kibanamachine · 2025-06-05T05:58:55Z

Starting backport for target branches: 8.19

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

…ue (elastic#222312) ## Summary Closes elastic#221776 **Case 1: Suggestions after the system params** <img width="925" alt="image" src="https://github.com/user-attachments/assets/496f0b57-a834-49df-9904-374cd433e370" /> **Case 2: Suggestions after a random named param** <img width="1009" alt="image" src="https://github.com/user-attachments/assets/d5c82af5-7677-41ad-ac9b-d0888bcc4520" /> **Case 3: Suggestions after field named param** <img width="469" alt="image" src="https://github.com/user-attachments/assets/827268f2-8abd-404a-b41a-71b1431c71fd" /> **Note:** Fixes are being applied to eval too. ### Some more technical info The named params (such as ?value or ??field) were typed as unknown to our code. For are not offering suggestions for this type but named params are important for controls and we should not block the suggestions because the user used a named param. This PR is changing the logic and treats the named params as of type `param` allowing the suggestions to follow. ### 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 - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit bee60b3)

kibanamachine · 2025-06-05T06:06:15Z

💚 All backports created successfully

Status	Branch	Result
✅	8.19

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

Questions ?

Please refer to the Backport tool documentation

…as ?value (#222312) (#222733) # Backport This will backport the following commits from `main` to `8.19`: - [[ES|QL] Fixing the suggestions in WHERE after a variable such as ?value (#222312)](#222312)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

…ue (elastic#222312) ## Summary Closes elastic#221776 **Case 1: Suggestions after the system params** <img width="925" alt="image" src="https://github.com/user-attachments/assets/496f0b57-a834-49df-9904-374cd433e370" /> **Case 2: Suggestions after a random named param** <img width="1009" alt="image" src="https://github.com/user-attachments/assets/d5c82af5-7677-41ad-ac9b-d0888bcc4520" /> **Case 3: Suggestions after field named param** <img width="469" alt="image" src="https://github.com/user-attachments/assets/827268f2-8abd-404a-b41a-71b1431c71fd" /> **Note:** Fixes are being applied to eval too. ### Some more technical info The named params (such as ?value or ??field) were typed as unknown to our code. For are not offering suggestions for this type but named params are important for controls and we should not block the suggestions because the user used a named param. This PR is changing the logic and treats the named params as of type `param` allowing the suggestions to follow. ### 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 - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

stratoula added 2 commits June 3, 2025 11:00

[ES|QL][WIP] Fixing the suggestions in WHERE after named params

91e05ba

More fixes and slight change of implementation

ffc009f

stratoula commented Jun 3, 2025

View reviewed changes

stratoula added 2 commits June 4, 2025 08:17

Merge branch 'main' into fix-where-named-param

179b782

Fixes where named params unit tests

db085bc

stratoula changed the title ~~[ES|QL][WIP] Fixing the suggestions in WHERE after named params~~ Jun 4, 2025

stratoula added 2 commits June 4, 2025 08:47

Some code cleanup

bcd0ef5

Further code optimization

4b3070a

stratoula commented Jun 4, 2025

View reviewed changes

Nit

c983b3c

stratoula added Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana t// v9.1.0 v8.19.0 backport:version Backport to applied version labels release_note:fix labels Jun 4, 2025

stratoula changed the title ~~[ES|QL] Fixing the suggestions in WHERE after named params~~ Jun 4, 2025

stratoula marked this pull request as ready for review June 4, 2025 09:20

stratoula requested a review from a team as a code owner June 4, 2025 09:20

elastic deleted a comment from elasticmachine Jun 4, 2025

stratoula requested a review from drewdaemon June 4, 2025 15:01

drewdaemon approved these changes Jun 4, 2025

View reviewed changes

stratoula merged commit bee60b3 into elastic:main Jun 5, 2025
22 checks passed

kibanamachine mentioned this pull request Jun 5, 2025

[8.19] [ES|QL] Fixing the suggestions in WHERE after a variable such as ?value (#222312) #222733

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Fixing the suggestions in WHERE after a variable such as ?value#222312

[ES|QL] Fixing the suggestions in WHERE after a variable such as ?value#222312
stratoula merged 7 commits intoelastic:mainfrom
stratoula:fix-where-named-param

stratoula commented Jun 3, 2025 •

edited by kibanamachine

Loading

stratoula Jun 3, 2025 •

edited

Loading

stratoula Jun 4, 2025

elasticmachine commented Jun 4, 2025

drewdaemon left a comment •

edited

Loading

drewdaemon Jun 4, 2025 •

edited

Loading

stratoula Jun 5, 2025

stratoula commented Jun 5, 2025

Uh oh!

kibanamachine commented Jun 5, 2025

kibanamachine commented Jun 5, 2025

Labels

4 participants

Conversation

stratoula commented Jun 3, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Some more technical info

Checklist

stratoula Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

stratoula Jun 4, 2025

Choose a reason for hiding this comment

elasticmachine commented Jun 4, 2025

drewdaemon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

drewdaemon Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

stratoula Jun 5, 2025

Choose a reason for hiding this comment

stratoula commented Jun 5, 2025

Uh oh!

kibanamachine commented Jun 5, 2025

kibanamachine commented Jun 5, 2025

💚 All backports created successfully

Questions ?

Labels

4 participants

stratoula commented Jun 3, 2025 •

edited by kibanamachine

Loading

stratoula Jun 3, 2025 •

edited

Loading

drewdaemon left a comment •

edited

Loading

drewdaemon Jun 4, 2025 •

edited

Loading