Skip to content

Commit

Permalink
[Lens] Do not add math columns for pass-through operations (#102656) (#…
Browse files Browse the repository at this point in the history 
…103075)

Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>
  • Loading branch information
@kibanamachine @flash1293
kibanamachine and flash1293 authored Jun 23, 2021
1 parent e583c6d commit 797c29c
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 73 deletions.
23 changes: 21 additions & 2 deletions .../plugins/lens/public/indexpattern_datasource/operations/definitions/calculations/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@

import { i18n } from '@kbn/i18n';
import type { ExpressionFunctionAST } from '@kbn/interpreter/common';
import memoizeOne from 'memoize-one';
import type { TimeScaleUnit } from '../../../time_scale';
import type { IndexPattern, IndexPatternLayer } from '../../../types';
import { adjustTimeScaleLabelSuffix } from '../../time_scale_utils';
import type { ReferenceBasedIndexPatternColumn } from '../column_types';
import { isColumnValidAsReference } from '../../layer_helpers';
import { getManagedColumnsFrom, isColumnValidAsReference } from '../../layer_helpers';
import { operationDefinitionMap } from '..';

export const buildLabelFunction = (ofName: (name?: string) => string) => (
Expand Down Expand Up @@ -45,6 +46,23 @@ export function checkForDateHistogram(layer: IndexPatternLayer, name: string) {
];
}

const getFullyManagedColumnIds = memoizeOne((layer: IndexPatternLayer) => {
const managedColumnIds = new Set<string>();
Object.entries(layer.columns).forEach(([id, column]) => {
if (
'references' in column &&
operationDefinitionMap[column.operationType].input === 'managedReference'
) {
managedColumnIds.add(id);
const managedColumns = getManagedColumnsFrom(id, layer.columns);
managedColumns.map(([managedId]) => {
managedColumnIds.add(managedId);
});
}
});
return managedColumnIds;
});

export function checkReferences(layer: IndexPatternLayer, columnId: string) {
const column = layer.columns[columnId] as ReferenceBasedIndexPatternColumn;

Expand Down Expand Up @@ -72,7 +90,8 @@ export function checkReferences(layer: IndexPatternLayer, columnId: string) {
column: referenceColumn,
});

