[Aggs] Fix column.meta.type for top hit, top metric and all filtered metrics (#169834)

## Summary

When checking my PR here https://github.com/elastic/kibana/pull/169258
@stratoula noticed that the `column.meta.type` is not set properly for
last value aggregation (it always defaults to 'number', same with all
the filtered metrics too!). When I dug deeper, I noticed that happens
because we calculate it as:
```
   type:
              column.aggConfig.type.valueType
              column.aggConfig.params.field?.type ||
              'number',
```

and some of the `AggConfigs` don't have the static `valueType` property
nor field, specifically the one with nested aggregations, like top_hits,
top_metrics and filtered_metric. instead of a static `valueType`, I've
decided to change it to a method `getValueType` where we can pass
AggConfigs and get the type from different places internally. This way
`top_hits`, `top_metrics` and `filtered_metric` get the type of the
field from `customMetric`.
I also changed the values for `min` and `max` aggregation - they were
set on `number`, but they can also be a `date`.
This commit is contained in:
Marta Bondyra 2023-10-26 16:16:38 +02:00 committed by GitHub
parent 8c93e5c6ff
commit cadbaee505
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
23 changed files with 130 additions and 97 deletions

View file

@ -144,9 +144,10 @@ export class AggConfig {
}
if (aggParam.deserialize) {
const isTyped = _.isFunction(aggParam.valueType);
const valueType = aggParam.getValueType?.(this);
const isTyped = _.isFunction(valueType);
const isType = isTyped && val instanceof aggParam.valueType;
const isType = isTyped && val instanceof valueType;
const isObject = !isTyped && _.isObject(val);
const isDeserialized = isType || isObject;

View file

@ -48,7 +48,7 @@ export interface AggTypeConfig<
hasNoDsl?: boolean;
hasNoDslParams?: boolean;
params?: Array<Partial<TParam>>;
valueType?: DatatableColumnType;
getValueType?: (aggConfig: TAggConfig) => DatatableColumnType;
getRequestAggs?: ((aggConfig: TAggConfig) => TAggConfig[]) | (() => TAggConfig[] | void);
getResponseAggs?: ((aggConfig: TAggConfig) => TAggConfig[]) | (() => TAggConfig[] | void);
customLabels?: boolean;
@ -105,7 +105,7 @@ export class AggType<
* The type the values produced by this agg will have in the final data table.
* If not specified, the type of the field is used.
*/
valueType?: DatatableColumnType;
getValueType?: (aggConfig: TAggConfig) => DatatableColumnType;
/**
* a function that will be called when this aggType is assigned to
* an aggConfig, and that aggConfig is being rendered (in a form, chart, etc.).
@ -261,7 +261,7 @@ export class AggType<
this.dslName = config.dslName || config.name;
this.expressionName = config.expressionName;
this.title = config.title;
this.valueType = config.valueType;
this.getValueType = config.getValueType;
this.makeLabel = config.makeLabel || constant(this.name);
this.ordered = config.ordered;
this.hasNoDsl = !!config.hasNoDsl;

View file

@ -26,7 +26,7 @@ export const getAvgMetricAgg = () => {
name: METRIC_TYPES.AVG,
expressionName: aggAvgFnName,
title: averageTitle,
valueType: 'number',
getValueType: () => 'number',
makeLabel: (aggConfig) => {
return i18n.translate('data.search.aggs.metrics.averageLabel', {
defaultMessage: 'Average {field}',

View file

@ -25,7 +25,7 @@ export interface AggParamsCardinality extends BaseAggParams {
export const getCardinalityMetricAgg = () =>
new MetricAggType({
name: METRIC_TYPES.CARDINALITY,
valueType: 'number',
getValueType: () => 'number',
expressionName: aggCardinalityFnName,
title: uniqueCountTitle,
enableEmptyAsNull: true,

View file

@ -71,6 +71,14 @@ export const getFilteredMetricAgg = ({ getConfig }: FiltersMetricAggDependencies
const customBucket = agg.getParam('customBucket');
return bucket && bucket[customBucket.id] && customMetric.getValue(bucket[customBucket.id]);
},
getValueType(agg) {
const customMetric = agg.getParam('customMetric');
return (
customMetric.type.getValueType?.(customMetric) ||
customMetric.params.field?.type ||
'number'
);
},
getValueBucketPath(agg) {
const customBucket = agg.getParam('customBucket');
const customMetric = agg.getParam('customMetric');

View file

@ -26,7 +26,6 @@ export const getMaxMetricAgg = () => {
name: METRIC_TYPES.MAX,
expressionName: aggMaxFnName,
title: maxTitle,
valueType: 'number',
makeLabel(aggConfig) {
return i18n.translate('data.search.aggs.metrics.maxLabel', {
defaultMessage: 'Max {field}',

View file

@ -27,7 +27,7 @@ export const getMedianMetricAgg = () => {
expressionName: aggMedianFnName,
dslName: 'percentiles',
title: medianTitle,
valueType: 'number',
getValueType: () => 'number',
makeLabel(aggConfig) {
return i18n.translate('data.search.aggs.metrics.medianLabel', {
defaultMessage: 'Median {field}',

View file

@ -26,7 +26,6 @@ export const getMinMetricAgg = () => {
name: METRIC_TYPES.MIN,
expressionName: aggMinFnName,
title: minTitle,
valueType: 'number',
makeLabel(aggConfig) {
return i18n.translate('data.search.aggs.metrics.minLabel', {
defaultMessage: 'Min {field}',

View file

@ -43,7 +43,7 @@ export const getPercentilesMetricAgg = () => {
title: i18n.translate('data.search.aggs.metrics.percentilesTitle', {
defaultMessage: 'Percentiles',
}),
valueType: 'number',
getValueType: () => 'number',
makeLabel(agg) {
return i18n.translate('data.search.aggs.metrics.percentilesLabel', {
defaultMessage: 'Percentiles of {field}',

View file

@ -27,7 +27,7 @@ export const getRateMetricAgg = () => {
name: METRIC_TYPES.RATE,
expressionName: aggRateFnName,
title: rateTitle,
valueType: 'number',
getValueType: () => 'number',
makeLabel: (aggConfig) => {
return i18n.translate('data.search.aggs.metrics.rateLabel', {
defaultMessage: 'Rate of {field} per {unit}',

View file

@ -28,7 +28,7 @@ export const getSinglePercentileMetricAgg = () => {
expressionName: aggSinglePercentileFnName,
dslName: 'percentiles',
title: singlePercentileTitle,
valueType: 'number',
getValueType: () => 'number',
makeLabel(aggConfig) {
return i18n.translate('data.search.aggs.metrics.singlePercentileLabel', {
defaultMessage: 'Percentile {field}',

View file

@ -29,7 +29,7 @@ export const getSinglePercentileRankMetricAgg = () => {
expressionName: aggSinglePercentileRankFnName,
dslName: 'percentile_ranks',
title: singlePercentileTitle,
valueType: 'number',
getValueType: () => 'number',
makeLabel(aggConfig) {
return i18n.translate('data.search.aggs.metrics.singlePercentileRankLabel', {
defaultMessage: 'Percentile rank of {field}',

View file

@ -27,7 +27,7 @@ export const getSumMetricAgg = () => {
name: METRIC_TYPES.SUM,
expressionName: aggSumFnName,
title: sumTitle,
valueType: 'number',
getValueType: () => 'number',
enableEmptyAsNull: true,
makeLabel(aggConfig) {
return i18n.translate('data.search.aggs.metrics.sumLabel', {

View file

@ -48,6 +48,9 @@ export const getTopHitMetricAgg = () => {
title: i18n.translate('data.search.aggs.metrics.topHitTitle', {
defaultMessage: 'Top Hit',
}),
getValueType: (aggConfig) => {
return aggConfig.getParam('field')?.type;
},
makeLabel(aggConfig) {
const lastPrefixLabel = i18n.translate('data.search.aggs.metrics.topHit.lastPrefixLabel', {
defaultMessage: 'Last',

View file

@ -41,6 +41,9 @@ export const getTopMetricsMetricAgg = () => {
title: i18n.translate('data.search.aggs.metrics.topMetricsTitle', {
defaultMessage: 'Top metrics',
}),
getValueType: (aggConfig) => {
return aggConfig.getParam('field')?.type;
},
makeLabel(aggConfig) {
const isDescOrder = aggConfig.getParam('sortOrder').value === 'desc';
const size = aggConfig.getParam('size');

View file

@ -24,7 +24,7 @@ export interface AggParamsValueCount extends BaseAggParams {
export const getValueCountMetricAgg = () =>
new MetricAggType({
name: METRIC_TYPES.VALUE_COUNT,
valueType: 'number',
getValueType: () => 'number',
expressionName: aggValueCountFnName,
title: valueCountTitle,
enableEmptyAsNull: true,

View file

@ -52,6 +52,6 @@ export class AggParamType<
}
this.makeAgg = config.makeAgg;
this.valueType = AggConfig;
this.getValueType = () => AggConfig;
}
}

View file

@ -28,8 +28,7 @@ export class BaseParamType<TAggConfig extends IAggConfig = IAggConfig> {
deserialize: (value: any, aggConfig?: TAggConfig) => any;
toExpressionAst?: (value: any) => ExpressionAstExpression[] | ExpressionAstExpression | undefined;
options: any[];
valueType?: any;
getValueType: (aggConfig: IAggConfig) => any;
onChange?(agg: TAggConfig): void;
shouldShow?(agg: TAggConfig): boolean;
@ -71,6 +70,7 @@ export class BaseParamType<TAggConfig extends IAggConfig = IAggConfig> {
this.options = config.options;
this.modifyAggConfigOnSearchRequestStart =
config.modifyAggConfigOnSearchRequestStart || function () {};
this.valueType = config.valueType || config.type;
this.getValueType = config.getValueType;
}
}

View file

@ -10,6 +10,7 @@ import { TabbedAggResponseWriter } from './response_writer';
import { AggConfigs, BUCKET_TYPES, METRIC_TYPES } from '../aggs';
import { mockAggTypesRegistry } from '../aggs/test_helpers';
import type { TabbedResponseWriterOptions } from './types';
import { Datatable } from '@kbn/expressions-plugin/common';
describe('TabbedAggResponseWriter class', () => {
let responseWriter: TabbedAggResponseWriter;
@ -31,6 +32,48 @@ describe('TabbedAggResponseWriter class', () => {
},
];
const multipleMetricsAggConfig = [
{
type: BUCKET_TYPES.DATE_HISTOGRAM,
params: {
field: 'timestamp',
},
},
{
type: METRIC_TYPES.COUNT,
},
{
type: METRIC_TYPES.MIN,
params: {
field: 'timestamp',
},
},
{
type: METRIC_TYPES.TOP_METRICS,
params: {
field: 'geo.src',
},
},
{
type: METRIC_TYPES.FILTERED_METRIC,
schema: 'metric',
params: {
customBucket: {
type: 'filter',
params: {
filter: { language: 'kuery', query: 'a: b' },
},
},
customMetric: {
type: METRIC_TYPES.TOP_HITS,
params: {
field: 'machine.os.raw',
},
},
},
},
];
const twoSplitsAggConfig = [
{
type: BUCKET_TYPES.TERMS,
@ -56,9 +99,19 @@ describe('TabbedAggResponseWriter class', () => {
const fields = [
{
name: 'geo.src',
type: 'string',
},
{
name: 'machine.os.raw',
type: 'string',
},
{
name: 'bytes',
type: 'number',
},
{
name: 'timestamp',
type: 'date',
},
];
@ -186,7 +239,7 @@ describe('TabbedAggResponseWriter class', () => {
},
type: 'terms',
},
type: 'number',
type: 'string',
});
expect(response.columns[1]).toHaveProperty('id', 'col-1-2');
@ -252,7 +305,7 @@ describe('TabbedAggResponseWriter class', () => {
},
type: 'terms',
},
type: 'number',
type: 'string',
});
expect(response.columns[1]).toHaveProperty('id', 'col-1-2');
@ -279,6 +332,33 @@ describe('TabbedAggResponseWriter class', () => {
type: 'number',
});
});
describe('produces correct column.meta.type', () => {
let response: Datatable;
beforeAll(() => {
response = createResponseWritter(multipleMetricsAggConfig).response();
});
test('returns number if getValueType is not defined and field doesnt exist ', () => {
const countColumn = response.columns.find((column) => column.name === 'Count');
expect(countColumn?.meta.type).toEqual('number');
});
test('returns field type if getValueType is not defined', () => {
const minColumn = response.columns.find((column) =>
column.name.includes('Min timestamp')
);
expect(minColumn?.meta.type).toEqual('date');
});
test('returns field type for top metrics', () => {
const topMetricsColumn = response.columns.find((column) => column.name.includes('Last'));
expect(topMetricsColumn?.meta.type).toEqual('string');
});
test('returns correct type of the customMetric for filtered metrics', () => {
const filteredColumn = response.columns.find((column) =>
column.name.includes('Filtered')
);
expect(filteredColumn?.meta.type).toEqual('string');
});
});
});
});
});

View file

@ -74,7 +74,9 @@ export class TabbedAggResponseWriter {
name: column.name,
meta: {
type:
column.aggConfig.type.valueType || column.aggConfig.params.field?.type || 'number',
column.aggConfig.type.getValueType?.(column.aggConfig) ||
column.aggConfig.params.field?.type ||
'number',
field: column.aggConfig.params.field?.name,
index: column.aggConfig.getIndexPattern()?.title,
params: column.aggConfig.toSerializedFieldFormat(),

View file

@ -8,22 +8,10 @@
import type { Datatable } from '@kbn/expressions-plugin/common';
import { getOriginalId } from './transpose_helpers';
function isValidNumber(value: unknown): boolean {
return typeof value === 'number' || value == null;
}
export function isNumericFieldForDatatable(currentData: Datatable | undefined, accessor: string) {
const column = currentData?.columns.find(
(col) => col.id === accessor || getOriginalId(col.id) === accessor
);
// min and max aggs are reporting as number but are actually dates - work around this by checking for the date formatter until this is fixed at the source
const isNumeric = column?.meta.type === 'number' && column?.meta.params?.id !== 'date';
return (
isNumeric &&
currentData?.rows.every((row) => {
const val = row[accessor];
return isValidNumber(val) || (Array.isArray(val) && val.every(isValidNumber));
})
);
return column?.meta.type === 'number';
}

View file

@ -545,53 +545,6 @@ describe('DatatableComponent', () => {
});
});
test('it detect last_value filtered metric type', () => {
const { data, args } = sampleArgs();
const column = data.columns[1];
column.meta = {
...column.meta,
field: undefined,
type: 'number',
sourceParams: { ...column.meta.sourceParams, type: 'filtered_metric' },
};
data.rows[0].b = 'Hello';
const wrapper = shallow(
<DatatableComponent
data={data}
args={{
...args,
columns: [
{ columnId: 'a', alignment: 'center', type: 'lens_datatable_column' },
{ columnId: 'b', type: 'lens_datatable_column' },
{ columnId: 'c', type: 'lens_datatable_column' },
],
sortingColumnId: 'b',
sortingDirection: 'desc',
}}
formatFactory={() => ({ convert: (x) => x } as IFieldFormat)}
dispatchEvent={onDispatchEvent}
getType={jest.fn()}
renderMode="view"
paletteService={chartPluginMock.createPaletteRegistry()}
theme={setUpMockTheme}
interactive
renderComplete={renderComplete}
/>
);
expect(wrapper.find(DataContext.Provider).prop('value').alignments).toEqual({
// set via args
a: 'center',
// default for string
b: 'left',
// default for number
c: 'right',
});
});
test('it should refresh the table header when the datatable data changes', () => {
const { data, args } = sampleArgs();

View file

@ -261,20 +261,17 @@ export const DatatableComponent = (props: DatatableRenderProps) => {
[onEditAction, setColumnConfig, columnConfig, isInteractive]
);
const isNumericMap: Record<string, boolean> = useMemo(() => {
const numericMap: Record<string, boolean> = {};
for (const column of firstLocalTable.columns) {
// filtered metrics result as "number" type, but have no field
numericMap[column.id] =
(column.meta.type === 'number' && column.meta.field != null) ||
// as fallback check the first available value type
// mind here: date can be seen as numbers, to carefully check that is a filtered metric
(column.meta.field == null &&
typeof firstLocalTable.rows.find((row) => row[column.id] != null)?.[column.id] ===
'number');
}
return numericMap;
}, [firstLocalTable]);
const isNumericMap: Record<string, boolean> = useMemo(
() =>
firstLocalTable.columns.reduce<Record<string, boolean>>(
(map, column) => ({
...map,
[column.id]: column.meta.type === 'number',
}),
{}
),
[firstLocalTable]
);
const alignments: Record<string, 'left' | 'right' | 'center'> = useMemo(() => {
const alignmentMap: Record<string, 'left' | 'right' | 'center'> = {};