Skip to content

Commit

Permalink
[8.x] [ES|QL] [Discover] Displays the histogram suggestion always for…
Browse files Browse the repository at this point in the history
… non transformational commands (#195863) (#196074)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] [Discover] Displays the histogram suggestion always for non
transformational commands
(#195863)](#195863)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Stratoula
Kalafateli","email":"efstratia.kalafateli@elastic.co"},"sourceCommit":{"committedDate":"2024-10-14T09:52:16Z","message":"[ES|QL]
[Discover] Displays the histogram suggestion always for non
transformational commands (#195863)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/195752\r\n\r\nThis PR is fixing
2 bugs:\r\n\r\n- It filters out counter fields from the breakdown as
they are not\r\nsupported. I created a new util for this\r\n- Fixes a
bug unrelated with the breakdown (it also exists in previous\r\nminors).
The LensVis service is computing suggestions and pushes them
to\r\n`availableSuggestionsWithType `. In some indexes (it depends on
the\r\ntypes of the first 5 columns of the index) the lens suggestions
api\r\nmight return a suggestion. So in that case the array has the
histogram\r\nsuggestion + the suggestion from the suggestions api. So
the service\r\nwill pick the first one which is not the histogram. But
we know that in\r\ncase of non transformational commands we want to
suggest the histogram.\r\nSo this PR is fixing it by ensuring that the
array is cleaned up before\r\npushing the histogram
suggestion.\r\n\r\n\r\nNote: The 2 bugs are unrelated I just decided to
fix them in one PR as\r\nthey are both histogram bugs.\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","sha":"f962cdcd796af9908449155c989dd03438165773","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:skip","v9.0.0","Team:DataDiscovery","backport:prev-minor","Feature:ES|QL"],"title":"[ES|QL]
[Discover] Displays the histogram suggestion always for non
transformational
commands","number":195863,"url":"https://github.com/elastic/kibana/pull/195863","mergeCommit":{"message":"[ES|QL]
[Discover] Displays the histogram suggestion always for non
transformational commands (#195863)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/195752\r\n\r\nThis PR is fixing
2 bugs:\r\n\r\n- It filters out counter fields from the breakdown as
they are not\r\nsupported. I created a new util for this\r\n- Fixes a
bug unrelated with the breakdown (it also exists in previous\r\nminors).
The LensVis service is computing suggestions and pushes them
to\r\n`availableSuggestionsWithType `. In some indexes (it depends on
the\r\ntypes of the first 5 columns of the index) the lens suggestions
api\r\nmight return a suggestion. So in that case the array has the
histogram\r\nsuggestion + the suggestion from the suggestions api. So
the service\r\nwill pick the first one which is not the histogram. But
we know that in\r\ncase of non transformational commands we want to
suggest the histogram.\r\nSo this PR is fixing it by ensuring that the
array is cleaned up before\r\npushing the histogram
suggestion.\r\n\r\n\r\nNote: The 2 bugs are unrelated I just decided to
fix them in one PR as\r\nthey are both histogram bugs.\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","sha":"f962cdcd796af9908449155c989dd03438165773"}},"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/195863","number":195863,"mergeCommit":{"message":"[ES|QL]
[Discover] Displays the histogram suggestion always for non
transformational commands (#195863)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/195752\r\n\r\nThis PR is fixing
2 bugs:\r\n\r\n- It filters out counter fields from the breakdown as
they are not\r\nsupported. I created a new util for this\r\n- Fixes a
bug unrelated with the breakdown (it also exists in previous\r\nminors).
The LensVis service is computing suggestions and pushes them
to\r\n`availableSuggestionsWithType `. In some indexes (it depends on
the\r\ntypes of the first 5 columns of the index) the lens suggestions
api\r\nmight return a suggestion. So in that case the array has the
histogram\r\nsuggestion + the suggestion from the suggestions api. So
the service\r\nwill pick the first one which is not the histogram. But
we know that in\r\ncase of non transformational commands we want to
suggest the histogram.\r\nSo this PR is fixing it by ensuring that the
array is cleaned up before\r\npushing the histogram
suggestion.\r\n\r\n\r\nNote: The 2 bugs are unrelated I just decided to
fix them in one PR as\r\nthey are both histogram bugs.\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","sha":"f962cdcd796af9908449155c989dd03438165773"}}]}]
BACKPORT-->

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
  • Loading branch information
kibanamachine and stratoula authored Oct 14, 2024
1 parent 93e770c commit b7c0e07
Show file tree
Hide file tree
Showing 8 changed files with 106 additions and 7 deletions.
1 change: 1 addition & 0 deletions packages/kbn-esql-utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export {
retrieveMetadataColumns,
getQueryColumnsFromESQLQuery,
isESQLColumnSortable,
isESQLColumnGroupable,
TextBasedLanguages,
} from './src';

Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-esql-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,4 @@ export {
getStartEndParams,
hasStartEndParams,
} from './utils/run_query';
export { isESQLColumnSortable } from './utils/esql_fields_utils';
export { isESQLColumnSortable, isESQLColumnGroupable } from './utils/esql_fields_utils';
43 changes: 42 additions & 1 deletion packages/kbn-esql-utils/src/utils/esql_fields_utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/
import type { DatatableColumn } from '@kbn/expressions-plugin/common';
import { isESQLColumnSortable } from './esql_fields_utils';
import { isESQLColumnSortable, isESQLColumnGroupable } from './esql_fields_utils';

describe('esql fields helpers', () => {
describe('isESQLColumnSortable', () => {
Expand Down Expand Up @@ -63,4 +63,45 @@ describe('esql fields helpers', () => {
expect(isESQLColumnSortable(keywordField)).toBeTruthy();
});
});

describe('isESQLColumnGroupable', () => {
it('returns false for unsupported fields', () => {
const unsupportedField = {
id: 'unsupported',
name: 'unsupported',
meta: {
type: 'unknown',
esType: 'unknown',
},
isNull: false,
} as DatatableColumn;
expect(isESQLColumnGroupable(unsupportedField)).toBeFalsy();
});

it('returns false for counter fields', () => {
const tsdbField = {
id: 'tsbd_counter',
name: 'tsbd_counter',
meta: {
type: 'number',
esType: 'counter_long',
},
isNull: false,
} as DatatableColumn;
expect(isESQLColumnGroupable(tsdbField)).toBeFalsy();
});

it('returns true for everything else', () => {
const keywordField = {
id: 'sortable',
name: 'sortable',
meta: {
type: 'string',
esType: 'keyword',
},
isNull: false,
} as DatatableColumn;
expect(isESQLColumnGroupable(keywordField)).toBeTruthy();
});
});
});
21 changes: 21 additions & 0 deletions packages/kbn-esql-utils/src/utils/esql_fields_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { DatatableColumn } from '@kbn/expressions-plugin/common';
const SPATIAL_FIELDS = ['geo_point', 'geo_shape', 'point', 'shape'];
const SOURCE_FIELD = '_source';
const TSDB_COUNTER_FIELDS_PREFIX = 'counter_';
const UNKNOWN_FIELD = 'unknown';

/**
* Check if a column is sortable.
Expand All @@ -38,3 +39,23 @@ export const isESQLColumnSortable = (column: DatatableColumn): boolean => {

return true;
};

/**
* Check if a column is groupable (| STATS ... BY <column>).
*
* @param column The DatatableColumn of the field.
* @returns True if the column is groupable, false otherwise.
*/

export const isESQLColumnGroupable = (column: DatatableColumn): boolean => {
// we don't allow grouping on the unknown field types
if (column.meta?.type === UNKNOWN_FIELD) {
return false;
}
// we don't allow grouping on tsdb counter fields
if (column.meta?.esType && column.meta?.esType?.indexOf(TSDB_COUNTER_FIELDS_PREFIX) !== -1) {
return false;
}

return true;
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import React, { useCallback, useMemo } from 'react';
import { EuiSelectableOption } from '@elastic/eui';
import { FieldIcon, getFieldIconProps, comboBoxFieldOptionMatcher } from '@kbn/field-utils';
import { css } from '@emotion/react';
import { isESQLColumnGroupable } from '@kbn/esql-utils';
import { type DataView, DataViewField } from '@kbn/data-views-plugin/common';
import type { DatatableColumn } from '@kbn/expressions-plugin/common';
import { convertDatatableColumnToDataViewFieldSpec } from '@kbn/data-view-utils';
Expand All @@ -34,10 +35,10 @@ export interface BreakdownFieldSelectorProps {
const mapToDropdownFields = (dataView: DataView, esqlColumns?: DatatableColumn[]) => {
if (esqlColumns) {
return (
// filter out unsupported field types and counter time series metrics
esqlColumns
.filter(isESQLColumnGroupable)
.map((column) => new DataViewField(convertDatatableColumnToDataViewFieldSpec(column)))
// filter out unsupported field types
.filter((field) => field.type !== 'unknown')
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,8 @@ describe('LensVisService attributes', () => {
},
],
"query": Object {
"esql": "from logstash-* | limit 10",
"esql": "from logstash-* | limit 10
| EVAL timestamp=DATE_TRUNC(10 minute, timestamp) | stats results = count(*) by timestamp | rename timestamp as \`timestamp every 10 minute\`",
},
"visualization": Object {
"gridConfig": Object {
Expand Down Expand Up @@ -706,7 +707,7 @@ describe('LensVisService attributes', () => {
"timeField": "timestamp",
"timeInterval": undefined,
},
"suggestionType": "lensSuggestion",
"suggestionType": "histogramForESQL",
}
`);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,37 @@ describe('LensVisService suggestions', () => {
expect(lensVis.visContext?.attributes.state.query).toStrictEqual(histogramQuery);
});

test('should return histogramSuggestion even if suggestions returned by the api', async () => {
const lensVis = await getLensVisMock({
filters: [],
query: { esql: 'from the-data-view | limit 100' },
dataView: dataViewMock,
timeInterval: 'auto',
timeRange: {
from: '2023-09-03T08:00:00.000Z',
to: '2023-09-04T08:56:28.274Z',
},
breakdownField: undefined,
columns: [
{
id: 'var0',
name: 'var0',
meta: {
type: 'number',
},
},
],
isPlainRecord: true,
allSuggestions: allSuggestionsMock,
hasHistogramSuggestionForESQL: true,
});

expect(lensVis.currentSuggestionContext?.type).toBe(
UnifiedHistogramSuggestionType.histogramForESQL
);
expect(lensVis.currentSuggestionContext?.suggestion).toBeDefined();
});

test('should return histogramSuggestion if no suggestions returned by the api with a geo point breakdown field correctly', async () => {
const lensVis = await getLensVisMock({
filters: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ export class LensVisService {
let currentSuggestion: Suggestion | undefined;

// takes lens suggestions if provided
const availableSuggestionsWithType: Array<{
let availableSuggestionsWithType: Array<{
suggestion: UnifiedHistogramSuggestionContext['suggestion'];
type: UnifiedHistogramSuggestionType;
}> = [];
Expand All @@ -254,6 +254,9 @@ export class LensVisService {
breakdownField,
});
if (histogramSuggestionForESQL) {
// In case if histogram suggestion, we want to empty the array and push the new suggestion
// to ensure that only the histogram suggestion is available
availableSuggestionsWithType = [];
availableSuggestionsWithType.push({
suggestion: histogramSuggestionForESQL,
type: UnifiedHistogramSuggestionType.histogramForESQL,
Expand Down

0 comments on commit b7c0e07

Please sign in to comment.