Skip to content

Commit

Permalink
[Aggs] Fix column.meta.type for top hit, top metric and all filtered …
Browse files Browse the repository at this point in the history
…metrics (elastic#169834)

## Summary

When checking my PR here elastic#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`.
  • Loading branch information
mbondyra authored and awahab07 committed Oct 31, 2023
1 parent 53ad6ef commit 139eb44
Show file tree
Hide file tree
Showing 23 changed files with 130 additions and 97 deletions.
5 changes: 3 additions & 2 deletions src/plugins/data/common/search/aggs/agg_config.ts
Expand Up @@ -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;

Expand Down
6 changes: 3 additions & 3 deletions src/plugins/data/common/search/aggs/agg_type.ts
Expand Up @@ -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;
Expand Down Expand Up @@ -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.).
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/search/aggs/metrics/avg.ts
Expand Up @@ -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}',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/search/aggs/metrics/cardinality.ts
Expand Up @@ -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,
Expand Down
Expand Up @@ -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');
Expand Down
1 change: 0 additions & 1 deletion src/plugins/data/common/search/aggs/metrics/max.ts
Expand Up @@ -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}',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/search/aggs/metrics/median.ts
Expand Up @@ -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}',
Expand Down
1 change: 0 additions & 1 deletion src/plugins/data/common/search/aggs/metrics/min.ts
Expand Up @@ -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}',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/search/aggs/metrics/percentiles.ts
Expand Up @@ -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}',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/search/aggs/metrics/rate.ts
Expand Up @@ -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}',
Expand Down
Expand Up @@ -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}',
Expand Down
Expand Up @@ -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}',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/search/aggs/metrics/sum.ts
Expand Up @@ -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', {
Expand Down
3 changes: 3 additions & 0 deletions src/plugins/data/common/search/aggs/metrics/top_hit.ts
Expand Up @@ -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',
Expand Down
3 changes: 3 additions & 0 deletions src/plugins/data/common/search/aggs/metrics/top_metrics.ts
Expand Up @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/search/aggs/metrics/value_count.ts
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/search/aggs/param_types/agg.ts
Expand Up @@ -52,6 +52,6 @@ export class AggParamType<
}

this.makeAgg = config.makeAgg;
this.valueType = AggConfig;
this.getValueType = () => AggConfig;
}
}
6 changes: 3 additions & 3 deletions src/plugins/data/common/search/aggs/param_types/base.ts
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
}
84 changes: 82 additions & 2 deletions src/plugins/data/common/search/tabify/response_writer.test.ts
Expand Up @@ -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;
Expand All @@ -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,
Expand All @@ -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',
},
];

Expand Down Expand Up @@ -186,7 +239,7 @@ describe('TabbedAggResponseWriter class', () => {
},
type: 'terms',
},
type: 'number',
type: 'string',
});

expect(response.columns[1]).toHaveProperty('id', 'col-1-2');
Expand Down Expand Up @@ -252,7 +305,7 @@ describe('TabbedAggResponseWriter class', () => {
},
type: 'terms',
},
type: 'number',
type: 'string',
});

expect(response.columns[1]).toHaveProperty('id', 'col-1-2');
Expand All @@ -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');
});
});
});
});
});
4 changes: 3 additions & 1 deletion src/plugins/data/common/search/tabify/response_writer.ts
Expand Up @@ -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(),
Expand Down
14 changes: 1 addition & 13 deletions x-pack/plugins/lens/common/expressions/datatable/utils.ts
Expand Up @@ -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';
}

0 comments on commit 139eb44

Please sign in to comment.