From 662bd9a410adf5669aba0f4a2fcb66636e9d548c Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Tue, 28 Feb 2023 09:15:13 -0600 Subject: [PATCH] [Lens] always retain source order for multi-metric partition chart layers (#151949) --- .../partition_vis_component.test.tsx.snap | 2 +- .../public/utils/layers/get_color.test.ts | 290 ++++++++++++++ .../public/utils/layers/get_layers.test.ts | 366 +++++------------- .../public/utils/layers/get_layers.ts | 8 +- .../public/utils/layers/sort_predicate.ts | 2 +- .../visualizations/partition/to_expression.ts | 24 +- .../partition/visualization.test.ts | 230 ++++++----- .../partition/visualization.tsx | 26 +- 8 files changed, 562 insertions(+), 386 deletions(-) diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/components/__snapshots__/partition_vis_component.test.tsx.snap b/src/plugins/chart_expressions/expression_partition_vis/public/components/__snapshots__/partition_vis_component.test.tsx.snap index c91e491887a99e..1a566571b4d6c0 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/components/__snapshots__/partition_vis_component.test.tsx.snap +++ b/src/plugins/chart_expressions/expression_partition_vis/public/components/__snapshots__/partition_vis_component.test.tsx.snap @@ -1267,7 +1267,7 @@ exports[`PartitionVisComponent should render correct structure for multi-metric "fillColor": [Function], }, "showAccessor": [Function], - "sortPredicate": undefined, + "sortPredicate": [Function], }, ] } diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_color.test.ts b/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_color.test.ts index 225f405bac2ca1..06fa1d9aae83b7 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_color.test.ts +++ b/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_color.test.ts @@ -10,6 +10,15 @@ import type { PaletteOutput, PaletteDefinition } from '@kbn/coloring'; import { chartPluginMock } from '@kbn/charts-plugin/public/mocks'; import { Datatable } from '@kbn/expressions-plugin/common'; import { byDataColorPaletteMap } from './get_color'; +import { ShapeTreeNode } from '@elastic/charts'; +import type { SeriesLayer } from '@kbn/coloring'; +import { dataPluginMock } from '@kbn/data-plugin/public/mocks'; +import { fieldFormatsMock } from '@kbn/field-formats-plugin/common/mocks'; +import type { DataPublicPluginStart } from '@kbn/data-plugin/public'; +import { getColor } from './get_color'; +import { createMockVisData, createMockBucketColumns, createMockPieParams } from '../../mocks'; +import { generateFormatters } from '../formatters'; +import { ChartTypes } from '../../../common/types'; describe('#byDataColorPaletteMap', () => { let datatable: Datatable; @@ -88,3 +97,284 @@ describe('#byDataColorPaletteMap', () => { ); }); }); + +describe('getColor', () => { + const visData = createMockVisData(); + const buckets = createMockBucketColumns(); + const visParams = createMockPieParams(); + const colors = ['color1', 'color2', 'color3', 'color4']; + const dataMock = dataPluginMock.createStartContract(); + interface RangeProps { + gte: number; + lt: number; + } + const defaultFormatter = jest.fn((...args) => fieldFormatsMock.deserialize(...args)); + const formatters = generateFormatters(visData, defaultFormatter); + + dataMock.fieldFormats = { + deserialize: jest.fn(() => ({ + convert: jest.fn((s: RangeProps) => { + return `≥ ${s.gte} and < ${s.lt}`; + }), + })), + } as unknown as DataPublicPluginStart['fieldFormats']; + + const getPaletteRegistry = () => { + const mockPalette1: jest.Mocked = { + id: 'default', + title: 'My Palette', + getCategoricalColor: jest.fn((layer: SeriesLayer[]) => colors[layer[0].rankAtDepth]), + getCategoricalColors: jest.fn((num: number) => colors), + toExpression: jest.fn(() => ({ + type: 'expression', + chain: [ + { + type: 'function', + function: 'system_palette', + arguments: { + name: ['default'], + }, + }, + ], + })), + }; + + return { + get: () => mockPalette1, + getAll: () => [mockPalette1], + }; + }; + it('should return the correct color based on the parent sortIndex', () => { + const d = { + dataName: 'ES-Air', + depth: 1, + sortIndex: 0, + parent: { + children: [['ES-Air'], ['Kibana Airlines']], + depth: 0, + sortIndex: 0, + }, + } as unknown as ShapeTreeNode; + const color = getColor( + ChartTypes.PIE, + d, + 0, + false, + {}, + buckets, + visData.rows, + visParams, + getPaletteRegistry(), + { getColor: () => undefined }, + false, + false, + dataMock.fieldFormats, + visData.columns[0], + formatters + ); + expect(color).toEqual(colors[0]); + }); + + it('slices with the same label should have the same color for small multiples', () => { + const d = { + dataName: 'ES-Air', + depth: 1, + sortIndex: 0, + parent: { + children: [['ES-Air'], ['Kibana Airlines']], + depth: 0, + sortIndex: 0, + }, + } as unknown as ShapeTreeNode; + const color = getColor( + ChartTypes.PIE, + d, + 0, + true, + {}, + buckets, + visData.rows, + visParams, + getPaletteRegistry(), + { getColor: () => undefined }, + false, + false, + dataMock.fieldFormats, + visData.columns[0], + formatters + ); + expect(color).toEqual('color3'); + }); + it('returns the overwriteColor if exists', () => { + const d = { + dataName: 'ES-Air', + depth: 1, + sortIndex: 0, + parent: { + children: [['ES-Air'], ['Kibana Airlines']], + depth: 0, + sortIndex: 0, + }, + } as unknown as ShapeTreeNode; + const color = getColor( + ChartTypes.PIE, + d, + 0, + true, + { 'ES-Air': '#000028' }, + buckets, + visData.rows, + visParams, + getPaletteRegistry(), + { getColor: () => undefined }, + false, + false, + dataMock.fieldFormats, + visData.columns[0], + formatters + ); + expect(color).toEqual('#000028'); + }); + + it('returns the overwriteColor for older visualizations with formatted values', () => { + const d = { + dataName: { + gte: 1000, + lt: 2000, + }, + depth: 1, + sortIndex: 0, + parent: { + children: [ + [ + { + gte: 1000, + lt: 2000, + }, + ], + [ + { + gte: 2000, + lt: 3000, + }, + ], + ], + depth: 0, + sortIndex: 0, + }, + } as unknown as ShapeTreeNode; + const visParamsNew = { + ...visParams, + distinctColors: true, + }; + const column = { + ...visData.columns[0], + format: { + id: 'range', + params: { + id: 'number', + }, + }, + }; + const color = getColor( + ChartTypes.PIE, + d, + 0, + true, + { '≥ 1000 and < 2000': '#3F6833' }, + buckets, + visData.rows, + visParamsNew, + getPaletteRegistry(), + { getColor: () => undefined }, + false, + false, + dataMock.fieldFormats, + column, + formatters + ); + expect(color).toEqual('#3F6833'); + }); + + it('should only pass the second layer for mosaic', () => { + const d = { + dataName: 'Second level 1', + depth: 2, + sortIndex: 0, + parent: { + children: [['Second level 1'], ['Second level 2']], + depth: 1, + sortIndex: 0, + parent: { + children: [['First level']], + depth: 0, + sortIndex: 0, + }, + }, + } as unknown as ShapeTreeNode; + const registry = getPaletteRegistry(); + getColor( + ChartTypes.MOSAIC, + d, + 1, + true, + {}, + buckets, + visData.rows, + visParams, + registry, + undefined, + true, + false, + dataMock.fieldFormats, + visData.columns[0], + formatters + ); + expect(registry.get().getCategoricalColor).toHaveBeenCalledWith( + [expect.objectContaining({ name: 'Second level 1' })], + expect.anything(), + expect.anything() + ); + }); + + it('should only pass the first layer for treemap', () => { + const d = { + dataName: 'Second level 1', + depth: 2, + sortIndex: 0, + parent: { + children: [['Second level 1'], ['Second level 2']], + depth: 1, + sortIndex: 0, + parent: { + children: [['First level']], + depth: 0, + sortIndex: 0, + }, + }, + } as unknown as ShapeTreeNode; + const registry = getPaletteRegistry(); + getColor( + ChartTypes.TREEMAP, + d, + 1, + true, + {}, + buckets, + visData.rows, + visParams, + registry, + undefined, + true, + false, + dataMock.fieldFormats, + visData.columns[0], + formatters + ); + expect(registry.get().getCategoricalColor).toHaveBeenCalledWith( + [expect.objectContaining({ name: 'First level' })], + expect.anything(), + expect.anything() + ); + }); +}); diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.test.ts b/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.test.ts index d96ada51ba47a0..7300aac06329fa 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.test.ts +++ b/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.test.ts @@ -5,294 +5,110 @@ * in compliance with, at your election, the Elastic License 2.0 or the Server * Side Public License, v 1. */ -import { ShapeTreeNode } from '@elastic/charts'; -import type { PaletteDefinition, SeriesLayer } from '@kbn/coloring'; -import { dataPluginMock } from '@kbn/data-plugin/public/mocks'; -import { fieldFormatsMock } from '@kbn/field-formats-plugin/common/mocks'; -import type { DataPublicPluginStart } from '@kbn/data-plugin/public'; -import { getColor } from './get_color'; -import { createMockVisData, createMockBucketColumns, createMockPieParams } from '../../mocks'; -import { generateFormatters } from '../formatters'; -import { ChartTypes } from '../../../common/types'; - -const visData = createMockVisData(); -const buckets = createMockBucketColumns(); -const visParams = createMockPieParams(); -const colors = ['color1', 'color2', 'color3', 'color4']; -const dataMock = dataPluginMock.createStartContract(); -interface RangeProps { - gte: number; - lt: number; -} -const defaultFormatter = jest.fn((...args) => fieldFormatsMock.deserialize(...args)); -const formatters = generateFormatters(visData, defaultFormatter); -dataMock.fieldFormats = { - deserialize: jest.fn(() => ({ - convert: jest.fn((s: RangeProps) => { - return `≥ ${s.gte} and < ${s.lt}`; - }), - })), -} as unknown as DataPublicPluginStart['fieldFormats']; +import { ArrayEntry, ArrayNode } from '@elastic/charts'; +import { fieldFormatsMock } from '@kbn/field-formats-plugin/common/mocks'; +import { BucketColumns, ChartTypes } from '../../../common/types'; +import { createMockPieParams, createMockVisData } from '../../mocks'; +import { getPaletteRegistry } from '../../__mocks__/palettes'; +import { getLayers } from './get_layers'; -export const getPaletteRegistry = () => { - const mockPalette1: jest.Mocked = { - id: 'default', - title: 'My Palette', - getCategoricalColor: jest.fn((layer: SeriesLayer[]) => colors[layer[0].rankAtDepth]), - getCategoricalColors: jest.fn((num: number) => colors), - toExpression: jest.fn(() => ({ - type: 'expression', - chain: [ - { - type: 'function', - function: 'system_palette', - arguments: { - name: ['default'], +describe('getLayers', () => { + it('preserves slice order for multi-metric layer', () => { + const visData = createMockVisData(); + const columns: BucketColumns[] = [ + { + id: 'col-0-0', + name: 'Normal column', + meta: { type: 'murmur3' }, + }, + { + id: 'col-0-0', + name: 'multi-metric column', + meta: { + type: 'number', + sourceParams: { + consolidatedMetricsColumn: true, }, }, - ], - })), - }; - - return { - get: () => mockPalette1, - getAll: () => [mockPalette1], - }; -}; - -describe('computeColor', () => { - it('should return the correct color based on the parent sortIndex', () => { - const d = { - dataName: 'ES-Air', - depth: 1, - sortIndex: 0, - parent: { - children: [['ES-Air'], ['Kibana Airlines']], - depth: 0, - sortIndex: 0, }, - } as unknown as ShapeTreeNode; - const color = getColor( + ]; + const visParams = createMockPieParams(); + const layers = getLayers( ChartTypes.PIE, - d, - 0, - false, - {}, - buckets, - visData.rows, + columns, visParams, - getPaletteRegistry(), - { getColor: () => undefined }, - false, - false, - dataMock.fieldFormats, - visData.columns[0], - formatters - ); - expect(color).toEqual(colors[0]); - }); - - it('slices with the same label should have the same color for small multiples', () => { - const d = { - dataName: 'ES-Air', - depth: 1, - sortIndex: 0, - parent: { - children: [['ES-Air'], ['Kibana Airlines']], - depth: 0, - sortIndex: 0, - }, - } as unknown as ShapeTreeNode; - const color = getColor( - ChartTypes.PIE, - d, - 0, - true, + visData, {}, - buckets, - visData.rows, - visParams, - getPaletteRegistry(), - { getColor: () => undefined }, - false, - false, - dataMock.fieldFormats, - visData.columns[0], - formatters - ); - expect(color).toEqual('color3'); - }); - it('returns the overwriteColor if exists', () => { - const d = { - dataName: 'ES-Air', - depth: 1, - sortIndex: 0, - parent: { - children: [['ES-Air'], ['Kibana Airlines']], - depth: 0, - sortIndex: 0, - }, - } as unknown as ShapeTreeNode; - const color = getColor( - ChartTypes.PIE, - d, - 0, - true, - { 'ES-Air': '#000028' }, - buckets, - visData.rows, - visParams, + [], getPaletteRegistry(), - { getColor: () => undefined }, - false, + {}, + fieldFormatsMock, false, - dataMock.fieldFormats, - visData.columns[0], - formatters + false ); - expect(color).toEqual('#000028'); - }); - it('returns the overwriteColor for older visualizations with formatted values', () => { - const d = { - dataName: { - gte: 1000, - lt: 2000, - }, - depth: 1, - sortIndex: 0, - parent: { - children: [ - [ - { - gte: 1000, - lt: 2000, - }, - ], - [ - { - gte: 2000, - lt: 3000, - }, - ], - ], - depth: 0, - sortIndex: 0, - }, - } as unknown as ShapeTreeNode; - const visParamsNew = { - ...visParams, - distinctColors: true, - }; - const column = { - ...visData.columns[0], - format: { - id: 'range', - params: { - id: 'number', - }, - }, - }; - const color = getColor( - ChartTypes.PIE, - d, - 0, - true, - { '≥ 1000 and < 2000': '#3F6833' }, - buckets, - visData.rows, - visParamsNew, - getPaletteRegistry(), - { getColor: () => undefined }, - false, - false, - dataMock.fieldFormats, - column, - formatters - ); - expect(color).toEqual('#3F6833'); - }); + expect(layers[0].sortPredicate).toBeUndefined(); + expect(layers[1].sortPredicate).toBeDefined(); - it('should only pass the second layer for mosaic', () => { - const d = { - dataName: 'Second level 1', - depth: 2, - sortIndex: 0, - parent: { - children: [['Second level 1'], ['Second level 2']], - depth: 1, - sortIndex: 0, - parent: { - children: [['First level']], - depth: 0, - sortIndex: 0, - }, - }, - } as unknown as ShapeTreeNode; - const registry = getPaletteRegistry(); - getColor( - ChartTypes.MOSAIC, - d, - 1, - true, - {}, - buckets, - visData.rows, - visParams, - registry, - undefined, - true, - false, - dataMock.fieldFormats, - visData.columns[0], - formatters - ); - expect(registry.get().getCategoricalColor).toHaveBeenCalledWith( - [expect.objectContaining({ name: 'Second level 1' })], - expect.anything(), - expect.anything() - ); - }); + const testNodes: ArrayEntry[] = [ + [ + '', + { + value: 2, + inputIndex: [3], + } as ArrayNode, + ], + [ + '', + { + value: 1, + inputIndex: [2], + } as ArrayNode, + ], - it('should only pass the first layer for treemap', () => { - const d = { - dataName: 'Second level 1', - depth: 2, - sortIndex: 0, - parent: { - children: [['Second level 1'], ['Second level 2']], - depth: 1, - sortIndex: 0, - parent: { - children: [['First level']], - depth: 0, - sortIndex: 0, - }, - }, - } as unknown as ShapeTreeNode; - const registry = getPaletteRegistry(); - getColor( - ChartTypes.TREEMAP, - d, - 1, - true, - {}, - buckets, - visData.rows, - visParams, - registry, - undefined, - true, - false, - dataMock.fieldFormats, - visData.columns[0], - formatters - ); - expect(registry.get().getCategoricalColor).toHaveBeenCalledWith( - [expect.objectContaining({ name: 'First level' })], - expect.anything(), - expect.anything() - ); + [ + '', + { + value: 3, + inputIndex: [1], + } as ArrayNode, + ], + ]; + + const predicate = layers[1].sortPredicate!; + testNodes.sort(predicate); + + expect(testNodes).toMatchInlineSnapshot(` + Array [ + Array [ + "", + Object { + "inputIndex": Array [ + 1, + ], + "value": 3, + }, + ], + Array [ + "", + Object { + "inputIndex": Array [ + 2, + ], + "value": 1, + }, + ], + Array [ + "", + Object { + "inputIndex": Array [ + 3, + ], + "value": 2, + }, + ], + ] + `); }); }); diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.ts b/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.ts index 646a0e4b45d2e2..93a9cc2b6c7d66 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.ts +++ b/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.ts @@ -12,7 +12,7 @@ import { FieldFormat } from '@kbn/field-formats-plugin/common'; import type { FieldFormatsStart } from '@kbn/field-formats-plugin/public'; import type { Datatable, DatatableRow } from '@kbn/expressions-plugin/public'; import { BucketColumns, ChartTypes, PartitionVisParams } from '../../../common/types'; -import { sortPredicateByType } from './sort_predicate'; +import { sortPredicateByType, sortPredicateSaveSourceOrder } from './sort_predicate'; import { byDataColorPaletteMap, getColor } from './get_color'; import { getNodeLabel } from './get_node_labels'; @@ -52,7 +52,7 @@ export const getLayers = ( ); } - const sortPredicate = sortPredicateByType(chartType, visParams, visData, columns); + const sortPredicateForType = sortPredicateByType(chartType, visParams, visData, columns); return columns.map((col, layerIndex) => { return { groupByRollup: (d: Datum) => (col.id ? d[col.id] ?? EMPTY_SLICE : col.name), @@ -62,7 +62,9 @@ export const getLayers = ( layerIndex === 0 && chartType === ChartTypes.MOSAIC ? { ...fillLabel, minFontSize: 14, maxFontSize: 14, clipText: true } : fillLabel, - sortPredicate, + sortPredicate: col.meta?.sourceParams?.consolidatedMetricsColumn + ? sortPredicateSaveSourceOrder() + : sortPredicateForType, shape: { fillColor: (d) => getColor( diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/sort_predicate.ts b/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/sort_predicate.ts index 0e4564a9bcfe5c..b5992c3b600bf5 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/sort_predicate.ts +++ b/src/plugins/chart_expressions/expression_partition_vis/public/utils/layers/sort_predicate.ts @@ -30,7 +30,7 @@ export const extractUniqTermsMap = (dataTable: Datatable, columnId: string) => {} ); -const sortPredicateSaveSourceOrder: SortPredicatePureFn = +export const sortPredicateSaveSourceOrder: SortPredicatePureFn = () => ([, node1], [, node2]) => { const [index1] = node1.inputIndex ?? []; diff --git a/x-pack/plugins/lens/public/visualizations/partition/to_expression.ts b/x-pack/plugins/lens/public/visualizations/partition/to_expression.ts index 188ddfccea0ba3..94438f242ec8d9 100644 --- a/x-pack/plugins/lens/public/visualizations/partition/to_expression.ts +++ b/x-pack/plugins/lens/public/visualizations/partition/to_expression.ts @@ -79,10 +79,10 @@ export const getColumnToLabelMap = ( return columnToLabel; }; -export const getSortedGroups = ( +export const getSortedAccessorsForGroup = ( datasource: DatasourcePublicAPI | undefined, layer: PieLayerState, - accessor: 'primaryGroups' | 'secondaryGroups' = 'primaryGroups' + accessor: 'primaryGroups' | 'secondaryGroups' | 'metrics' ) => { const originalOrder = datasource ?.getTableSpec() @@ -174,7 +174,9 @@ const generateCommonArguments = ( datasourceLayers: DatasourceLayers, paletteService: PaletteRegistry ) => { - const columnToLabelMap = getColumnToLabelMap(layer.metrics, datasourceLayers[layer.layerId]); + const datasource = datasourceLayers[layer.layerId]; + const columnToLabelMap = getColumnToLabelMap(layer.metrics, datasource); + const sortedMetricAccessors = getSortedAccessorsForGroup(datasource, layer, 'metrics'); return { labels: generateCommonLabelsAstArgs(state, attributes, layer, columnToLabelMap), @@ -182,7 +184,7 @@ const generateCommonArguments = ( .filter(({ columnId }) => !isCollapsed(columnId, layer)) .map(({ columnId }) => columnId) .map(prepareDimension), - metrics: (layer.allowMultipleMetrics ? layer.metrics : [layer.metrics[0]]).map( + metrics: (layer.allowMultipleMetrics ? sortedMetricAccessors : [sortedMetricAccessors[0]]).map( prepareDimension ), metricsToLabels: JSON.stringify(columnToLabelMap), @@ -290,16 +292,18 @@ function expressionHelper( const layer = state.layers[0]; const datasource = datasourceLayers[layer.layerId]; - const groups = Array.from( + const accessors = Array.from( new Set( [ - getSortedGroups(datasource, layer, 'primaryGroups'), - layer.secondaryGroups ? getSortedGroups(datasource, layer, 'secondaryGroups') : [], + getSortedAccessorsForGroup(datasource, layer, 'primaryGroups'), + layer.secondaryGroups + ? getSortedAccessorsForGroup(datasource, layer, 'secondaryGroups') + : [], ].flat() ) ); - const operations = groups + const operations = accessors .map((columnId) => ({ columnId, operation: datasource?.getOperationForColumnId(columnId) as Operation | null, @@ -323,11 +327,11 @@ function expressionHelper( type: 'expression', chain: [ ...(datasourceAst ? datasourceAst.chain : []), - ...groups + ...accessors .filter((columnId) => layer.collapseFns?.[columnId]) .map((columnId) => { return buildExpressionFunction('lens_collapse', { - by: groups.filter((chk) => chk !== columnId), + by: accessors.filter((chk) => chk !== columnId), metric: layer.metrics, fn: [layer.collapseFns![columnId]!], }).toAst(); diff --git a/x-pack/plugins/lens/public/visualizations/partition/visualization.test.ts b/x-pack/plugins/lens/public/visualizations/partition/visualization.test.ts index 2bf830c0028cd8..a11c1667f807f7 100644 --- a/x-pack/plugins/lens/public/visualizations/partition/visualization.test.ts +++ b/x-pack/plugins/lens/public/visualizations/partition/visualization.test.ts @@ -448,119 +448,173 @@ describe('pie_visualization', () => { }); }); - it("doesn't count collapsed columns toward the dimension limits", () => { - const colIds = new Array(PartitionChartsMeta.pie.maxBuckets) - .fill(undefined) - .map((_, i) => String(i + 1)); - - const frame = mockFrame(); - frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => - colIds.map((id) => ({ columnId: id, fields: [] })); + it('orders metric accessors by datasource column order', () => { + const colIds = ['1', '2', '3', '4']; const state = getExampleState(); - state.layers[0].primaryGroups = colIds; - - const getConfig = (_state: PieVisualizationState) => - pieVisualization.getConfiguration({ - state: _state, - frame, - layerId: state.layers[0].layerId, - }); - - expect(findPrimaryGroup(getConfig(state))?.supportsMoreColumns).toBeFalsy(); + state.layers[0].metrics = colIds; + state.layers[0].allowMultipleMetrics = true; - const stateWithCollapsed = cloneDeep(state); - stateWithCollapsed.layers[0].collapseFns = { '1': 'sum' }; + const frame = mockFrame(); + frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => + // reverse the column IDs in the datasource + colIds.reverse().map((id) => ({ columnId: id, fields: [] })); + + // this is to make sure the accessors get sorted before palette colors are applied + const palette = paletteServiceMock.get('default'); + palette.getCategoricalColor + .mockReturnValueOnce('color 1') + .mockReturnValueOnce('color 2') + .mockReturnValueOnce('color 3') + .mockReturnValueOnce('color 4'); + + const config = pieVisualization.getConfiguration({ + state, + frame, + layerId: state.layers[0].layerId, + }); - expect(findPrimaryGroup(getConfig(stateWithCollapsed))?.supportsMoreColumns).toBeTruthy(); + expect(findMetricGroup(config)?.accessors).toMatchInlineSnapshot(` + Array [ + Object { + "color": "color 1", + "columnId": "4", + "triggerIconType": "color", + }, + Object { + "color": "color 2", + "columnId": "3", + "triggerIconType": "color", + }, + Object { + "color": "color 3", + "columnId": "2", + "triggerIconType": "color", + }, + Object { + "color": "color 4", + "columnId": "1", + "triggerIconType": "color", + }, + ] + `); }); - it('counts multiple metrics toward the dimension limits when not mosaic', () => { - const colIds = new Array(PartitionChartsMeta.pie.maxBuckets - 1) - .fill(undefined) - .map((_, i) => String(i + 1)); + describe('dimension limits', () => { + it("doesn't count collapsed columns toward the dimension limits", () => { + const colIds = new Array(PartitionChartsMeta.pie.maxBuckets) + .fill(undefined) + .map((_, i) => String(i + 1)); - const frame = mockFrame(); - frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => - colIds.map((id) => ({ columnId: id, fields: [] })); + const frame = mockFrame(); + frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => + colIds.map((id) => ({ columnId: id, fields: [] })); - const state = getExampleState(); - state.layers[0].primaryGroups = colIds; - state.layers[0].allowMultipleMetrics = true; + const state = getExampleState(); + state.layers[0].primaryGroups = colIds; - const getConfig = (_state: PieVisualizationState) => - pieVisualization.getConfiguration({ - state: _state, - frame, - layerId: state.layers[0].layerId, - }); + const getConfig = (_state: PieVisualizationState) => + pieVisualization.getConfiguration({ + state: _state, + frame, + layerId: state.layers[0].layerId, + }); - expect(findPrimaryGroup(getConfig(state))?.supportsMoreColumns).toBeTruthy(); + expect(findPrimaryGroup(getConfig(state))?.supportsMoreColumns).toBeFalsy(); - const stateWithMultipleMetrics = cloneDeep(state); - stateWithMultipleMetrics.layers[0].metrics.push('1', '2'); + const stateWithCollapsed = cloneDeep(state); + stateWithCollapsed.layers[0].collapseFns = { '1': 'sum' }; - expect( - findPrimaryGroup(getConfig(stateWithMultipleMetrics))?.supportsMoreColumns - ).toBeFalsy(); - }); + expect(findPrimaryGroup(getConfig(stateWithCollapsed))?.supportsMoreColumns).toBeTruthy(); + }); - it('does NOT count multiple metrics toward the dimension limits when mosaic', () => { - const frame = mockFrame(); - frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => []; + it('counts multiple metrics toward the dimension limits when not mosaic', () => { + const colIds = new Array(PartitionChartsMeta.pie.maxBuckets - 1) + .fill(undefined) + .map((_, i) => String(i + 1)); - const state = getExampleState(); - state.shape = 'mosaic'; - state.layers[0].primaryGroups = []; - state.layers[0].allowMultipleMetrics = false; // always true for mosaic + const frame = mockFrame(); + frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => + colIds.map((id) => ({ columnId: id, fields: [] })); - const getConfig = (_state: PieVisualizationState) => - pieVisualization.getConfiguration({ - state: _state, - frame, - layerId: state.layers[0].layerId, - }); + const state = getExampleState(); + state.layers[0].primaryGroups = colIds; + state.layers[0].allowMultipleMetrics = true; - expect(findPrimaryGroup(getConfig(state))?.supportsMoreColumns).toBeTruthy(); + const getConfig = (_state: PieVisualizationState) => + pieVisualization.getConfiguration({ + state: _state, + frame, + layerId: state.layers[0].layerId, + }); - const stateWithMultipleMetrics = cloneDeep(state); - stateWithMultipleMetrics.layers[0].metrics.push('1', '2'); + expect(findPrimaryGroup(getConfig(state))?.supportsMoreColumns).toBeTruthy(); - expect( - findPrimaryGroup(getConfig(stateWithMultipleMetrics))?.supportsMoreColumns - ).toBeTruthy(); - }); + const stateWithMultipleMetrics = cloneDeep(state); + stateWithMultipleMetrics.layers[0].metrics.push('1', '2'); - it('reports too many metric dimensions if multiple not enabled', () => { - const colIds = ['1', '2', '3', '4']; + expect( + findPrimaryGroup(getConfig(stateWithMultipleMetrics))?.supportsMoreColumns + ).toBeFalsy(); + }); - const frame = mockFrame(); - frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => - colIds.map((id) => ({ columnId: id, fields: [] })); + it('does NOT count multiple metrics toward the dimension limits when mosaic', () => { + const frame = mockFrame(); + frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => []; - const state = getExampleState(); - state.layers[0].metrics = colIds; - state.layers[0].allowMultipleMetrics = false; - expect( - findMetricGroup( - pieVisualization.getConfiguration({ - state, - frame, - layerId: state.layers[0].layerId, - }) - )?.dimensionsTooMany - ).toBe(3); + const state = getExampleState(); + state.shape = 'mosaic'; + state.layers[0].primaryGroups = []; + state.layers[0].allowMultipleMetrics = false; // always true for mosaic - state.layers[0].allowMultipleMetrics = true; - expect( - findMetricGroup( + const getConfig = (_state: PieVisualizationState) => pieVisualization.getConfiguration({ - state, + state: _state, frame, layerId: state.layers[0].layerId, - }) - )?.dimensionsTooMany - ).toBe(0); + }); + + expect(findPrimaryGroup(getConfig(state))?.supportsMoreColumns).toBeTruthy(); + + const stateWithMultipleMetrics = cloneDeep(state); + stateWithMultipleMetrics.layers[0].metrics.push('1', '2'); + + expect( + findPrimaryGroup(getConfig(stateWithMultipleMetrics))?.supportsMoreColumns + ).toBeTruthy(); + }); + + it('reports too many metric dimensions if multiple not enabled', () => { + const colIds = ['1', '2', '3', '4']; + + const frame = mockFrame(); + frame.datasourceLayers[LAYER_ID]!.getTableSpec = () => + colIds.map((id) => ({ columnId: id, fields: [] })); + + const state = getExampleState(); + state.layers[0].metrics = colIds; + state.layers[0].allowMultipleMetrics = false; + expect( + findMetricGroup( + pieVisualization.getConfiguration({ + state, + frame, + layerId: state.layers[0].layerId, + }) + )?.dimensionsTooMany + ).toBe(3); + + state.layers[0].allowMultipleMetrics = true; + expect( + findMetricGroup( + pieVisualization.getConfiguration({ + state, + frame, + layerId: state.layers[0].layerId, + }) + )?.dimensionsTooMany + ).toBe(0); + }); }); it.each(Object.values(PieChartTypes).filter((type) => type !== 'mosaic'))( diff --git a/x-pack/plugins/lens/public/visualizations/partition/visualization.tsx b/x-pack/plugins/lens/public/visualizations/partition/visualization.tsx index cedfb12f72df79..a5c218a6483e19 100644 --- a/x-pack/plugins/lens/public/visualizations/partition/visualization.tsx +++ b/x-pack/plugins/lens/public/visualizations/partition/visualization.tsx @@ -29,7 +29,7 @@ import type { } from '../../types'; import { getColumnToLabelMap, - getSortedGroups, + getSortedAccessorsForGroup, toExpression, toPreviewExpression, } from './to_expression'; @@ -91,12 +91,13 @@ export const getDefaultColorForMultiMetricDimension = ({ datasource: DatasourcePublicAPI | undefined; }) => { const columnToLabelMap = datasource ? getColumnToLabelMap(layer.metrics, datasource) : {}; + const sortedMetrics = getSortedAccessorsForGroup(datasource, layer, 'metrics'); return paletteService.get('default').getCategoricalColor([ { name: columnToLabelMap[columnId], - rankAtDepth: layer.metrics.indexOf(columnId), - totalSeriesAtDepth: layer.metrics.length, + rankAtDepth: sortedMetrics.indexOf(columnId), + totalSeriesAtDepth: sortedMetrics.length, }, ]) as string; }; @@ -167,7 +168,7 @@ export const getPieVisualization = ({ const datasource = frame.datasourceLayers[layer.layerId]; const getPrimaryGroupConfig = (): VisualizationDimensionGroupConfig => { - const originalOrder = getSortedGroups(datasource, layer); + const originalOrder = getSortedAccessorsForGroup(datasource, layer, 'primaryGroups'); // When we add a column it could be empty, and therefore have no order const accessors = originalOrder.map((accessor) => ({ columnId: accessor, @@ -273,7 +274,11 @@ export const getPieVisualization = ({ }; const getSecondaryGroupConfig = (): VisualizationDimensionGroupConfig | undefined => { - const originalSecondaryOrder = getSortedGroups(datasource, layer, 'secondaryGroups'); + const originalSecondaryOrder = getSortedAccessorsForGroup( + datasource, + layer, + 'secondaryGroups' + ); const accessors = originalSecondaryOrder.map((accessor) => ({ columnId: accessor, triggerIconType: isCollapsed(accessor, layer) ? 'aggregate' : undefined, @@ -317,7 +322,11 @@ export const getPieVisualization = ({ const getMetricGroupConfig = (): VisualizationDimensionGroupConfig => { const hasSliceBy = layer.primaryGroups.length + (layer.secondaryGroups?.length ?? 0); - const accessors: AccessorConfig[] = layer.metrics.map((columnId, index) => ({ + const accessors: AccessorConfig[] = getSortedAccessorsForGroup( + datasource, + layer, + 'metrics' + ).map((columnId) => ({ columnId, ...(layer.allowMultipleMetrics ? hasSliceBy @@ -371,7 +380,7 @@ export const getPieVisualization = ({ }; }, - setDimension({ prevState, layerId, columnId, groupId }) { + setDimension({ prevState, layerId, columnId, groupId, previousColumn }) { return { ...prevState, layers: prevState.layers.map((l) => { @@ -393,7 +402,8 @@ export const getPieVisualization = ({ ], }; } - return { ...l, metrics: [...l.metrics.filter((metric) => metric !== columnId), columnId] }; + const metrics = [...l.metrics.filter((metric) => metric !== columnId), columnId]; + return { ...l, metrics }; }), }; },