Skip to content

Commit

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

## Summary

Closes #195752

This PR is fixing 2 bugs:

- It filters out counter fields from the breakdown as they are not
supported. I created a new util for this
- Fixes a bug unrelated with the breakdown (it also exists in previous
minors). The LensVis service is computing suggestions and pushes them to
`availableSuggestionsWithType `. In some indexes (it depends on the
types of the first 5 columns of the index) the lens suggestions api
might return a suggestion. So in that case the array has the histogram
suggestion + the suggestion from the suggestions api. So the service
will pick the first one which is not the histogram. But we know that in
case of non transformational commands we want to suggest the histogram.
So this PR is fixing it by ensuring that the array is cleaned up before
pushing the histogram suggestion.


Note: The 2 bugs are unrelated I just decided to fix them in one PR as
they are both histogram bugs.

### 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
  • Loading branch information
stratoula authored Oct 14, 2024
1 parent fed9a19 commit f962cdc
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 f962cdc

Please sign in to comment.