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.

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

[ES|QL] Batch of fixes and new features for autocomplete and validation #175986

Merged
merged 37 commits into from
Feb 1, 2024

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Jan 31, 2024

Summary

This PR is a batch of fixes and (tiny) new features for validation and autocomplete items in ES|QL client side area.
Content of the PR:

  • 🐛 remove array/multi-value definition annotation for mv_* functions - fix [ES|QL] Use non array types for multi-value functions #174709
    Screenshot 2024-01-31 at 13 43 37
    Screenshot 2024-01-31 at 13 43 25

    • while the type check has been relaxed for lists, the actual non-multivalued argument type is retained and used for validation
      Screenshot 2024-01-31 at 13 44 06
  • 🐛 Remove array/multi-value definition annotation for mv_expand command - fix [ES|QL] Use non array types for multi-value functions #174709

    Screenshot 2024-01-31 at 14 36 19 Screenshot 2024-01-31 at 14 36 13
  • ✨ add metadata fields validation + autocomplete (fields are retrieved via Kibana uiSettings) ( part of [ES|QL] New grammar follow up tasks #172646 )

    metadata_fix

    Screenshot 2024-01-31 at 13 45 43 * ✨ Quick fixes now available also for metadata fields

    metadata_fixes

  • 🐛 fixed autocomplete for NOT operation

    Screenshot 2024-01-31 at 13 46 43
  • fixed autocomplete for in operation, with or without not:

    • ✨ now a missing list bracket is correctly detected and suggested after in
    • ✨ the content for the list is suggested and takes into account both the left side of the in/not in operator, but also the current content of the list

in_list_fix_2
in_list_fix
not_in_list_fix

  • fixed grok and dissect in various ways
    *:sparkles: the pattern provided by autocomplete for both was not valid in dissect nor useful at all, so I've changed to something more useful like '"%{WORD:firstWord}"' for grok and '"%{firstWord}"' for dissect to match the first word in the text field.

    • 🐛 there was a bug in the validation engine as both grok and dissect could generate new columns based on matches, so now a new field query is fired (only when either a GROK or a DISSECT command is detected) which enriches the set of fields available (similar to the enrich fields)
    • ✅ Added tons of tests here

    grok_fix
    dissect_fix

  • 🐛 fixed an issue with proposing an assign (=) operator when it should not

    • ... EVAL var0 = round( ...) <here> not within another assignment
    • ... EVAL var0 = 1 <here> not within another assignment
    • ... EVAL var0 = 1 year + 2 <here> not within another assignment
    • ... | WHERE field should not shadow a field name in this case
  • 🐛 fix an annoying auto trigger suggestion on field selection, and kept it only for functions
    auto_trigger_fix

  • 🐛 fixed an error in console with autocomplete when a typed function does not exists

  • ✅ Add tests for the hover feature

  • 🔥 Removed some unused functions detected via code coverage analysis

Checklist

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:ES|QL ES|QL related features in Kibana v8.13.0 labels Jan 31, 2024
@dej611
Copy link
Contributor Author

dej611 commented Jan 31, 2024

/ci

@dej611
Copy link
Contributor Author

dej611 commented Feb 1, 2024

/ci

@dej611 dej611 marked this pull request as ready for review February 1, 2024 10:49
@dej611 dej611 requested a review from a team as a code owner February 1, 2024 10:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@stratoula
Copy link
Contributor

/ci

@dej611
Copy link
Contributor Author

dej611 commented Feb 1, 2024

/ci

@dej611
Copy link
Contributor Author

dej611 commented Feb 1, 2024

/ci

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love all the fixes, I am just not so sure about the metadata validation and autocomplete.

@@ -341,6 +342,7 @@ export const TextBasedLanguagesEditor = memo(function TextBasedLanguagesEditor({
}
return [];
},
getMetaFields: async () => uiSettings?.get('metaFields') || [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we get them from the advanced setting? I think is better to hardcode them. Check below. I have removed the _id but if I use it in my query it doesn't fail. So we should not error out or not suggest it imho

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who decides what's a metafield or not?
Right now this information can be configured by Users/Admins into Kibana Settings, and we're using them already.
Let me ask the ES team where are they sourcing metadata fields.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your point but I dont think that is correct to error out a query which is not wrong and is going to return results. I am pretty sure that there is a list we can use without retrieving this information from the ui setting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found in the ES source code a hardcoded list of metadata fields:

  • _version
  • _index
  • _source
  • _id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline with the ES ES|QL team and they suggested to adopt their own hardcoded list of fields.
Changed here 6faf762

@dej611
Copy link
Contributor Author

dej611 commented Feb 1, 2024

/ci

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice enhancements Marco! LGTM! 💃

@dej611 dej611 enabled auto-merge (squash) February 1, 2024 14:37
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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/monaco 104 105 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
textBasedLanguages 154.1KB 154.2KB +61.0B

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 2.9MB 2.9MB +3.0KB
Unknown metric groups

API count

id before after diff
@kbn/monaco 104 105 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mistic mistic disabled auto-merge February 1, 2024 16:18
@mistic mistic merged commit af9951f into elastic:main Feb 1, 2024
18 of 19 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 1, 2024
fkanout pushed a commit to fkanout/kibana that referenced this pull request Feb 7, 2024
…on (elastic#175986)

## Summary

This PR is a batch of fixes and (tiny) new features for validation and
autocomplete items in ES|QL client side area.
Content of the PR:
* 🐛 remove array/multi-value definition annotation for `mv_*`
functions - fix elastic#174709
<img width="305" alt="Screenshot 2024-01-31 at 13 43 37"
src="https://github.com/elastic/kibana/assets/924948/2a458e74-9c87-4a67-8b42-0071e7a04f48">
<img width="287" alt="Screenshot 2024-01-31 at 13 43 25"
src="https://github.com/elastic/kibana/assets/924948/7205d984-d0c9-410b-8b39-c1d530c3f54f">
* while the type check has been relaxed for lists, the actual
non-multivalued argument type is retained and used for validation
<img width="758" alt="Screenshot 2024-01-31 at 13 44 06"
src="https://github.com/elastic/kibana/assets/924948/384e9dfa-aaf8-4e0f-953e-a2d656776ea7">

* 🐛 Remove array/multi-value definition annotation for `mv_expand`
command - fix elastic#174709

<img width="556" alt="Screenshot 2024-01-31 at 14 36 19"
src="https://github.com/elastic/kibana/assets/924948/0debbccc-da9a-4e6a-bfc9-3fded61300b5">
<img width="277" alt="Screenshot 2024-01-31 at 14 36 13"
src="https://github.com/elastic/kibana/assets/924948/9438832b-d8d5-48c5-a030-b8983057546e">


* ✨ add metadata fields validation + autocomplete (fields are
retrieved via Kibana `uiSettings`) ( part of elastic#172646 )
  

![metadata_fix](https://github.com/elastic/kibana/assets/924948/958e6609-ab73-4949-8652-f368c6604a09)
  
<img width="760" alt="Screenshot 2024-01-31 at 13 45 43"
src="https://github.com/elastic/kibana/assets/924948/b3f13d99-3e12-4586-b39c-f4ea3691a2f1">
  * ✨ Quick fixes now available also for metadata fields
  

![metadata_fixes](https://github.com/elastic/kibana/assets/924948/b273418f-a754-4c50-9539-a128f35fc4cd)

* 🐛 fixed autocomplete for `NOT` operation

<img width="549" alt="Screenshot 2024-01-31 at 13 46 43"
src="https://github.com/elastic/kibana/assets/924948/29b7aa7c-db66-485a-a5b0-329e66df842f">

* fixed autocomplete for `in` operation, with or without `not`:
* ✨ now a missing list bracket is correctly detected and
suggested after `in`
* ✨ the content for the list is suggested and takes into
account both the left side of the `in`/`not in` operator, but also the
current content of the list


![in_list_fix_2](https://github.com/elastic/kibana/assets/924948/50ea8db5-23da-411c-b81e-5ba3397b65c1)

![in_list_fix](https://github.com/elastic/kibana/assets/924948/cbb73ad2-4073-4eb6-9c12-61835858fd5e)

![not_in_list_fix](https://github.com/elastic/kibana/assets/924948/9d7fa756-3d34-49fe-ac7a-28e2edbe7fee)

* fixed `grok` and `dissect` in various ways
*:sparkles: the pattern provided by autocomplete for both was not valid
in `dissect` nor useful at all, so I've changed to something more useful
like `'"%{WORD:firstWord}"'` for `grok` and `'"%{firstWord}"'` for
`dissect` to match the first word in the text field.
* :bug: there was a bug in the validation engine as both `grok` and
`dissect` could generate new columns based on matches, so now a new
field query is fired (only when either a `GROK` or a `DISSECT` command
is detected) which enriches the set of fields available (similar to the
enrich fields)
  * ✅ Added tons of tests here


![grok_fix](https://github.com/elastic/kibana/assets/924948/a23c8edc-0702-4531-aaa5-6126e375913b)

![dissect_fix](https://github.com/elastic/kibana/assets/924948/d5591753-775e-4768-be00-31c4371be330)

* 🐛 fixed an issue with proposing an assign (`=`) operator when it
should not
  * `... EVAL var0 = round( ...) <here>` not within another assignment
  * `... EVAL var0 = 1 <here>` not within another assignment
  * `... EVAL var0 = 1 year + 2 <here>` not within another assignment
  * `... | WHERE field` should not shadow a field name in this case

* 🐛 fix an annoying auto trigger suggestion on field selection, and
kept it only for functions

![auto_trigger_fix](https://github.com/elastic/kibana/assets/924948/31d0c296-6338-450d-8f81-e08cd1c7526d)

* 🐛 fixed an error in console with autocomplete when a typed
function does not exists
* ✅  Add tests for the hover feature
* 🔥 Removed some unused functions detected via code coverage
analysis


### 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: Stratoula Kalafateli <[email protected]>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
…on (elastic#175986)

## Summary

This PR is a batch of fixes and (tiny) new features for validation and
autocomplete items in ES|QL client side area.
Content of the PR:
* 🐛 remove array/multi-value definition annotation for `mv_*`
functions - fix elastic#174709
<img width="305" alt="Screenshot 2024-01-31 at 13 43 37"
src="https://github.com/elastic/kibana/assets/924948/2a458e74-9c87-4a67-8b42-0071e7a04f48">
<img width="287" alt="Screenshot 2024-01-31 at 13 43 25"
src="https://github.com/elastic/kibana/assets/924948/7205d984-d0c9-410b-8b39-c1d530c3f54f">
* while the type check has been relaxed for lists, the actual
non-multivalued argument type is retained and used for validation
<img width="758" alt="Screenshot 2024-01-31 at 13 44 06"
src="https://github.com/elastic/kibana/assets/924948/384e9dfa-aaf8-4e0f-953e-a2d656776ea7">

* 🐛 Remove array/multi-value definition annotation for `mv_expand`
command - fix elastic#174709

<img width="556" alt="Screenshot 2024-01-31 at 14 36 19"
src="https://github.com/elastic/kibana/assets/924948/0debbccc-da9a-4e6a-bfc9-3fded61300b5">
<img width="277" alt="Screenshot 2024-01-31 at 14 36 13"
src="https://github.com/elastic/kibana/assets/924948/9438832b-d8d5-48c5-a030-b8983057546e">


* ✨ add metadata fields validation + autocomplete (fields are
retrieved via Kibana `uiSettings`) ( part of elastic#172646 )
  

![metadata_fix](https://github.com/elastic/kibana/assets/924948/958e6609-ab73-4949-8652-f368c6604a09)
  
<img width="760" alt="Screenshot 2024-01-31 at 13 45 43"
src="https://github.com/elastic/kibana/assets/924948/b3f13d99-3e12-4586-b39c-f4ea3691a2f1">
  * ✨ Quick fixes now available also for metadata fields
  

![metadata_fixes](https://github.com/elastic/kibana/assets/924948/b273418f-a754-4c50-9539-a128f35fc4cd)

* 🐛 fixed autocomplete for `NOT` operation

<img width="549" alt="Screenshot 2024-01-31 at 13 46 43"
src="https://github.com/elastic/kibana/assets/924948/29b7aa7c-db66-485a-a5b0-329e66df842f">

* fixed autocomplete for `in` operation, with or without `not`:
* ✨ now a missing list bracket is correctly detected and
suggested after `in`
* ✨ the content for the list is suggested and takes into
account both the left side of the `in`/`not in` operator, but also the
current content of the list


![in_list_fix_2](https://github.com/elastic/kibana/assets/924948/50ea8db5-23da-411c-b81e-5ba3397b65c1)

![in_list_fix](https://github.com/elastic/kibana/assets/924948/cbb73ad2-4073-4eb6-9c12-61835858fd5e)

![not_in_list_fix](https://github.com/elastic/kibana/assets/924948/9d7fa756-3d34-49fe-ac7a-28e2edbe7fee)

* fixed `grok` and `dissect` in various ways
*:sparkles: the pattern provided by autocomplete for both was not valid
in `dissect` nor useful at all, so I've changed to something more useful
like `'"%{WORD:firstWord}"'` for `grok` and `'"%{firstWord}"'` for
`dissect` to match the first word in the text field.
* :bug: there was a bug in the validation engine as both `grok` and
`dissect` could generate new columns based on matches, so now a new
field query is fired (only when either a `GROK` or a `DISSECT` command
is detected) which enriches the set of fields available (similar to the
enrich fields)
  * ✅ Added tons of tests here


![grok_fix](https://github.com/elastic/kibana/assets/924948/a23c8edc-0702-4531-aaa5-6126e375913b)

![dissect_fix](https://github.com/elastic/kibana/assets/924948/d5591753-775e-4768-be00-31c4371be330)

* 🐛 fixed an issue with proposing an assign (`=`) operator when it
should not
  * `... EVAL var0 = round( ...) <here>` not within another assignment
  * `... EVAL var0 = 1 <here>` not within another assignment
  * `... EVAL var0 = 1 year + 2 <here>` not within another assignment
  * `... | WHERE field` should not shadow a field name in this case

* 🐛 fix an annoying auto trigger suggestion on field selection, and
kept it only for functions

![auto_trigger_fix](https://github.com/elastic/kibana/assets/924948/31d0c296-6338-450d-8f81-e08cd1c7526d)

* 🐛 fixed an error in console with autocomplete when a typed
function does not exists
* ✅  Add tests for the hover feature
* 🔥 Removed some unused functions detected via code coverage
analysis


### 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: Stratoula Kalafateli <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
…on (elastic#175986)

## Summary

This PR is a batch of fixes and (tiny) new features for validation and
autocomplete items in ES|QL client side area.
Content of the PR:
* 🐛 remove array/multi-value definition annotation for `mv_*`
functions - fix elastic#174709
<img width="305" alt="Screenshot 2024-01-31 at 13 43 37"
src="https://github.com/elastic/kibana/assets/924948/2a458e74-9c87-4a67-8b42-0071e7a04f48">
<img width="287" alt="Screenshot 2024-01-31 at 13 43 25"
src="https://github.com/elastic/kibana/assets/924948/7205d984-d0c9-410b-8b39-c1d530c3f54f">
* while the type check has been relaxed for lists, the actual
non-multivalued argument type is retained and used for validation
<img width="758" alt="Screenshot 2024-01-31 at 13 44 06"
src="https://github.com/elastic/kibana/assets/924948/384e9dfa-aaf8-4e0f-953e-a2d656776ea7">

* 🐛 Remove array/multi-value definition annotation for `mv_expand`
command - fix elastic#174709

<img width="556" alt="Screenshot 2024-01-31 at 14 36 19"
src="https://github.com/elastic/kibana/assets/924948/0debbccc-da9a-4e6a-bfc9-3fded61300b5">
<img width="277" alt="Screenshot 2024-01-31 at 14 36 13"
src="https://github.com/elastic/kibana/assets/924948/9438832b-d8d5-48c5-a030-b8983057546e">


* ✨ add metadata fields validation + autocomplete (fields are
retrieved via Kibana `uiSettings`) ( part of elastic#172646 )
  

![metadata_fix](https://github.com/elastic/kibana/assets/924948/958e6609-ab73-4949-8652-f368c6604a09)
  
<img width="760" alt="Screenshot 2024-01-31 at 13 45 43"
src="https://github.com/elastic/kibana/assets/924948/b3f13d99-3e12-4586-b39c-f4ea3691a2f1">
  * ✨ Quick fixes now available also for metadata fields
  

![metadata_fixes](https://github.com/elastic/kibana/assets/924948/b273418f-a754-4c50-9539-a128f35fc4cd)

* 🐛 fixed autocomplete for `NOT` operation

<img width="549" alt="Screenshot 2024-01-31 at 13 46 43"
src="https://github.com/elastic/kibana/assets/924948/29b7aa7c-db66-485a-a5b0-329e66df842f">

* fixed autocomplete for `in` operation, with or without `not`:
* ✨ now a missing list bracket is correctly detected and
suggested after `in`
* ✨ the content for the list is suggested and takes into
account both the left side of the `in`/`not in` operator, but also the
current content of the list


![in_list_fix_2](https://github.com/elastic/kibana/assets/924948/50ea8db5-23da-411c-b81e-5ba3397b65c1)

![in_list_fix](https://github.com/elastic/kibana/assets/924948/cbb73ad2-4073-4eb6-9c12-61835858fd5e)

![not_in_list_fix](https://github.com/elastic/kibana/assets/924948/9d7fa756-3d34-49fe-ac7a-28e2edbe7fee)

* fixed `grok` and `dissect` in various ways
*:sparkles: the pattern provided by autocomplete for both was not valid
in `dissect` nor useful at all, so I've changed to something more useful
like `'"%{WORD:firstWord}"'` for `grok` and `'"%{firstWord}"'` for
`dissect` to match the first word in the text field.
* :bug: there was a bug in the validation engine as both `grok` and
`dissect` could generate new columns based on matches, so now a new
field query is fired (only when either a `GROK` or a `DISSECT` command
is detected) which enriches the set of fields available (similar to the
enrich fields)
  * ✅ Added tons of tests here


![grok_fix](https://github.com/elastic/kibana/assets/924948/a23c8edc-0702-4531-aaa5-6126e375913b)

![dissect_fix](https://github.com/elastic/kibana/assets/924948/d5591753-775e-4768-be00-31c4371be330)

* 🐛 fixed an issue with proposing an assign (`=`) operator when it
should not
  * `... EVAL var0 = round( ...) <here>` not within another assignment
  * `... EVAL var0 = 1 <here>` not within another assignment
  * `... EVAL var0 = 1 year + 2 <here>` not within another assignment
  * `... | WHERE field` should not shadow a field name in this case

* 🐛 fix an annoying auto trigger suggestion on field selection, and
kept it only for functions

![auto_trigger_fix](https://github.com/elastic/kibana/assets/924948/31d0c296-6338-450d-8f81-e08cd1c7526d)

* 🐛 fixed an error in console with autocomplete when a typed
function does not exists
* ✅  Add tests for the hover feature
* 🔥 Removed some unused functions detected via code coverage
analysis


### 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: Stratoula Kalafateli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:build-webpack-bundle-analyzer Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ES|QL] Use non array types for multi-value functions
6 participants