-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] open suggestions automatically in sources lists and ENRICH
#191312
[ES|QL] open suggestions automatically in sources lists and ENRICH
#191312
Conversation
/ci |
/ci |
ENRICH
Pinging @elastic/kibana-esql (Team:ESQL) |
@elasticmachine merge upstream |
@@ -845,6 +961,57 @@ describe('autocomplete', () => { | |||
'| ', | |||
]); | |||
|
|||
describe('ENRICH', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth splitting these new blocks out to new files 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you split it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I suggested this because there are two separate describe('enrich') blocks in this file. But one is to test the actual suggestions for enrich, and one is to test advancing the cursor and opening the suggestion menu automatically. We can discuss this in office hours how best to structure these autocomplete tests, but imo it might be worth moving the big blocks like advancing the cursor
into their own files in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. There's a tension between splitting things up by command or by language feature across commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what is best but, yes, worth a discussion!
* @returns | ||
*/ | ||
export function findFinalWord(text: string) { | ||
const words = text.split(/\s+/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the regex in this and in findPreviousWord
, is it worth trimming the text first? For example "foo bar "
would give ['', 'foo', 'bar', '']
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok I see now we do it expect it to be an empty space in some cases.
@@ -272,7 +273,7 @@ export const buildPoliciesDefinitions = ( | |||
): SuggestionRawDefinition[] => | |||
policies.map(({ name: label, sourceIndices }) => ({ | |||
label, | |||
text: getSafeInsertText(label, { dashSupported: true }), | |||
text: getSafeInsertText(label, { dashSupported: true }) + ' ', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to think we should make this ' '
a constant for clarity and easy searching. So something like OPEN_SUGGESTION = ' '
and then text + OPEN_SUGGESTION
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ' '
is not what opens the suggestion menu. That is the command
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant advance cursor. Switched it around in my brain. And this is just a thought, not a blocker.
// ok, this is a bit of cheating as it's using the same logic as in the helper | ||
enrichFields.map((field) => (/[^a-zA-Z\d_\.@]/.test(field) ? `\`${field}\`` : field)) | ||
); | ||
.flatMap(({ enrichFields }) => enrichFields.map((field) => getSafeInsertText(field))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can probably use enrichFields.map(getSafeInsertText)
here
@elasticmachine merge upstream |
LGTM 🎉 |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elasticmachine merge upstream |
I'm going to keep it out of draft mode since it will be unblocked when #192953 merges |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unblocking the PR now that the blocker got resolved. LGTM, I did a brief testing locally to ensure that it works as expected.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
…lastic#191312) ## Summary Part of elastic#189662 Also, a follow-on to elastic#187184 with certain source names (e.g. `foo$bar`, `my-policy`) and fields in the `ENRICH` command. During this effort I discovered elastic#191321 and a bug with the validation of field names in the "ENRICH ... WITH" list (documented [here](elastic#177699)), both of which will be addressed separately. ## Sources ### General flow https://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf ### Works with wild-card matches https://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c ### `METADATA` field list https://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a ## ENRICH Autosuggest now helps you along https://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f Also, fixed this bug (follow on to elastic#187184 ) https://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85 ### 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: Elastic Machine <[email protected]> Co-authored-by: Stratoula Kalafateli <[email protected]> (cherry picked from commit e404a39)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…60;ENRICH` (#191312) (#193013) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] open suggestions automatically in sources lists and `ENRICH` (#191312)](#191312) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Drew Tate","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-16T13:41:38Z","message":"[ES|QL] open suggestions automatically in sources lists and `ENRICH` (#191312)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/189662\r\n\r\nAlso, a follow-on to https://github.com/elastic/kibana/issues/187184\r\nwith certain source names (e.g. `foo$bar`, `my-policy`) and fields in\r\nthe `ENRICH` command.\r\n\r\nDuring this effort I discovered\r\nhttps://github.com//issues/191321 and a bug with the\r\nvalidation of field names in the \"ENRICH ... WITH\" list (documented\r\n[here](#177699)), both of which\r\nwill be addressed separately.\r\n\r\n## Sources\r\n\r\n### General flow\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf\r\n\r\n### Works with wild-card matches\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c\r\n\r\n### `METADATA` field list\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a\r\n\r\n## ENRICH\r\n\r\nAutosuggest now helps you along\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f\r\n\r\nAlso, fixed this bug (follow on to\r\nhttps://github.com//issues/187184 )\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>\r\nCo-authored-by: Stratoula Kalafateli <[email protected]>","sha":"e404a3992e220735ae51918c532a1a032e7f7993","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","v9.0.0","backport:prev-minor","Feature:ES|QL","Team:ESQL"],"title":"[ES|QL] open suggestions automatically in sources lists and `ENRICH`","number":191312,"url":"https://github.com/elastic/kibana/pull/191312","mergeCommit":{"message":"[ES|QL] open suggestions automatically in sources lists and `ENRICH` (#191312)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/189662\r\n\r\nAlso, a follow-on to https://github.com/elastic/kibana/issues/187184\r\nwith certain source names (e.g. `foo$bar`, `my-policy`) and fields in\r\nthe `ENRICH` command.\r\n\r\nDuring this effort I discovered\r\nhttps://github.com//issues/191321 and a bug with the\r\nvalidation of field names in the \"ENRICH ... WITH\" list (documented\r\n[here](#177699)), both of which\r\nwill be addressed separately.\r\n\r\n## Sources\r\n\r\n### General flow\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf\r\n\r\n### Works with wild-card matches\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c\r\n\r\n### `METADATA` field list\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a\r\n\r\n## ENRICH\r\n\r\nAutosuggest now helps you along\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f\r\n\r\nAlso, fixed this bug (follow on to\r\nhttps://github.com//issues/187184 )\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>\r\nCo-authored-by: Stratoula Kalafateli <[email protected]>","sha":"e404a3992e220735ae51918c532a1a032e7f7993"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/191312","number":191312,"mergeCommit":{"message":"[ES|QL] open suggestions automatically in sources lists and `ENRICH` (#191312)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/189662\r\n\r\nAlso, a follow-on to https://github.com/elastic/kibana/issues/187184\r\nwith certain source names (e.g. `foo$bar`, `my-policy`) and fields in\r\nthe `ENRICH` command.\r\n\r\nDuring this effort I discovered\r\nhttps://github.com//issues/191321 and a bug with the\r\nvalidation of field names in the \"ENRICH ... WITH\" list (documented\r\n[here](#177699)), both of which\r\nwill be addressed separately.\r\n\r\n## Sources\r\n\r\n### General flow\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf\r\n\r\n### Works with wild-card matches\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c\r\n\r\n### `METADATA` field list\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a\r\n\r\n## ENRICH\r\n\r\nAutosuggest now helps you along\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f\r\n\r\nAlso, fixed this bug (follow on to\r\nhttps://github.com//issues/187184 )\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <[email protected]>\r\nCo-authored-by: Stratoula Kalafateli <[email protected]>","sha":"e404a3992e220735ae51918c532a1a032e7f7993"}}]}] BACKPORT--> Co-authored-by: Drew Tate <[email protected]>
Summary
Part of #189662
Also, a follow-on to #187184 with certain source names (e.g.
foo$bar
,my-policy
) and fields in theENRICH
command.During this effort I discovered #191321 and a bug with the validation of field names in the "ENRICH ... WITH" list (documented here), both of which will be addressed separately.
Sources
General flow
Screen.Recording.2024-08-28.at.2.22.32.PM.mov
Works with wild-card matches
Screen.Recording.2024-08-26.at.2.38.55.PM.mov
METADATA
field listScreen.Recording.2024-08-26.at.4.06.40.PM.mov
ENRICH
Autosuggest now helps you along
Screen.Recording.2024-08-28.at.12.57.32.PM.mov
Also, fixed this bug (follow on to #187184 )
Screen.Recording.2024-08-28.at.12.36.51.PM.mov
Checklist