Skip to content

Commit

Permalink
[8.x] [ES|QL] separate `STATS` autocomplete routine (#198224)…
Browse files Browse the repository at this point in the history
… (#198693)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] separate `STATS` autocomplete routine
(#198224)](#198224)

<!--- 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-11-01T15:11:05Z","message":"[ES|QL]
separate `STATS` autocomplete routine (#198224)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/195418\r\n\r\nThis PR moves the
`STATS` completion logic to its own home.\r\n\r\nThere are also a few
changes in behavior. I am open for feedback on any\r\nof these.\r\n- the
cursor is automatically advanced after accepting a
comma\r\nsuggestion\r\n- variables from previous `EVAL` commands are no
longer suggested (e.g.\r\n`...| EVAL foo = 1 | STATS /`). I'm not sure
about this change, but it\r\nseemed potentially unintended to suggest
variables but no other columns\r\nsuch as field names.\r\n- a new
variable is suggested for new expressions in the `BY`
clause.\r\nFormerly, new variables were only suggested in the `STATS`
clause.\r\n- `+` and `-` are no longer suggested after a completed
function call\r\nwithin an assignment in the `BY` clause (e.g. `... |
STATS ... BY var1 =\r\nBUCKET(dateField, 1 day) /`. This behavior was
encoded in our tests, but\r\nit feels unintended to me, especially since
it only applied when the\r\nresult of the function was assigned to a new
variable in the `BY`\r\nclause.\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: Stratoula Kalafateli
<[email protected]>","sha":"7f2b56f0e687d466c20127d358d25a0456e51a03","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Feature:ES|QL","Team:ESQL","backport:version","v8.17.0"],"title":"[ES|QL]
separate `STATS` autocomplete
routine","number":198224,"url":"https://github.com/elastic/kibana/pull/198224","mergeCommit":{"message":"[ES|QL]
separate `STATS` autocomplete routine (#198224)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/195418\r\n\r\nThis PR moves the
`STATS` completion logic to its own home.\r\n\r\nThere are also a few
changes in behavior. I am open for feedback on any\r\nof these.\r\n- the
cursor is automatically advanced after accepting a
comma\r\nsuggestion\r\n- variables from previous `EVAL` commands are no
longer suggested (e.g.\r\n`...| EVAL foo = 1 | STATS /`). I'm not sure
about this change, but it\r\nseemed potentially unintended to suggest
variables but no other columns\r\nsuch as field names.\r\n- a new
variable is suggested for new expressions in the `BY`
clause.\r\nFormerly, new variables were only suggested in the `STATS`
clause.\r\n- `+` and `-` are no longer suggested after a completed
function call\r\nwithin an assignment in the `BY` clause (e.g. `... |
STATS ... BY var1 =\r\nBUCKET(dateField, 1 day) /`. This behavior was
encoded in our tests, but\r\nit feels unintended to me, especially since
it only applied when the\r\nresult of the function was assigned to a new
variable in the `BY`\r\nclause.\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: Stratoula Kalafateli
<[email protected]>","sha":"7f2b56f0e687d466c20127d358d25a0456e51a03"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198224","number":198224,"mergeCommit":{"message":"[ES|QL]
separate `STATS` autocomplete routine (#198224)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/195418\r\n\r\nThis PR moves the
`STATS` completion logic to its own home.\r\n\r\nThere are also a few
changes in behavior. I am open for feedback on any\r\nof these.\r\n- the
cursor is automatically advanced after accepting a
comma\r\nsuggestion\r\n- variables from previous `EVAL` commands are no
longer suggested (e.g.\r\n`...| EVAL foo = 1 | STATS /`). I'm not sure
about this change, but it\r\nseemed potentially unintended to suggest
variables but no other columns\r\nsuch as field names.\r\n- a new
variable is suggested for new expressions in the `BY`
clause.\r\nFormerly, new variables were only suggested in the `STATS`
clause.\r\n- `+` and `-` are no longer suggested after a completed
function call\r\nwithin an assignment in the `BY` clause (e.g. `... |
STATS ... BY var1 =\r\nBUCKET(dateField, 1 day) /`. This behavior was
encoded in our tests, but\r\nit feels unintended to me, especially since
it only applied when the\r\nresult of the function was assigned to a new
variable in the `BY`\r\nclause.\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: Stratoula Kalafateli
<[email protected]>","sha":"7f2b56f0e687d466c20127d358d25a0456e51a03"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Drew Tate <[email protected]>
  • Loading branch information
kibanamachine and drewdaemon authored Nov 1, 2024
1 parent 6ddfb80 commit 07e56b9
Show file tree
Hide file tree
Showing 14 changed files with 334 additions and 204 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@

import { FieldType, FunctionReturnType } from '../../definitions/types';
import { ESQL_COMMON_NUMERIC_TYPES, ESQL_NUMBER_TYPES } from '../../shared/esql_types';
import { getDateHistogramCompletionItem } from '../commands/stats/util';
import { allStarConstant } from '../complete_items';
import { getAddDateHistogramSnippet } from '../factories';
import { roundParameterTypes } from './constants';
import {
setup,
Expand Down Expand Up @@ -71,7 +71,7 @@ describe('autocomplete.suggest', () => {
test('on space after aggregate field', async () => {
const { assertSuggestions } = await setup();

await assertSuggestions('from a | stats a=min(b) /', ['BY $0', ',', '| ']);
await assertSuggestions('from a | stats a=min(b) /', ['BY ', ', ', '| ']);
});

test('on space after aggregate field with comma', async () => {
Expand Down Expand Up @@ -184,17 +184,16 @@ describe('autocomplete.suggest', () => {
test('when typing right paren', async () => {
const { assertSuggestions } = await setup();

await assertSuggestions('from a | stats a = min(b)/ | sort b', ['BY $0', ',', '| ']);
await assertSuggestions('from a | stats a = min(b)/ | sort b', ['BY ', ', ', '| ']);
});

test('increments suggested variable name counter', async () => {
const { assertSuggestions } = await setup();

await assertSuggestions('from a | eval var0=round(b), var1=round(c) | stats /', [
'var2 = ',
// TODO verify that this change is ok
...allAggFunctions,
'var0',
'var1',
...allEvaFunctions,
]);
await assertSuggestions('from a | stats var0=min(b),var1=c,/', [
Expand All @@ -210,7 +209,7 @@ describe('autocomplete.suggest', () => {
const { assertSuggestions } = await setup();
const expected = [
'var0 = ',
getAddDateHistogramSnippet(),
getDateHistogramCompletionItem(),
...getFieldNamesByType('any').map((field) => `${field} `),
...allEvaFunctions,
...allGroupingFunctions,
Expand All @@ -224,7 +223,7 @@ describe('autocomplete.suggest', () => {
test('on space after grouping field', async () => {
const { assertSuggestions } = await setup();

await assertSuggestions('from a | stats a=c by d /', [',', '| ']);
await assertSuggestions('from a | stats a=c by d /', [', ', '| ']);
});

test('after comma "," in grouping fields', async () => {
Expand All @@ -233,7 +232,7 @@ describe('autocomplete.suggest', () => {
const fields = getFieldNamesByType('any').map((field) => `${field} `);
await assertSuggestions('from a | stats a=c by d, /', [
'var0 = ',
getAddDateHistogramSnippet(),
getDateHistogramCompletionItem(),
...fields,
...allEvaFunctions,
...allGroupingFunctions,
Expand All @@ -245,7 +244,7 @@ describe('autocomplete.suggest', () => {
]);
await assertSuggestions('from a | stats avg(b) by c, /', [
'var0 = ',
getAddDateHistogramSnippet(),
getDateHistogramCompletionItem(),
...fields,
...getFunctionSignaturesByReturnType('eval', 'any', { scalar: true }),
...allGroupingFunctions,
Expand All @@ -262,17 +261,16 @@ describe('autocomplete.suggest', () => {
...getFunctionSignaturesByReturnType('eval', ['integer', 'double', 'long'], {
scalar: true,
}),

...allGroupingFunctions,
]);
await assertSuggestions('from a | stats avg(b) by var0 = /', [
getAddDateHistogramSnippet(),
getDateHistogramCompletionItem(),
...getFieldNamesByType('any').map((field) => `${field} `),
...allEvaFunctions,
...allGroupingFunctions,
]);
await assertSuggestions('from a | stats avg(b) by c, var0 = /', [
getAddDateHistogramSnippet(),
getDateHistogramCompletionItem(),
...getFieldNamesByType('any').map((field) => `${field} `),
...allEvaFunctions,
...allGroupingFunctions,
Expand All @@ -282,21 +280,17 @@ describe('autocomplete.suggest', () => {
test('on space after expression right hand side operand', async () => {
const { assertSuggestions } = await setup();

await assertSuggestions('from a | stats avg(b) by doubleField % 2 /', [',', '| ']);
await assertSuggestions('from a | stats avg(b) by doubleField % 2 /', [',', '| '], {
await assertSuggestions('from a | stats avg(b) by doubleField % 2 /', [', ', '| '], {
triggerCharacter: ' ',
});

await assertSuggestions(
'from a | stats var0 = AVG(doubleField) BY var1 = BUCKET(dateField, 1 day)/',
[',', '| ', '+ $0', '- $0']
);
await assertSuggestions(
'from a | stats var0 = AVG(doubleField) BY var1 = BUCKET(dateField, 1 day) /',
[',', '| ', '+ $0', '- $0'],
[', ', '| '],
{ triggerCharacter: ' ' }
);
});

test('on space within bucket()', async () => {
const { assertSuggestions } = await setup();
await assertSuggestions('from a | stats avg(b) by BUCKET(/, 50, ?_tstart, ?_tend)', [
Expand Down Expand Up @@ -330,6 +324,29 @@ describe('autocomplete.suggest', () => {
const suggestions = await suggest('from a | stats count(/)');
expect(suggestions).toContain(allStarConstant);
});

describe('date histogram snippet', () => {
test('uses histogramBarTarget preference when available', async () => {
const { suggest } = await setup();
const histogramBarTarget = Math.random() * 100;
const expectedCompletionItem = getDateHistogramCompletionItem(histogramBarTarget);

const suggestions = await suggest('FROM a | STATS BY /', {
callbacks: { getPreferences: () => Promise.resolve({ histogramBarTarget }) },
});

expect(suggestions).toContainEqual(expectedCompletionItem);
});

test('defaults gracefully', async () => {
const { suggest } = await setup();
const expectedCompletionItem = getDateHistogramCompletionItem();

const suggestions = await suggest('FROM a | STATS BY /');

expect(suggestions).toContainEqual(expectedCompletionItem);
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export function getFunctionSignaturesByReturnType(
({ returnType }) =>
expectedReturnType.includes('any') || expectedReturnType.includes(returnType as string)
);
if (!filteredByReturnType.length) {
if (!filteredByReturnType.length && !expectedReturnType.includes('any')) {
return false;
}
if (paramsTypes?.length) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { scalarFunctionDefinitions } from '../definitions/generated/scalar_funct
import { timeUnitsToSuggest } from '../definitions/literals';
import { commandDefinitions as unmodifiedCommandDefinitions } from '../definitions/commands';
import {
getAddDateHistogramSnippet,
getDateLiterals,
getSafeInsertText,
TIME_SYSTEM_PARAMS,
Expand All @@ -38,6 +37,7 @@ import { METADATA_FIELDS } from '../shared/constants';
import { ESQL_COMMON_NUMERIC_TYPES, ESQL_STRING_TYPES } from '../shared/esql_types';
import { log10ParameterTypes, powParameterTypes } from './__tests__/constants';
import { getRecommendedQueries } from './recommended_queries/templates';
import { getDateHistogramCompletionItem } from './commands/stats/util';

const commandDefinitions = unmodifiedCommandDefinitions.filter(({ hidden }) => !hidden);

Expand Down Expand Up @@ -737,12 +737,12 @@ describe('autocomplete', () => {
]);

// STATS argument BY
testSuggestions('FROM index1 | STATS AVG(booleanField) B/', ['BY $0', ',', '| ']);
testSuggestions('FROM index1 | STATS AVG(booleanField) B/', ['BY ', ', ', '| ']);

// STATS argument BY expression
testSuggestions('FROM index1 | STATS field BY f/', [
'var0 = ',
getAddDateHistogramSnippet(),
getDateHistogramCompletionItem(),
...getFunctionSignaturesByReturnType('stats', 'any', { grouping: true, scalar: true }),
...getFieldNamesByType('any').map((field) => `${field} `),
]);
Expand Down Expand Up @@ -1072,8 +1072,8 @@ describe('autocomplete', () => {

// STATS argument BY
testSuggestions('FROM a | STATS AVG(numberField) /', [
',',
attachAsSnippet(attachTriggerCommand('BY $0')),
', ',
attachTriggerCommand('BY '),
attachTriggerCommand('| '),
]);

Expand All @@ -1090,7 +1090,7 @@ describe('autocomplete', () => {
'by'
);
testSuggestions('FROM a | STATS AVG(numberField) BY /', [
getAddDateHistogramSnippet(),
getDateHistogramCompletionItem(),
attachTriggerCommand('var0 = '),
...getFieldNamesByType('any')
.map((field) => `${field} `)
Expand All @@ -1100,7 +1100,7 @@ describe('autocomplete', () => {

// STATS argument BY assignment (checking field suggestions)
testSuggestions('FROM a | STATS AVG(numberField) BY var0 = /', [
getAddDateHistogramSnippet(),
getDateHistogramCompletionItem(),
...getFieldNamesByType('any')
.map((field) => `${field} `)
.map(attachTriggerCommand),
Expand Down
Loading

0 comments on commit 07e56b9

Please sign in to comment.