-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[ES|QL] Fixes the suggestion problem in where for multiline queries #213240
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
Changes from all commits
aa3572d
6729417
4608570
a46bc7e
a91227a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -608,7 +608,7 @@ export async function getSuggestionsToRightOfOperatorExpression({ | |
| ) { | ||
| suggestions.push(listCompleteItem); | ||
| } else { | ||
| const finalType = leftArgType || leftArgType || 'any'; | ||
| const finalType = leftArgType || 'any'; | ||
| const supportedTypes = getSupportedTypesForBinaryOperators(fnDef, finalType as string); | ||
|
|
||
| // this is a special case with AND/OR | ||
|
|
@@ -671,12 +671,11 @@ export async function getSuggestionsToRightOfOperatorExpression({ | |
| } | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont think that this is needed unless I am missing something
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, you basically moved this shift to the translation step?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No I removed this offset gaining one position closet to the real position (it was by 2 positions wrong due to this) |
||
| return { | ||
| ...s, | ||
| rangeToReplace: { | ||
| start: overlap.start + offset, | ||
| end: overlap.end + offset, | ||
| start: overlap.start, | ||
| end: overlap.end, | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a test because it took me some time to understand how this function works |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
| import { offsetRangeToMonacoRange } from './utils'; | ||
|
|
||
| describe('offsetRangeToMonacoRange', () => { | ||
| test('should convert offset range to monaco range when the cursor is not at the end', () => { | ||
| const expression = 'FROM test | WHERE test == | LIMIT 1'; | ||
| const range = { start: 26, end: 26 }; | ||
| const monacoRange = offsetRangeToMonacoRange(expression, range); | ||
|
|
||
| expect(monacoRange).toEqual({ | ||
| startColumn: 26, | ||
| endColumn: 26, | ||
| startLineNumber: 1, | ||
| endLineNumber: 1, | ||
| }); | ||
| }); | ||
|
|
||
| test('should convert offset range to monaco range when the cursor is at the end', () => { | ||
| const expression = 'FROM test | WHERE test == 1 | LIMIT 1'; | ||
| const range = { start: 37, end: 37 }; | ||
| const monacoRange = offsetRangeToMonacoRange(expression, range); | ||
|
|
||
| expect(monacoRange).toEqual({ | ||
| startColumn: 37, | ||
| endColumn: 37, | ||
| startLineNumber: 1, | ||
| endLineNumber: 1, | ||
| }); | ||
| }); | ||
|
|
||
| test('should convert offset range to monaco range for multiple lines query when the cursor is not at the end', () => { | ||
| const expression = 'FROM test \n| WHERE test == \n| LIMIT 1'; | ||
| const range = { start: 27, end: 27 }; | ||
| const monacoRange = offsetRangeToMonacoRange(expression, range); | ||
|
|
||
| expect(monacoRange).toEqual({ | ||
| startColumn: 16, | ||
| endColumn: 16, | ||
| startLineNumber: 2, | ||
| endLineNumber: 2, | ||
| }); | ||
| }); | ||
|
|
||
| test('should convert offset range to monaco range for multiple lines query when the cursor is at the end', () => { | ||
| const expression = 'FROM test \n| WHERE test == \n| LIMIT 1'; | ||
| const range = { start: 35, end: 35 }; | ||
| const monacoRange = offsetRangeToMonacoRange(expression, range); | ||
|
|
||
| expect(monacoRange).toEqual({ | ||
| startColumn: 7, | ||
| endColumn: 7, | ||
| startLineNumber: 3, | ||
| endLineNumber: 3, | ||
| }); | ||
| }); | ||
| }); |
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
|
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.
irrelevant but I saw it and decided to fix here