Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] optimize percentiles fetching #131875

Merged
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9c2a41d
first attempt at grouping percentage aggs
drewdaemon May 9, 2022
7b264fe
include only columns with corresponding esAggs fns in idMap
drewdaemon May 9, 2022
6fa190a
make restore column id algo independent of agg order
drewdaemon May 9, 2022
51b4cce
update ID map deterministically following optimization
drewdaemon May 10, 2022
eba3156
move optimization logic to operation class
drewdaemon May 11, 2022
5d65cad
group percentiles by timeshift as well as field
drewdaemon May 11, 2022
0cdc172
Merge branch 'main' into 126941/optimize-percentiles-fetching
kibanamachine Jun 3, 2022
62043dc
Merge branch 'main' into 126941/optimize-percentiles-fetching
drewdaemon Jun 9, 2022
7610415
support one-to-many for remapping columns
drewdaemon Jun 13, 2022
4ea17ed
handle duplicate percentile dimensions
drewdaemon Jun 13, 2022
aee9e26
basic collapse-by-parameter unit test
drewdaemon Jun 13, 2022
a6cdca5
more tests
drewdaemon Jun 13, 2022
970a78b
update indexpattern toExpression test
drewdaemon Jun 13, 2022
edd4061
Merge branch 'main' into 126941/optimize-percentiles-fetching
kibanamachine Jun 14, 2022
5d61742
test optimizeEsAggs contract
drewdaemon Jun 14, 2022
78bfdce
test updating idMap based on new agg order
drewdaemon Jun 14, 2022
7c5eb48
Merge branch '126941/optimize-percentiles-fetching' of github.com:and…
drewdaemon Jun 14, 2022
3ca5e4f
code improvements
drewdaemon Jun 16, 2022
d35a3f3
Merge branch 'main' into 126941/optimize-percentiles-fetching
kibanamachine Jun 16, 2022
957923d
Merge branch 'main' into 126941/optimize-percentiles-fetching
kibanamachine Jun 20, 2022
ff96f55
renameColumns -> mapToColumns
drewdaemon Jun 20, 2022
c67dd72
remove unused translations
drewdaemon Jun 20, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,18 @@ describe('rename_columns', () => {
};

const idMap = {
a: {
id: 'b',
label: 'Austrailia',
},
b: {
id: 'c',
label: 'Boomerang',
},
a: [
{
id: 'b',
label: 'Austrailia',
},
],
b: [
{
id: 'c',
label: 'Boomerang',
},
],
};

const result = await renameColumns.fn(
Expand Down Expand Up @@ -99,7 +103,7 @@ describe('rename_columns', () => {
};

const idMap = {
b: { id: 'c', label: 'Catamaran' },
b: [{ id: 'c', label: 'Catamaran' }],
};

const result = await renameColumns.fn(
Expand Down Expand Up @@ -149,6 +153,67 @@ describe('rename_columns', () => {
`);
});

it('should map to multiple original columns', async () => {
const input: Datatable = {
type: 'datatable',
columns: [{ id: 'b', name: 'B', meta: { type: 'number' } }],
rows: [{ b: 2 }, { b: 4 }, { b: 6 }, { b: 8 }],
};

const idMap = {
b: [
{ id: 'c', label: 'Catamaran' },
{ id: 'd', label: 'Dinghy' },
],
};

const result = await renameColumns.fn(
input,
{ idMap: JSON.stringify(idMap) },
createMockExecutionContext()
);

expect(result).toMatchInlineSnapshot(`
Object {
"columns": Array [
Object {
"id": "c",
"meta": Object {
"type": "number",
},
"name": "Catamaran",
},
Object {
"id": "d",
"meta": Object {
"type": "number",
},
"name": "Dinghy",
},
],
"rows": Array [
Object {
"c": 2,
"d": 2,
},
Object {
"c": 4,
"d": 4,
},
Object {
"c": 6,
"d": 6,
},
Object {
"c": 8,
"d": 8,
},
],
"type": "datatable",
}
`);
});

it('should rename date histograms', async () => {
const input: Datatable = {
type: 'datatable',
Expand All @@ -165,7 +230,7 @@ describe('rename_columns', () => {
};

const idMap = {
b: { id: 'c', label: 'Apple', operationType: 'date_histogram', sourceField: 'banana' },
b: [{ id: 'c', label: 'Apple', operationType: 'date_histogram', sourceField: 'banana' }],
};

const result = await renameColumns.fn(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
*/

import { i18n } from '@kbn/i18n';
import type { RenameColumnsExpressionFunction } from './types';
import type { MapToOriginalColumnsExpressionFunction } from './types';

export const renameColumns: RenameColumnsExpressionFunction = {
name: 'lens_rename_columns',
export const renameColumns: MapToOriginalColumnsExpressionFunction = {
name: 'lens_map_to_original_columns',
Copy link
Contributor Author

@drewdaemon drewdaemon Jun 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we think about this expression function name change? I understand that lens_rename_columns was meant to be general wording, but it isn't used anywhere else and this name more clearly communicates how the function is actually used IMO.

If it seems better to others as well, I can rename the files to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dej611 any feelings on this one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion here.
Just noticed there's a mention of lens_rename_columns into x-pack/plugins/lens/server/migrations/saved_object_migrations.test.ts to be migrated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the mentions. But, I'm not sure we should change those expressions since they're in tests for previous Kibana versions. WDYT?

type: 'datatable',
help: i18n.translate('xpack.lens.functions.renameColumns.help', {
defaultMessage: 'A helper to rename the columns of a datatable',
Expand All @@ -26,7 +26,7 @@ export const renameColumns: RenameColumnsExpressionFunction = {
inputTypes: ['datatable'],
async fn(...args) {
/** Build optimization: prevent adding extra code into initial bundle **/
const { renameColumnFn } = await import('./rename_columns_fn');
return renameColumnFn(...args);
const { mapToOriginalColumns } = await import('./rename_columns_fn');
return mapToOriginalColumns(...args);
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import type { DatatableColumn } from '@kbn/expressions-plugin/common';
import type { OriginalColumn, RenameColumnsExpressionFunction } from './types';
import type { OriginalColumn, MapToOriginalColumnsExpressionFunction } from './types';

function getColumnName(originalColumn: OriginalColumn, newColumn: DatatableColumn) {
if (originalColumn?.operationType === 'date_histogram') {
Expand All @@ -21,42 +21,43 @@ function getColumnName(originalColumn: OriginalColumn, newColumn: DatatableColum
return originalColumn.label;
}

export const renameColumnFn: RenameColumnsExpressionFunction['fn'] = (
export const mapToOriginalColumns: MapToOriginalColumnsExpressionFunction['fn'] = (
data,
{ idMap: encodedIdMap }
) => {
const idMap = JSON.parse(encodedIdMap) as Record<string, OriginalColumn>;
const idMap = JSON.parse(encodedIdMap) as Record<string, OriginalColumn[]>;

return {
...data,
rows: data.rows.map((row) => {
const mappedRow: Record<string, unknown> = {};
Object.entries(idMap).forEach(([fromId, toId]) => {
mappedRow[toId.id] = row[fromId];
});

Object.entries(row).forEach(([id, value]) => {
if (id in idMap) {
mappedRow[idMap[id].id] = value;
idMap[id].forEach(({ id: originalId }) => {
mappedRow[originalId] = value;
});
} else {
mappedRow[id] = value;
}
});

return mappedRow;
}),
columns: data.columns.map((column) => {
const mappedItem = idMap[column.id];

if (!mappedItem) {
return column;
}

return {
...column,
id: mappedItem.id,
name: getColumnName(mappedItem, column),
};
}),
columns: data.columns
.map((column) => {
const originalColumns = idMap[column.id];

if (!originalColumns) {
return column;
}

return originalColumns.map((originalColumn) => ({
...column,
id: originalColumn.id,
name: getColumnName(originalColumn, column),
}));
})
.flat(),
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ export type OriginalColumn = { id: string; label: string } & (
| { operationType: string; sourceField: never }
);

export type RenameColumnsExpressionFunction = ExpressionFunctionDefinition<
'lens_rename_columns',
export type MapToOriginalColumnsExpressionFunction = ExpressionFunctionDefinition<
'lens_map_to_original_columns',
Datatable,
{
idMap: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -491,10 +491,10 @@ describe('IndexPattern Data Source', () => {
Object {
"arguments": Object {
"idMap": Array [
"{\\"col-0-0\\":{\\"label\\":\\"Count of records\\",\\"dataType\\":\\"number\\",\\"isBucketed\\":false,\\"sourceField\\":\\"___records___\\",\\"operationType\\":\\"count\\",\\"id\\":\\"col1\\"},\\"col-1-1\\":{\\"label\\":\\"Date\\",\\"dataType\\":\\"date\\",\\"isBucketed\\":true,\\"operationType\\":\\"date_histogram\\",\\"sourceField\\":\\"timestamp\\",\\"params\\":{\\"interval\\":\\"1d\\"},\\"id\\":\\"col2\\"}}",
"{\\"col-0-0\\":[{\\"label\\":\\"Count of records\\",\\"dataType\\":\\"number\\",\\"isBucketed\\":false,\\"sourceField\\":\\"___records___\\",\\"operationType\\":\\"count\\",\\"id\\":\\"col1\\"}],\\"col-1-1\\":[{\\"label\\":\\"Date\\",\\"dataType\\":\\"date\\",\\"isBucketed\\":true,\\"operationType\\":\\"date_histogram\\",\\"sourceField\\":\\"timestamp\\",\\"params\\":{\\"interval\\":\\"1d\\"},\\"id\\":\\"col2\\"}]}",
],
},
"function": "lens_rename_columns",
"function": "lens_map_to_original_columns",
"type": "function",
},
],
Expand Down Expand Up @@ -905,9 +905,9 @@ describe('IndexPattern Data Source', () => {
const ast = indexPatternDatasource.toExpression(state, 'first') as Ast;
expect(ast.chain[1].arguments.metricsAtAllLevels).toEqual([false]);
expect(JSON.parse(ast.chain[2].arguments.idMap[0] as string)).toEqual({
'col-0-0': expect.objectContaining({ id: 'bucket1' }),
'col-1-1': expect.objectContaining({ id: 'bucket2' }),
'col-2-2': expect.objectContaining({ id: 'metric' }),
'col-0-0': [expect.objectContaining({ id: 'bucket1' })],
'col-1-1': [expect.objectContaining({ id: 'bucket2' })],
'col-2-2': [expect.objectContaining({ id: 'metric' })],
});
});

Expand Down Expand Up @@ -1026,10 +1026,13 @@ describe('IndexPattern Data Source', () => {
const state = enrichBaseState(queryBaseState);

const ast = indexPatternDatasource.toExpression(state, 'first') as Ast;

expect(JSON.parse(ast.chain[2].arguments.idMap[0] as string)).toEqual({
'col-0-0': expect.objectContaining({
id: 'col1',
}),
'col-0-0': [
expect.objectContaining({
id: 'col1',
}),
],
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import {
CoreStart,
} from '@kbn/core/public';
import { IStorageWrapper } from '@kbn/kibana-utils-plugin/public';
import { ExpressionAstFunction } from '@kbn/expressions-plugin/public';
import {
ExpressionAstExpressionBuilder,
ExpressionAstFunction,
} from '@kbn/expressions-plugin/public';
import { DataPublicPluginStart } from '@kbn/data-plugin/public';
import { UnifiedSearchPublicPluginStart } from '@kbn/unified-search-plugin/public';
import { DataViewsPublicPluginStart } from '@kbn/data-views-plugin/public';
Expand Down Expand Up @@ -55,6 +58,7 @@ import { IndexPattern, IndexPatternField, IndexPatternLayer } from '../../types'
import { DateRange, LayerType } from '../../../../common';
import { rangeOperation } from './ranges';
import { IndexPatternDimensionEditorProps, OperationSupportMatrix } from '../../dimension_panel';
import type { OriginalColumn } from '../../to_expression';

export type {
IncompleteColumn,
Expand Down Expand Up @@ -378,6 +382,17 @@ interface BaseOperationDefinitionProps<C extends BaseIndexPatternColumn, P = {}>
* Title for the help component
*/
helpComponentTitle?: string;
/**
* Optimizes EsAggs expression. Invoked only once per operation type. May mutate arguments.
*/
optimizeEsAggs?: (
aggs: ExpressionAstExpressionBuilder[],
esAggsIdMap: Record<string, OriginalColumn[]>,
aggExpressionToEsAggsIdMap: Map<ExpressionAstExpressionBuilder, string>
) => {
aggs: ExpressionAstExpressionBuilder[];
esAggsIdMap: Record<string, OriginalColumn[]>;
};
}

interface BaseBuildColumnArgs {
Expand Down
Loading