[ES|QL] Fixes the suggestion problem in where for multiline queries#213240
Merged
stratoula merged 5 commits intoelastic:mainfrom Mar 10, 2025
Merged
[ES|QL] Fixes the suggestion problem in where for multiline queries#213240stratoula merged 5 commits intoelastic:mainfrom
stratoula merged 5 commits intoelastic:mainfrom
Conversation
stratoula
commented
Mar 5, 2025
| ) { | ||
| suggestions.push(listCompleteItem); | ||
| } else { | ||
| const finalType = leftArgType || leftArgType || 'any'; |
Contributor
Author
There was a problem hiding this comment.
irrelevant but I saw it and decided to fix here
stratoula
commented
Mar 5, 2025
| } | ||
| return suggestions.map<SuggestionRawDefinition>((s) => { | ||
| const overlap = getOverlapRange(queryText, s.text); | ||
| const offset = overlap.start === overlap.end ? 1 : 0; |
Contributor
Author
There was a problem hiding this comment.
I dont think that this is needed unless I am missing something
Contributor
There was a problem hiding this comment.
So, you basically moved this shift to the translation step?
Contributor
Author
There was a problem hiding this comment.
No I removed this offset gaining one position closet to the real position (it was by 2 positions wrong due to this)
stratoula
commented
Mar 5, 2025
Contributor
Author
There was a problem hiding this comment.
I added a test because it took me some time to understand how this function works
stratoula
commented
Mar 5, 2025
Contributor
Author
There was a problem hiding this comment.
The problem here is that the range.start is 2 positions after the /n and this creates the problem. So if the /n is at position 10, the start comes at 12 which causes the bug and is wrong as the range start should be either 1 position or at the same with the newline character.
- By removing this
const offset = overlap.start === overlap.end ? 1 : 0;
the difference becomes 1 position
-
By moving the line counter in the end we ensure that the line won't increase before we end the loop if we find the position
-
I am introducing an offset here to be sure that we catch the position correctly for multilines (as I explain above the position difference is 1 here for multilines
stratoula
added
v9.1.0
v8.19.0
backport:version
Contributor
|
Pinging @elastic/kibana-esql (Team:ESQL) |
This was referenced Mar 6, 2025
drewdaemon
reviewed
Mar 6, 2025
Contributor
drewdaemon
left a comment
drewdaemon
left a comment
There was a problem hiding this comment.
I've confirmed it fixes the problem. This whole issue makes my head hurt. I left some comments, including one about the tests. Hopefully they make sense...
| } | ||
| return suggestions.map<SuggestionRawDefinition>((s) => { | ||
| const overlap = getOverlapRange(queryText, s.text); | ||
| const offset = overlap.start === overlap.end ? 1 : 0; |
Contributor
There was a problem hiding this comment.
So, you basically moved this shift to the translation step?
src/platform/packages/shared/kbn-monaco/src/languages/esql/lib/shared/utils.test.ts
Outdated
Show resolved
Hide resolved
src/platform/packages/shared/kbn-monaco/src/languages/esql/lib/shared/utils.ts
Show resolved
Hide resolved
drewdaemon
approved these changes
Mar 7, 2025
Contributor
drewdaemon
left a comment
drewdaemon
left a comment
There was a problem hiding this comment.
Thank you for this fix!
Contributor
💚 Build Succeeded
Metrics [docs]Page load bundle
History
|
eokoneyo
approved these changes
Mar 10, 2025
Contributor
eokoneyo
left a comment
eokoneyo
left a comment
There was a problem hiding this comment.
Tested locally, suggestions are presented as expected
Contributor
|
Starting backport for target branches: 8.x |
Contributor
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
Contributor
Author
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
stratoula
added a commit
to stratoula/kibana
that referenced
this pull request
Mar 10, 2025
…lastic#213240) ## Summary Closes elastic#213323 This is the attempt to fix this bug: ``` FROM kibana_sample_data_logs | WHERE event.dataset == # cursor on this line | LIMIT 10 ``` In main the suggestions do not trigger. The problem is that the range is completely wrong. The lineNumber is 3 while it should be 2 and the start and end columns are also wrong. This PR attempts to fix it (hopefully).  ### 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 (cherry picked from commit 6f83177) # Conflicts: # src/platform/packages/shared/kbn-monaco/src/esql/lib/shared/utils.test.ts
stratoula
added a commit
that referenced
this pull request
Mar 10, 2025
…ries (#213240) (#213744) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Fixes the suggestion problem in where for multiline queries (#213240)](#213240) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Stratoula Kalafateli","email":"efstratia.kalafateli@elastic.co"},"sourceCommit":{"committedDate":"2025-03-10T12:02:21Z","message":"[ES|QL] Fixes the suggestion problem in where for multiline queries (#213240)\n\n## Summary\n\nCloses https://github.com/elastic/kibana/issues/213323\n\nThis is the attempt to fix this bug:\n\n```\nFROM kibana_sample_data_logs\n| WHERE event.dataset == # cursor on this line\n| LIMIT 10\n```\n\nIn main the suggestions do not trigger. The problem is that the range is\ncompletely wrong. The lineNumber is 3 while it should be 2 and the start\nand end columns are also wrong.\n\n\nThis PR attempts to fix it (hopefully).\n\n\n\n\n### Checklist\n\n- [ ] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios","sha":"6f831770fd77456e0d5f0c4eded122709481c043","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Feature:ES|QL","Team:ESQL","backport:version","v9.1.0","v8.19.0"],"title":"[ES|QL] Fixes the suggestion problem in where for multiline queries","number":213240,"url":"https://github.com/elastic/kibana/pull/213240","mergeCommit":{"message":"[ES|QL] Fixes the suggestion problem in where for multiline queries (#213240)\n\n## Summary\n\nCloses https://github.com/elastic/kibana/issues/213323\n\nThis is the attempt to fix this bug:\n\n```\nFROM kibana_sample_data_logs\n| WHERE event.dataset == # cursor on this line\n| LIMIT 10\n```\n\nIn main the suggestions do not trigger. The problem is that the range is\ncompletely wrong. The lineNumber is 3 while it should be 2 and the start\nand end columns are also wrong.\n\n\nThis PR attempts to fix it (hopefully).\n\n\n\n\n### Checklist\n\n- [ ] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios","sha":"6f831770fd77456e0d5f0c4eded122709481c043"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/213240","number":213240,"mergeCommit":{"message":"[ES|QL] Fixes the suggestion problem in where for multiline queries (#213240)\n\n## Summary\n\nCloses https://github.com/elastic/kibana/issues/213323\n\nThis is the attempt to fix this bug:\n\n```\nFROM kibana_sample_data_logs\n| WHERE event.dataset == # cursor on this line\n| LIMIT 10\n```\n\nIn main the suggestions do not trigger. The problem is that the range is\ncompletely wrong. The lineNumber is 3 while it should be 2 and the start\nand end columns are also wrong.\n\n\nThis PR attempts to fix it (hopefully).\n\n\n\n\n### Checklist\n\n- [ ] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios","sha":"6f831770fd77456e0d5f0c4eded122709481c043"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Mar 22, 2025
…lastic#213240) ## Summary Closes elastic#213323 This is the attempt to fix this bug: ``` FROM kibana_sample_data_logs | WHERE event.dataset == # cursor on this line | LIMIT 10 ``` In main the suggestions do not trigger. The problem is that the range is completely wrong. The lineNumber is 3 while it should be 2 and the start and end columns are also wrong. This PR attempts to fix it (hopefully).  ### 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #213323
This is the attempt to fix this bug:
In main the suggestions do not trigger. The problem is that the range is completely wrong. The lineNumber is 3 while it should be 2 and the start and end columns are also wrong.
This PR attempts to fix it (hopefully).
Checklist