Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ES|QL] revisit field and variable representations #191111

Closed
Tracked by #176033
drewdaemon opened this issue Aug 22, 2024 · 2 comments · Fixed by #195149
Closed
Tracked by #176033

[ES|QL] revisit field and variable representations #191111

drewdaemon opened this issue Aug 22, 2024 · 2 comments · Fixed by #195149
Assignees
@drewdaemon
Labels
enhancement New value added to drive a business result Feature:ES|QL ES|QL related features in Kibana impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana

Comments

@drewdaemon
Copy link
Contributor

drewdaemon commented Aug 22, 2024 •
edited
Loading

Describe the feature:
Column names can be escaped and written in a variety of ways. For example, the following are all valid for event.dataset.

`event.dataset`
"event.dataset"
"""event.dataset"""
`event`.`dataset`

// low priority
`event` . `dataset`
event . dataset
event . /* comment */ dataset

Our support for these in validation and autocomplete is spotty. There are several known bugs that are likely related to this.

FROM kibana_sample_data_logs | KEEP `geo`.`dest`

There are also things we just can't support today

The ask

Take a comprehensive look at the situation. We have options

  • Is the looseness here necessary? Can we tighten the language grammar?
  • Would using the AST more in autocomplete help here?
  • How are we handling escaping in our cached field and variable lists? It seems like some of the bugs are due to cache misses (e.g. looking for the escaped version of a field when the cache contains the unescaped field name instead).

Some implementations
@drewdaemon drewdaemon added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana labels Aug 22, 2024
@elasticmachine
Copy link
Contributor

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

This was referenced Aug 22, 2024
Open
Merged
drewdaemon added a commit that referenced this issue Aug 23, 2024
@drewdaemon
[ES|QL] Auto open suggestions for field lists (#190466)
15ef37f 
## Summary

Part of #189662

## Suggests comma and pipe


https://github.com/user-attachments/assets/c09bc6fd-20a6-4f42-a871-c70e68e6d81a

## Doesn't suggest comma when there are no more fields


https://github.com/user-attachments/assets/29fce13e-e58b-4d93-bce5-6b1f913b4d92

## Doesn't work for escaped columns :(


https://github.com/user-attachments/assets/3d65f3b9-923d-4c0e-9c50-51dd83115c8b

As part of this effort I discovered
#191100 and
#191105, as well as a problem
with column name validation (see
#177699)

I think we can revisit column escaping and probably resolve all of these
issues (issue [here](#191111)).


### 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
@stratoula stratoula added the enhancement New value added to drive a business result label Aug 25, 2024
@drewdaemon
Copy link
Contributor Author

Relevant discussion: #182393 (comment)

This was referenced Sep 30, 2024
Open
Merged
drewdaemon added a commit that referenced this issue Oct 11, 2024
@drewdaemon @elasticmachine
[ES|QL] Improve variable and field name handling (#195149)
731c4e4 
## 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](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
#195682

User-facing improvements
- Validation used to fail for field names with multiple escaped parts
(e.g. `geo`.`dest`)
- #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]>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 11, 2024
@drewdaemon
[ES|QL] Improve variable and field name handling (elastic#195149)
5d473e2 
## 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@drewdaemon drewdaemon

Labels
enhancement New value added to drive a business result Feature:ES|QL ES|QL related features in Kibana impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:ESQL ES|QL related features in Kibana
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[ES|QL] Improve variable and field name handling drewdaemon/kibana
3 participants
@drewdaemon @elasticmachine @stratoula