if (!isValid) {
// do not enforce column validity if current column is part of managed subtree
if (!isValid && !getFullyManagedColumnIds(layer).has(columnId)) {
errors.push(
i18n.translate('xpack.lens.indexPattern.invalidReferenceConfiguration', {
defaultMessage: 'Dimension "{dimensionLabel}" is configured incorrectly',
Expand Down
20 changes: 4 additions & 16 deletions ...ugins/lens/public/indexpattern_datasource/operations/definitions/formula/formula.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -413,13 +413,13 @@ describe('formula', () => {
).newLayer
).toEqual({
...layer,
columnOrder: ['col1X0', 'col1X1', 'col1'],
columnOrder: ['col1X0', 'col1'],
columns: {
...layer.columns,
col1: {
...currentColumn,
label: 'average(bytes)',
references: ['col1X1'],
references: ['col1X0'],
params: {
...currentColumn.params,
formula: 'average(bytes)',
Expand All @@ -436,18 +436,6 @@ describe('formula', () => {
sourceField: 'bytes',
timeScale: false,
},
col1X1: {
customLabel: true,
dataType: 'number',
isBucketed: false,
label: 'Part of average(bytes)',
operationType: 'math',
params: {
tinymathAst: 'col1X0',
},
references: ['col1X0'],
scale: 'ratio',
},
},
});
});
Expand Down Expand Up @@ -568,8 +556,8 @@ describe('formula', () => {
).locations
).toEqual({
col1X0: { min: 15, max: 29 },
col1X2: { min: 0, max: 41 },
col1X3: { min: 42, max: 50 },
col1X1: { min: 0, max: 41 },
col1X2: { min: 42, max: 50 },
});
});
});
Expand Down
52 changes: 31 additions & 21 deletions x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/parse.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,17 +123,20 @@ function extractColumns(
if (nodeOperation.input === 'fullReference') {
const [referencedOp] = functions;
const consumedParam = parseNode(referencedOp);
const hasActualMathContent = typeof consumedParam !== 'string';

const subNodeVariables = consumedParam ? findVariables(consumedParam) : [];
const mathColumn = mathOperation.buildColumn({
layer,
indexPattern,
});
mathColumn.references = subNodeVariables.map(({ value }) => value);
mathColumn.params.tinymathAst = consumedParam!;
columns.push({ column: mathColumn });
mathColumn.customLabel = true;
mathColumn.label = label;
if (hasActualMathContent) {
const subNodeVariables = consumedParam ? findVariables(consumedParam) : [];
const mathColumn = mathOperation.buildColumn({
layer,
indexPattern,
});
mathColumn.references = subNodeVariables.map(({ value }) => value);
mathColumn.params.tinymathAst = consumedParam!;
columns.push({ column: mathColumn });
mathColumn.customLabel = true;
mathColumn.label = label;
}

const mappedParams = getOperationParams(nodeOperation, namedArguments || []);
const newCol = (nodeOperation as OperationDefinition<
Expand All @@ -143,7 +146,11 @@ function extractColumns(
{
layer,
indexPattern,
referenceIds: [getManagedId(idPrefix, columns.length - 1)],
referenceIds: [
hasActualMathContent
? getManagedId(idPrefix, columns.length - 1)
: (consumedParam as string),
],
},
mappedParams
);
Expand All @@ -160,16 +167,19 @@ function extractColumns(
if (root === undefined) {
return [];
}
const variables = findVariables(root);
const mathColumn = mathOperation.buildColumn({
layer,
indexPattern,
});
mathColumn.references = variables.map(({ value }) => value);
mathColumn.params.tinymathAst = root!;
mathColumn.customLabel = true;
mathColumn.label = label;
columns.push({ column: mathColumn });
const topLevelMath = typeof root !== 'string';
if (topLevelMath) {
const variables = findVariables(root);
const mathColumn = mathOperation.buildColumn({
layer,
indexPattern,
});
mathColumn.references = variables.map(({ value }) => value);
mathColumn.params.tinymathAst = root!;
mathColumn.customLabel = true;
mathColumn.label = label;
columns.push({ column: mathColumn });
}
return columns;
}

Expand Down
63 changes: 29 additions & 34 deletions x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { documentField } from '../document_field';
import { getFieldByNameFactory } from '../pure_helpers';
import { generateId } from '../../id_generator';
import { createMockedFullReference, createMockedManagedReference } from './mocks';
import { TinymathAST } from 'packages/kbn-tinymath';

jest.mock('../operations');
jest.mock('../../id_generator');
Expand Down Expand Up @@ -105,28 +106,34 @@ describe('state_helpers', () => {
const source = {
dataType: 'number' as const,
isBucketed: false,
label: 'moving_average(sum(bytes), window=5)',
label: '5 + moving_average(sum(bytes), window=5)',
operationType: 'formula' as const,
params: {
formula: 'moving_average(sum(bytes), window=5)',
formula: '5 + moving_average(sum(bytes), window=5)',
isFormulaBroken: false,
},
references: ['formulaX1'],
references: ['formulaX2'],
};
const math = {
customLabel: true,
dataType: 'number' as const,
isBucketed: false,
label: 'Part of moving_average(sum(bytes), window=5)',
operationType: 'math' as const,
params: { tinymathAst: 'formulaX2' },
references: ['formulaX2'],
label: 'Part of 5 + moving_average(sum(bytes), window=5)',
references: ['formulaX1'],
params: {
tinymathAst: {
type: 'function',
name: 'add',
args: [5, 'formulaX1'],
} as TinymathAST,
},
};
const sum = {
customLabel: true,
dataType: 'number' as const,
isBucketed: false,
label: 'Part of moving_average(sum(bytes), window=5)',
label: 'Part of 5 + moving_average(sum(bytes), window=5)',
operationType: 'sum' as const,
scale: 'ratio' as const,
sourceField: 'bytes',
Expand All @@ -135,7 +142,7 @@ describe('state_helpers', () => {
customLabel: true,
dataType: 'number' as const,
isBucketed: false,
label: 'Part of moving_average(sum(bytes), window=5)',
label: 'Part of 5 + moving_average(sum(bytes), window=5)',
operationType: 'moving_average' as const,
params: { window: 5 },
references: ['formulaX0'],
Expand All @@ -148,14 +155,8 @@ describe('state_helpers', () => {
columns: {
source,
formulaX0: sum,
formulaX1: math,
formulaX2: movingAvg,
formulaX3: {
...math,
label: 'Part of moving_average(sum(bytes), window=5)',
references: ['formulaX2'],
params: { tinymathAst: 'formulaX2' },
},
formulaX1: movingAvg,
formulaX2: math,
},
},
targetId: 'copy',
Expand All @@ -171,40 +172,34 @@ describe('state_helpers', () => {
'formulaX0',
'formulaX1',
'formulaX2',
'formulaX3',
'copyX0',
'copyX1',
'copyX2',
'copyX3',
'copy',
],
columns: {
source,
formulaX0: sum,
formulaX1: math,
formulaX2: movingAvg,
formulaX3: {
...math,
references: ['formulaX2'],
params: { tinymathAst: 'formulaX2' },
},
copy: expect.objectContaining({ ...source, references: ['copyX3'] }),
formulaX1: movingAvg,
formulaX2: math,
copy: expect.objectContaining({ ...source, references: ['copyX2'] }),
copyX0: expect.objectContaining({
...sum,
}),
copyX1: expect.objectContaining({
...math,
...movingAvg,
references: ['copyX0'],
params: { tinymathAst: 'copyX0' },
}),
copyX2: expect.objectContaining({
...movingAvg,
references: ['copyX1'],
}),
copyX3: expect.objectContaining({
...math,
references: ['copyX2'],
params: { tinymathAst: 'copyX2' },
references: ['copyX1'],
params: {
tinymathAst: expect.objectContaining({
type: 'function',
name: 'add',
args: [5, 'copyX1'],
} as TinymathAST),
},
}),
},
});
Expand Down

0 comments on commit 797c29c

Please sign in to comment.