Skip to content

Commit

Permalink
[ES|QL] WHERE replacement ranges correctly generated for every case (e…
Browse files Browse the repository at this point in the history
…lastic#209684)

## Summary

fix elastic#204441

It ain't beautiful but it works. I am going to come in with another pr
to deal with prefix detection holistically

### 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 <elasticmachine@users.noreply.github.com>
  • Loading branch information
drewdaemon and elasticmachine authored Feb 6, 2025
1 parent 16fae1c commit 4ee3b50
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -318,30 +318,57 @@ describe('WHERE <expression>', () => {
);
});

test('attaches ranges', async () => {
const { suggest } = await setup();
describe('attaches ranges', () => {
test('omits ranges if there is no prefix', async () => {
const { suggest } = await setup();

const suggestions = await suggest('FROM index | WHERE doubleField IS N/');
(await suggest('FROM index | WHERE /')).forEach((suggestion) => {
expect(suggestion.rangeToReplace).toBeUndefined();
});
});

expect(suggestions).toContainEqual(
expect.objectContaining({
text: 'IS NOT NULL',
rangeToReplace: {
start: 32,
end: 36,
},
})
);
test('uses indices of single prefix by default', async () => {
const { suggest } = await setup();

expect(suggestions).toContainEqual(
expect.objectContaining({
text: 'IS NULL',
rangeToReplace: {
start: 32,
end: 36,
},
})
);
(await suggest('FROM index | WHERE some.prefix/')).forEach((suggestion) => {
expect(suggestion.rangeToReplace).toEqual({
start: 20,
end: 30,
});
});
});

test('"IS (NOT) NULL" with a matching prefix', async () => {
const { suggest } = await setup();

const suggestions = await suggest('FROM index | WHERE doubleField IS N/');

expect(suggestions.find((s) => s.text === 'IS NOT NULL')?.rangeToReplace).toEqual({
start: 32,
end: 36,
});

expect(suggestions.find((s) => s.text === 'IS NULL')?.rangeToReplace).toEqual({
start: 32,
end: 36,
});
});

test('"IS (NOT) NULL" with a matching prefix with trailing space', async () => {
const { suggest } = await setup();

const suggestions = await suggest('FROM index | WHERE doubleField IS /');

expect(suggestions.find((s) => s.text === 'IS NOT NULL')?.rangeToReplace).toEqual({
start: 32,
end: 35,
});

expect(suggestions.find((s) => s.text === 'IS NULL')?.rangeToReplace).toEqual({
start: 32,
end: 35,
});
});
});

describe('create control suggestion', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1185,7 +1185,7 @@ describe('autocomplete', () => {
describe('Replacement ranges are attached when needed', () => {
testSuggestions('FROM a | WHERE doubleField IS NOT N/', [
{ text: 'IS NOT NULL', rangeToReplace: { start: 28, end: 36 } },
{ text: 'IS NULL', rangeToReplace: { start: 37, end: 37 } },
{ text: 'IS NULL', rangeToReplace: { start: 35, end: 35 } },
'!= $0',
'== $0',
'IN $0',
Expand All @@ -1200,7 +1200,7 @@ describe('autocomplete', () => {
testSuggestions('FROM a | WHERE doubleField IS N/', [
{ text: 'IS NOT NULL', rangeToReplace: { start: 28, end: 32 } },
{ text: 'IS NULL', rangeToReplace: { start: 28, end: 32 } },
{ text: '!= $0', rangeToReplace: { start: 33, end: 33 } },
{ text: '!= $0', rangeToReplace: { start: 31, end: 31 } },
'== $0',
'IN $0',
'AND $0',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ export async function suggest(
*/
const expressionRoot = command.args[0] as ESQLSingleAstItem | undefined;

switch (getPosition(innerText, command)) {
const position = getPosition(innerText, command);
switch (position) {
/**
* After a column name
*/
Expand Down Expand Up @@ -192,15 +193,38 @@ export async function suggest(
suggestions.push(pipeCompleteItem);
}

return suggestions.map<SuggestionRawDefinition>((s) => {
const overlap = getOverlapRange(innerText, s.text);
const offset = overlap.start === overlap.end ? 1 : 0;
return {
...s,
rangeToReplace: {
start: overlap.start + offset,
end: overlap.end + offset,
},
};
});
/**
* Attach replacement ranges if there's a prefix.
*
* Can't rely on Monaco because
* - it counts "." as a word separator
* - it doesn't handle multi-word completions (like "is null")
*
* TODO - think about how to generalize this.
*/
const hasNonWhitespacePrefix = !/\s/.test(innerText[innerText.length - 1]);
if (hasNonWhitespacePrefix) {
// get index of first char of final word
const lastWhitespaceIndex = innerText.search(/\S(?=\S*$)/);
suggestions.forEach((s) => {
if (['IS NULL', 'IS NOT NULL'].includes(s.text)) {
// this suggestion has spaces in it (e.g. "IS NOT NULL")
// so we need to see if there's an overlap
const overlap = getOverlapRange(innerText, s.text);
if (overlap.start < overlap.end) {
// there's an overlap so use that
s.rangeToReplace = overlap;
return;
}
}

// no overlap, so just replace from the last whitespace
s.rangeToReplace = {
start: lastWhitespaceIndex + 1,
end: innerText.length,
};
});
}

return suggestions;
}

0 comments on commit 4ee3b50

Please sign in to comment.