This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch adopt/37712-x-axis-sort-with-dimensions in repository https://gitbox.apache.org/repos/asf/superset.git
commit 15361212dd5883dc2f445e0d1e164a999a6c7702 Author: rusackas <[email protected]> AuthorDate: Tue Sep 22 17:42:12 2026 -0700 fix(echarts-bar): sort the x-axis by any column or metric when dimensions are set The "X-Axis Sort By" control on ECharts bar charts offered a different set of choices once a dimension was added (only the series aggregates), and even a metric that was selected before adding the dimension was silently ignored by the multi-series render path, which only understood `SortSeriesType`. Carry the chosen field through the grouped sort path instead of guessing an aggregate for it: - the control offers the x-axis column and every metric (the query "Sort By" metric included) with or without dimensions, adding the series aggregates for multi-series charts and deduping by value; - the sort-only limit metric is queried with dimensions too, so the pivot returns one `<metric>, <dimension values>` column per series; - transformProps resolves a metric's pivoted columns through the response's `label_map` and sorts the rows by their sum (`sortRows` gains a typed `{ sumOfColumns }` mode), sorts by axis value for the x-axis column, and keeps the aggregate behaviour for `SortSeriesType` values; the same lookup keeps the sort-only metric's columns out of the rendered series and the stacked totals. Adopted from #37712. Fixes #34352 Fixes #38249 Co-authored-by: Yousuf Ansari <[email protected]> Co-Authored-By: Evan Rusackas <[email protected]> Co-Authored-By: Claude Fable 5.1 <[email protected]> --- .../src/operators/utils/extractExtraMetrics.ts | 10 +- .../src/shared-controls/customControls.tsx | 59 +++-- .../operators/utils/extractExtraMetrics.test.ts | 13 +- .../test/shared-controls/xAxisSortControl.test.ts | 161 +++++++++++++ .../src/Timeseries/transformProps.ts | 61 ++++- .../plugin-chart-echarts/src/utils/series.ts | 43 +++- .../test/Timeseries/buildQuery.test.ts | 43 ++++ .../Timeseries/xAxisSortWithDimensions.test.ts | 250 +++++++++++++++++++++ .../plugin-chart-echarts/test/utils/series.test.ts | 91 ++++++++ 9 files changed, 694 insertions(+), 37 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts index 4344d088af3..35de84de593 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts @@ -26,11 +26,15 @@ import { export function extractExtraMetrics( formData: QueryFormData, ): QueryFormMetric[] { - const { groupby, timeseries_limit_metric, x_axis_sort, metrics } = formData; + const { timeseries_limit_metric, x_axis_sort, metrics } = formData; const extra_metrics: QueryFormMetric[] = []; - const limitMetric = ensureIsArray(timeseries_limit_metric)[0]; + const [limitMetric] = ensureIsArray(timeseries_limit_metric); + // The "Sort By" limit metric is only queried when the axis is sorted by it + // and it is not already a value metric. This applies with dimensions too: + // the pivot then yields one `<limit metric>, <dimension values>` column + // per series, which the chart sums client-side to order the axis (the + // backend sort operator only handles the single-series case). if ( - !(groupby || []).length && limitMetric && getMetricLabel(limitMetric) === x_axis_sort && !metrics?.some(metric => getMetricLabel(metric) === x_axis_sort) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx index 3604b099359..929ac839800 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx @@ -146,9 +146,7 @@ export const xAxisSortControl = { const columns = [controls?.x_axis?.value as QueryFormColumn].filter( Boolean, ); - const isSingleSortAvailable = - ensureIsArray(controls?.groupby?.value).length === 0; - const isMultiSortAvailable = + const isMultiSeries = !!ensureIsArray(controls?.groupby?.value).length || ensureIsArray(controls?.metrics?.value).length > 1; const metrics = [ @@ -156,25 +154,44 @@ export const xAxisSortControl = { controls?.timeseries_limit_metric?.value as QueryFormMetric, ].filter(Boolean); const metricLabels = [...new Set(metrics.map(getMetricLabel))]; + // The x-axis column and every metric (the "Sort By" limit metric + // included) can order the axis with or without dimensions: the backend + // sort operator handles the single-series case, and the chart resolves + // a metric's pivoted columns through `label_map` when dimensions split + // it into several series. + const fieldOptions = [ + ...columns.map(column => { + const value = getColumnLabel(column); + return { value, label: dataset?.verbose_map?.[value] || value }; + }), + ...metricLabels.map(value => ({ + value, + label: dataset?.verbose_map?.[value] || value, + })), + ]; + // Aggregating across series only means something once there is more + // than one series per x-axis value. + const aggregateOptions = isMultiSeries + ? SORT_SERIES_CHOICES.map(choice => ({ + value: choice[0], + label: choice[1], + })) + : []; + // Option values double as the stored `x_axis_sort`, and the aggregate + // values (`name`, `sum`, ...) are what the chart matches first, so a + // column or metric whose label collides with one is dropped rather + // than shadowing the aggregate. The same set also dedupes a column + // that shares its label with a metric. + const seenValues = new Set(aggregateOptions.map(option => option.value)); const options = [ - ...(isSingleSortAvailable - ? [ - ...columns.map(column => { - const value = getColumnLabel(column); - return { value, label: dataset?.verbose_map?.[value] || value }; - }), - ...metricLabels.map(value => ({ - value, - label: dataset?.verbose_map?.[value] || value, - })), - ] - : []), - ...(isMultiSortAvailable - ? SORT_SERIES_CHOICES.map(choice => ({ - value: choice[0], - label: choice[1], - })) - : []), + ...fieldOptions.filter(option => { + if (seenValues.has(option.value)) { + return false; + } + seenValues.add(option.value); + return true; + }), + ...aggregateOptions, ]; const shouldReset = !( diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/extractExtraMetrics.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/extractExtraMetrics.test.ts index dd2a10cbe55..7832bc684e1 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/extractExtraMetrics.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/extractExtraMetrics.test.ts @@ -82,7 +82,7 @@ test('handles adhoc metrics correctly', () => { ).toEqual([]); }); -test('returns empty array if groupby populated', () => { +test('returns the limit metric when groupby is populated so the chart can sort by its pivoted columns', () => { expect( extractExtraMetrics({ ...baseFormData, @@ -90,6 +90,17 @@ test('returns empty array if groupby populated', () => { timeseries_limit_metric: 'foo', x_axis_sort: 'foo', }), + ).toEqual(['foo']); +}); + +test('returns empty array with groupby when the axis is sorted by a series aggregate', () => { + expect( + extractExtraMetrics({ + ...baseFormData, + groupby: ['bar'], + timeseries_limit_metric: 'foo', + x_axis_sort: 'sum', + }), ).toEqual([]); }); diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts new file mode 100644 index 00000000000..ac967c3975b --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts @@ -0,0 +1,161 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import type { QueryFormMetric } from '@superset-ui/core'; +import { xAxisSortControl } from '../../src/shared-controls/customControls'; +import { SORT_SERIES_CHOICES } from '../../src/constants'; +import { SortSeriesType } from '../../src/types'; +import type { + ControlPanelState, + ControlState, + ControlStateMapping, + Dataset, +} from '../../src/types'; + +const naSales: QueryFormMetric = { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'na_sales' }, + label: 'SUM(na_sales)', +}; + +const dataset = { + id: 1, + type: 'table', + columns: [ + { column_name: 'genre', type: 'STRING' }, + { column_name: 'platform', type: 'STRING' }, + { column_name: 'na_sales', type: 'FLOAT' }, + { column_name: 'sum', type: 'STRING' }, + ], + metrics: [{ metric_name: 'count', expression: 'COUNT(*)' }], + verbose_map: { + genre: 'Genre', + count: 'Count', + 'SUM(na_sales)': 'NA Sales', + }, +} as unknown as Dataset; + +const aggregateOptions = SORT_SERIES_CHOICES.map(([value, label]) => ({ + value, + label, +})); + +function mapStateToProps( + controlValues: Record<string, unknown>, + value?: string, +) { + const controls = Object.fromEntries( + Object.entries(controlValues).map(([name, controlValue]) => [ + name, + { value: controlValue }, + ]), + ); + const state = { + controls: { + ...controls, + datasource: { datasource: dataset }, + } as unknown as ControlStateMapping, + datasource: dataset, + } as unknown as ControlPanelState; + const controlState = { + value, + type: 'XAxisSortControl', + } as unknown as ControlState; + return xAxisSortControl.config.mapStateToProps(state, controlState); +} + +const singleSeriesControls = { + x_axis: 'genre', + metrics: ['count'], + timeseries_limit_metric: naSales, + groupby: [], +}; + +const fieldOptions = [ + { value: 'genre', label: 'Genre' }, + { value: 'count', label: 'Count' }, + { value: 'SUM(na_sales)', label: 'NA Sales' }, +]; + +test('offers the x-axis column and every metric with verbose labels when there are no dimensions', () => { + expect(mapStateToProps(singleSeriesControls).options).toEqual(fieldOptions); +}); + +test('keeps the same field options and adds the series aggregates when a dimension is set', () => { + expect( + mapStateToProps({ ...singleSeriesControls, groupby: ['platform'] }).options, + ).toEqual([...fieldOptions, ...aggregateOptions]); +}); + +test('adds the series aggregates when there are several metrics and no dimension', () => { + expect( + mapStateToProps({ ...singleSeriesControls, metrics: ['count', naSales] }) + .options, + ).toEqual([...fieldOptions, ...aggregateOptions]); +}); + +test('drops a field whose label collides with a series aggregate value', () => { + const { options } = mapStateToProps({ + ...singleSeriesControls, + x_axis: 'sum', + groupby: ['platform'], + }); + expect(options.filter(option => option.value === 'sum')).toEqual([ + { value: SortSeriesType.Sum, label: 'Total value' }, + ]); +}); + +test('offers a field named like a series aggregate when there is nothing to collide with', () => { + const { options } = mapStateToProps({ + ...singleSeriesControls, + x_axis: 'sum', + }); + expect(options.filter(option => option.value === 'sum')).toEqual([ + { value: 'sum', label: 'sum' }, + ]); +}); + +test('keeps a metric selection when a dimension is added', () => { + const { shouldReset } = mapStateToProps( + { ...singleSeriesControls, groupby: ['platform'] }, + 'SUM(na_sales)', + ); + expect(shouldReset).toBe(false); +}); + +test('keeps a series aggregate selection when a dimension is set', () => { + const { shouldReset } = mapStateToProps( + { ...singleSeriesControls, groupby: ['platform'] }, + SortSeriesType.Max, + ); + expect(shouldReset).toBe(false); +}); + +test('resets a series aggregate selection once the dimension is removed', () => { + const { shouldReset } = mapStateToProps( + singleSeriesControls, + SortSeriesType.Max, + ); + expect(shouldReset).toBe(true); +}); + +test('resets a selection that is no longer offered', () => { + const { shouldReset } = mapStateToProps(singleSeriesControls, 'platform'); + expect(shouldReset).toBe(true); +}); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 276db9e6325..48a206773b6 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -55,6 +55,7 @@ import { getOriginalSeries, getTimeOffset, isDerivedSeries, + SortSeriesType, } from '@superset-ui/chart-controls'; import type { EChartsCoreOption } from 'echarts/core'; import type { @@ -96,6 +97,7 @@ import { getMinAndMaxFromBounds, getTemporalAxisTickConfig, resolveTemporalTickValues, + XAxisSortSeries, } from '../utils/series'; import { resolveLegendLayout } from '../utils/legendLayout'; import { @@ -460,14 +462,34 @@ export default function transformProps( rebaseToPercentChange(forecastRebasedData, xAxisLabel || DTTM_ALIAS) : forecastRebasedData; const isHorizontal = orientation === OrientationType.Horizontal; + // With dimensions set, the pivot splits a metric into one + // `<metric>, <dimension values>` column per series. `label_map` lists each + // flattened column as `[metric, ...dimension values]`, so a metric's + // columns are resolved through it rather than by matching on the column + // name. A single-metric chart with `truncate_metric` drops the metric part + // from its value columns, which is fine: only sort-only metrics need + // resolving, and those always keep it. + const pivotedColumnsOf = (metricLabel: string): string[] => + Object.entries(labelMap) + .filter( + ([column, parts]) => + column !== metricLabel && + parts.length > 1 && + parts[0] === metricLabel && + !derivedComparisonSeries.has(column), + ) + .map(([column]) => column); // rebasedData's keys have already been through rebaseForecastDatum, which // renames a key to its verboseMap entry when one is configured for that // metric. extraMetricLabels must be mapped the same way, or a sort-only // metric with a verbose_name set would silently fail to match here (and in - // extractSeries below, which has the same requirement). + // extractSeries below, which has the same requirement). The pivoted columns + // keep their raw names (verbose mapping only applies to an exact metric + // label), and must be excluded too so a sort-only metric is neither + // rendered as series nor counted in stacked totals. const extraMetricLabels = extractExtraMetrics(chartProps.rawFormData) .map(getMetricLabel) - .map(label => verboseMap[label] ?? label); + .flatMap(label => [verboseMap[label] ?? label, ...pivotedColumnsOf(label)]); const { totalStackedValues, thresholdValues } = extractDataTotalValues( rebasedData, { @@ -480,6 +502,35 @@ export default function transformProps( ); const isMultiSeries = groupBy.length || metrics?.length > 1; + // `x_axis_sort` stores either a series aggregate (a `SortSeriesType`) or + // the label of the x-axis column or of a metric. A single-series chart is + // sorted by the backend sort operator; with several series the rows are + // ordered here instead: by axis value, by an aggregate over every series, + // or by the sum of the chosen metric's columns. The field is carried + // through as-is, so a column or metric that happens to be named like an + // aggregate is never guessed at. + const sortSeriesTypes = new Set<string>(Object.values(SortSeriesType)); + const resolveXAxisSortSeries = (): XAxisSortSeries | undefined => { + if (!isMultiSeries || typeof xAxisSort !== 'string') { + return undefined; + } + if (sortSeriesTypes.has(xAxisSort)) { + return xAxisSort as SortSeriesType; + } + if ( + xAxisSort === xAxisLabel || + xAxisSort === getXAxisLabel(chartProps.rawFormData) + ) { + return SortSeriesType.Name; + } + const dataColumns = new Set(Object.keys(rebasedData[0] ?? {})); + const sumOfColumns = [ + verboseMap[xAxisSort] ?? xAxisSort, + ...pivotedColumnsOf(xAxisSort), + ].filter(column => dataColumns.has(column)); + return sumOfColumns.length ? { sumOfColumns } : undefined; + }; + const xAxisSortSeries = resolveXAxisSortSeries(); const xAxisDataType = dataTypes?.[xAxisLabel] ?? dataTypes?.[xAxisOrig]; const xAxisType = getAxisType( stack, @@ -499,8 +550,10 @@ export default function transformProps( isHorizontal, sortSeriesType, sortSeriesAscending, - xAxisSortSeries: isMultiSeries ? xAxisSort : undefined, - xAxisSortSeriesAscending: isMultiSeries ? xAxisSortAsc : undefined, + xAxisSortSeries, + xAxisSortSeriesAscending: isDefined(xAxisSortSeries) + ? xAxisSortAsc + : undefined, xAxisType, }, ); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts index 6ededf12ac3..95789295a23 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts @@ -519,13 +519,39 @@ export function sortAndFilterSeries( ).map(({ name }) => name); } +/** + * Sum of a specific set of columns, used to order the rows of a multi-series + * chart by one metric once dimensions have pivoted that metric into a column + * per series. + */ +export type XAxisSortBySumOfColumns = { sumOfColumns: string[] }; + +/** + * How the rows (one per x-axis value) of a multi-series chart are ordered: + * a `SortSeriesType` aggregates every numeric column in the row, while + * `XAxisSortBySumOfColumns` only sums the named columns. + */ +export type XAxisSortSeries = SortSeriesType | XAxisSortBySumOfColumns; + +export function isXAxisSortBySumOfColumns( + sort: XAxisSortSeries, +): sort is XAxisSortBySumOfColumns { + return typeof sort === 'object'; +} + export function sortRows( rows: DataRecord[], totalStackedValues: number[], xAxis: string, - xAxisSortSeries: SortSeriesType, + xAxisSortSeries: XAxisSortSeries, xAxisSortSeriesAscending: boolean, ) { + const sumOfColumns = isXAxisSortBySumOfColumns(xAxisSortSeries) + ? new Set(xAxisSortSeries.sumOfColumns) + : undefined; + const aggregation: SortSeriesType = sumOfColumns + ? SortSeriesType.Sum + : (xAxisSortSeries as SortSeriesType); const sortedRows = rows.map((row, idx) => { let sortKey: DataRecordValue = ''; let aggregate: number | undefined; @@ -536,17 +562,18 @@ export function sortRows( sortKey = value; } if ( - xAxisSortSeries === SortSeriesType.Name || - typeof value !== 'number' + aggregation === SortSeriesType.Name || + typeof value !== 'number' || + (sumOfColumns && !sumOfColumns.has(key)) ) { return; } - if (!(xAxisSortSeries === SortSeriesType.Avg && !isValueDefined)) { + if (!(aggregation === SortSeriesType.Avg && !isValueDefined)) { entries += 1; } - switch (xAxisSortSeries) { + switch (aggregation) { case SortSeriesType.Avg: case SortSeriesType.Sum: if (aggregate === undefined) { @@ -572,7 +599,7 @@ export function sortRows( } }); if ( - xAxisSortSeries === SortSeriesType.Avg && + aggregation === SortSeriesType.Avg && entries > 0 && aggregate !== undefined ) { @@ -580,7 +607,7 @@ export function sortRows( } const value = - xAxisSortSeries === SortSeriesType.Name + aggregation === SortSeriesType.Name ? typeof sortKey === 'string' ? sortKey.toLowerCase() : sortKey @@ -613,7 +640,7 @@ export function extractSeries( isHorizontal?: boolean; sortSeriesType?: SortSeriesType; sortSeriesAscending?: boolean; - xAxisSortSeries?: SortSeriesType; + xAxisSortSeries?: XAxisSortSeries; xAxisSortSeriesAscending?: boolean; xAxisType?: AxisType; } = {}, diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts index 4c3cf309e95..5138a251868 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts @@ -64,6 +64,49 @@ describe('Timeseries buildQuery', () => { expect(query.metrics).toEqual(['bar', 'baz']); }); + test('should query the sort-only limit metric with dimensions so its pivoted columns can order the axis', () => { + const queryContext = buildQuery({ + ...formData, + metrics: ['count'], + x_axis: 'genre', + groupby: ['platform'], + timeseries_limit_metric: 'na_sales', + x_axis_sort: 'na_sales', + x_axis_sort_asc: false, + }); + const [query] = queryContext.queries; + expect(query.metrics).toEqual(['count', 'na_sales']); + const pivot = (query.post_processing || []).find( + op => op?.operation === 'pivot', + ); + expect(pivot?.options).toMatchObject({ + index: ['genre'], + columns: ['platform'], + aggregates: { + count: { operator: 'mean' }, + na_sales: { operator: 'mean' }, + }, + }); + // With dimensions the chart sorts the pivoted rows itself. + expect( + (query.post_processing || []).map(op => op?.operation), + ).not.toContain('sort'); + }); + + test('should not query the limit metric with dimensions when the axis is sorted by a series aggregate', () => { + const queryContext = buildQuery({ + ...formData, + metrics: ['count'], + x_axis: 'genre', + groupby: ['platform'], + timeseries_limit_metric: 'na_sales', + x_axis_sort: 'sum', + x_axis_sort_asc: false, + }); + const [query] = queryContext.queries; + expect(query.metrics).toEqual(['count']); + }); + test('should apply contribution before rename with time comparison', () => { // rename strips the `__<offset>` suffix that contribution relies on to // compute each time shift separately diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts new file mode 100644 index 00000000000..ebc0c4cae50 --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts @@ -0,0 +1,250 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + ChartDataResponseResult, + DataRecord, + QueryFormMetric, +} from '@superset-ui/core'; +import { SortSeriesType } from '@superset-ui/chart-controls'; +import transformProps from '../../src/Timeseries/transformProps'; +import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants'; +import { + EchartsTimeseriesChartProps, + EchartsTimeseriesFormData, + EchartsTimeseriesSeriesType, +} from '../../src/Timeseries/types'; +import { createEchartsTimeseriesTestChartProps } from '../helpers'; + +// A bar chart of COUNT(*) per genre, split by platform, with the axis sorted +// by a metric that is neither the value metric nor a dimension (#34352). +// The rows are what the backend returns for that query: the value metric's +// columns lose their metric label (`truncate_metric`), the sort-only metric +// keeps it, and `label_map` records the structure behind each column. +const naSales: QueryFormMetric = { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'na_sales' }, + label: 'SUM(na_sales)', +}; + +const rows: DataRecord[] = [ + { + genre: 'Action', + PS4: 30, + XOne: 20, + 'SUM(na_sales), PS4': 5, + 'SUM(na_sales), XOne': 3, + }, + { + genre: 'Puzzle', + PS4: 5, + XOne: 5, + 'SUM(na_sales), PS4': 20, + 'SUM(na_sales), XOne': 10, + }, + { + genre: 'Sports', + PS4: 40, + XOne: 10, + 'SUM(na_sales), PS4': 1, + 'SUM(na_sales), XOne': 1, + }, +]; + +const labelMap = { + genre: ['genre'], + PS4: ['PS4'], + XOne: ['XOne'], + 'SUM(na_sales), PS4': ['SUM(na_sales)', 'PS4'], + 'SUM(na_sales), XOne': ['SUM(na_sales)', 'XOne'], +}; + +const queriesData = [ + { + annotation_data: null, + cache_key: null, + cache_timeout: null, + cached_dttm: null, + queried_dttm: null, + data: rows, + colnames: [], + coltypes: [], + error: null, + is_cached: false, + query: '', + rowcount: rows.length, + sql_rowcount: rows.length, + stacktrace: null, + status: 'success', + from_dttm: null, + to_dttm: null, + label_map: labelMap, + } as unknown as ChartDataResponseResult, +]; + +type Series = { + name: string; + data: [string, number][]; + label: { + formatter: (params: { + value: [string, number]; + dataIndex: number; + seriesIndex: number; + }) => string; + }; +}; + +// transformProps reads camelCase keys from `formData` and snake_case keys +// from `rawFormData`; the test helper hands the same object to both. +function transform( + overrides: Record<string, unknown>, + verboseMap = {}, + data = rows, +) { + const chartProps = createEchartsTimeseriesTestChartProps< + EchartsTimeseriesFormData, + EchartsTimeseriesChartProps + >({ + defaultFormData: DEFAULT_FORM_DATA, + defaultVizType: 'echarts_timeseries_bar', + formData: { + colorScheme: 'bnbColors', + seriesType: EchartsTimeseriesSeriesType.Bar, + x_axis: 'genre', + xAxis: 'genre', + metrics: ['count'], + groupby: ['platform'], + timeseries_limit_metric: naSales, + truncate_metric: true, + stack: true, + showValue: true, + onlyTotal: true, + ...overrides, + } as Partial<EchartsTimeseriesFormData>, + queriesData: + data === rows + ? queriesData + : [{ ...queriesData[0], data } as ChartDataResponseResult], + datasource: { verboseMap }, + }); + const transformed = transformProps(chartProps); + const series = transformed.echartOptions.series as Series[]; + return { + series, + // Legend order is a separate control; only the set of series matters here. + seriesNames: series.map(({ name }) => name).sort(), + axisOrder: series[0].data.map(([genre]) => genre), + // With `onlyTotal` the top series' label carries the stacked total. + stackedTotals: series + .flatMap((entry, seriesIndex) => + entry.data.map((value, dataIndex) => + entry.label.formatter({ value, dataIndex, seriesIndex }), + ), + ) + .filter(label => label !== ''), + }; +} + +test('sorts the axis by the sort-only metric across its pivoted columns', () => { + const { axisOrder, seriesNames, stackedTotals } = transform({ + x_axis_sort: 'SUM(na_sales)', + xAxisSort: 'SUM(na_sales)', + x_axis_sort_asc: false, + xAxisSortAsc: false, + }); + // SUM(na_sales) totals: Puzzle 30, Action 8, Sports 2. Sorting by the value + // metric would put Action/Sports first, sorting by name would put Puzzle + // second. + expect(axisOrder).toEqual(['Puzzle', 'Action', 'Sports']); + expect(seriesNames).toEqual(['PS4', 'XOne']); + // COUNT(*) totals only, in the sorted order. + expect(stackedTotals).toEqual(['10', '50', '50']); +}); + +test('sorts the axis by the sort-only metric ascending', () => { + const { axisOrder } = transform({ + x_axis_sort: 'SUM(na_sales)', + xAxisSort: 'SUM(na_sales)', + x_axis_sort_asc: true, + xAxisSortAsc: true, + }); + expect(axisOrder).toEqual(['Sports', 'Action', 'Puzzle']); +}); + +test('sorts by the sort-only metric when it has a verbose name', () => { + const { axisOrder, seriesNames } = transform( + { + x_axis_sort: 'SUM(na_sales)', + xAxisSort: 'SUM(na_sales)', + x_axis_sort_asc: false, + xAxisSortAsc: false, + }, + { 'SUM(na_sales)': 'NA Sales' }, + ); + expect(axisOrder).toEqual(['Puzzle', 'Action', 'Sports']); + expect(seriesNames).toEqual(['PS4', 'XOne']); +}); + +test('sorts the axis by the x-axis column with dimensions set', () => { + expect( + transform({ + x_axis_sort: 'genre', + xAxisSort: 'genre', + x_axis_sort_asc: false, + xAxisSortAsc: false, + }).axisOrder, + ).toEqual(['Sports', 'Puzzle', 'Action']); + expect( + transform({ + x_axis_sort: 'genre', + xAxisSort: 'genre', + x_axis_sort_asc: true, + xAxisSortAsc: true, + }).axisOrder, + ).toEqual(['Action', 'Puzzle', 'Sports']); +}); + +test('still sorts the axis by a series aggregate with dimensions set', () => { + // An aggregate sort does not query the sort-only metric, so its columns + // are not in the response. + const valueRows = rows.map(({ genre, PS4, XOne }) => ({ genre, PS4, XOne })); + const { axisOrder, seriesNames } = transform( + { + x_axis_sort: SortSeriesType.Max, + xAxisSort: SortSeriesType.Max, + x_axis_sort_asc: true, + xAxisSortAsc: true, + }, + {}, + valueRows, + ); + // Max per row: Puzzle 5, Action 30, Sports 40. + expect(axisOrder).toEqual(['Puzzle', 'Action', 'Sports']); + expect(seriesNames).toEqual(['PS4', 'XOne']); +}); + +test('leaves the query order alone when the sort field is not in the data', () => { + const { axisOrder } = transform({ + x_axis_sort: 'jp_sales', + xAxisSort: 'jp_sales', + x_axis_sort_asc: false, + xAxisSortAsc: false, + }); + expect(axisOrder).toEqual(['Action', 'Puzzle', 'Sports']); +}); diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts index 78e88961a00..03af2ee7b27 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts @@ -333,6 +333,55 @@ test('sortRows by max descending', () => { ]); }); +test('sortRows by the sum of specific columns descending', () => { + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + { sumOfColumns: ['x', 'z'] }, + false, + ), + ).toEqual([ + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, + ]); +}); + +test('sortRows by the sum of specific columns ascending ignores the other columns', () => { + // `y` would put `foo` last; only `x` and `z` count. + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + { sumOfColumns: ['x', 'z'] }, + true, + ), + ).toEqual([ + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + ]); +}); + +test('sortRows by the sum of specific columns puts rows with no value last when ascending', () => { + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + { sumOfColumns: ['x'] }, + true, + ), + ).toEqual([ + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + ]); +}); + test('sortAndFilterSeries by min ascending', () => { expect( sortAndFilterSeries(sortData, 'my_x_axis', [], SortSeriesType.Min, true), @@ -505,6 +554,48 @@ test('extractDataTotalValues still respects legendState alongside extraMetricLab expect(result.totalStackedValues).toEqual([32]); }); +test("extractSeries orders rows by the sum of a metric's pivoted columns and hides those columns", () => { + const data = [ + { + genre: 'Action', + PS4: 30, + XOne: 20, + 'SUM(na_sales), PS4': 5, + 'SUM(na_sales), XOne': 3, + }, + { + genre: 'Puzzle', + PS4: 5, + XOne: 5, + 'SUM(na_sales), PS4': 20, + 'SUM(na_sales), XOne': 10, + }, + { + genre: 'Sports', + PS4: 40, + XOne: 10, + 'SUM(na_sales), PS4': 1, + 'SUM(na_sales), XOne': 1, + }, + ]; + const [series, sortedTotals] = extractSeries(data, { + xAxis: 'genre', + extraMetricLabels: ['SUM(na_sales), PS4', 'SUM(na_sales), XOne'], + totalStackedValues: [50, 10, 50], + xAxisSortSeries: { + sumOfColumns: ['SUM(na_sales), PS4', 'SUM(na_sales), XOne'], + }, + xAxisSortSeriesAscending: false, + }); + expect(series.map(({ name }) => name).sort()).toEqual(['PS4', 'XOne']); + expect(series.find(({ name }) => name === 'PS4')?.data).toEqual([ + ['Puzzle', 5], + ['Action', 30], + ['Sports', 40], + ]); + expect(sortedTotals).toEqual([10, 50, 50]); +}); + describe('extractSeries', () => { test('should generate a valid ECharts timeseries series object', () => { const data = [
