sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4333203498
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +488,67 @@ 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.
+ // `label_map` is built from the cached DataFrame's final columns, after
+ // every post-processing operator (including Prophet) has run, so a
+ // forecast's `<metric>, <dims>__yhat`/`__yhat_lower`/`__yhat_upper`
+ // columns have the same `[metric, ...dims]` shape and would otherwise be
+ // mistaken for one of the metric's own pivoted columns. Those are excluded
+ // the same way they are hidden from the rendered series (see
+ // extractForecastSeriesContext usage above), so the sort key never
+ // depends on the forecast trend or confidence bounds.
+ const pivotedColumnsOf = (
+ metricLabel: string,
+ { includeForecast = false }: { includeForecast?: boolean } = {},
+ ): string[] =>
+ Object.entries(labelMap)
+ .filter(
+ ([column, parts]) =>
+ column !== metricLabel &&
+ // a metric-prefixed column carries the metric plus one part per
+ // dimension; a truncated dimension tuple has no metric part and
+ // must not be mistaken for one when a dimension value reads like
+ // the metric label
+ parts.length === groupBy.length + 1 &&
+ parts[0] === metricLabel &&
+ !derivedComparisonSeries.has(column) &&
+ (includeForecast ||
+ extractForecastSeriesContext(column).type ===
+ ForecastSeriesEnum.Observation),
+ )
+ .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).
- const extraMetricLabels = extractExtraMetrics(chartProps.rawFormData)
- .map(getMetricLabel)
- .map(label => verboseMap[label] ?? label);
+ // 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.
+ // The scatter dot-size metric is also queried as an extra metric when it
+ // doubles as the sort target; it must stay in the extracted series so the
+ // size lookup can read it (it is hidden from rendering separately).
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const extraMetrics = extractExtraMetrics(chartProps.rawFormData);
+ const extraMetricLabels = extraMetrics.map(getMetricLabel).flatMap(label =>
+ label === sizeMetricLabel
+ ? []
+ : [
+ verboseMap[label] ?? label,
Review Comment:
With one dimension, Truncate Metric on, and a dimension value `B`, selecting
a sort-only metric that is also labeled `B` still hides the displayed `B`
series: the bare label is pushed into `extraMetricLabels`, so it drops out of
the chart, legend, and stacked totals, and the `sumOfColumns` lookup further
down adds that displayed column into the hidden metric's sort key. For example,
with displayed `count`, `platform` rows X (`B`=100, hidden total 1) and Y
(`B`=1, hidden total 10), a descending sort puts X ahead of Y. The tuple-length
guard in `pivotedColumnsOf` only covers the metric-prefixed path, not this
bare-label one. Could the bare label be excluded only when the data confirms it
is the sort-only metric's own column, rather than by name alone?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1171,7 +1357,12 @@ export default function transformProps(
for (const s of series) {
if (s.id) {
const columnsArr = labelMap[s.id];
- const dimensionValue = columnsArr?.[idxSelectedDimension];
+ const metricPartCount = Math.max(
+ 0,
+ (columnsArr?.length ?? 0) - groupBy.length,
+ );
+ const dimensionValue =
+ columnsArr?.[metricPartCount + idxSelectedDimension];
Review Comment:
If `stackDimension` is stale (say `dept`, which was removed from `groupby`),
`idxSelectedDimension` is `-1`, and adding `metricPartCount` turns that into a
valid index. With metrics `sales`/`profit` and `groupby: ['region']`, the tuple
is `['sales', 'East']`, so `metricPartCount` is 1, the lookup reads index 0,
and each series gets stack `sales` or `profit`. Values 10 and 20 then render as
separate bars instead of one stacked 30, while the total-label path still
combines them. Before this change the `-1` index found nothing and the stack
assignment was skipped. Should this loop skip when `idxSelectedDimension < 0`,
as `seriesStackIds` does?
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts:
##########
@@ -0,0 +1,853 @@
+/**
+ * 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,
+): {
+ series: Series[];
+ seriesNames: string[];
+ axisOrder: string[];
+ stackedTotals: string[];
+} {
+ 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']);
+});
+
+test('sorts the axis by the sole displayed metric when truncate_metric drops
its label', () => {
+ // With a single displayed metric and `truncate_metric` on, the backend
+ // pivots columns straight to the dimension values (`PS4`/`XOne`) with no
+ // `count, ` prefix, so `label_map` never marks them as belonging to
+ // `count`. Selecting the displayed metric itself as the sort target must
+ // still sum those columns rather than leaving the axis in query order.
+ const valueRows = rows.map(({ genre, PS4, XOne }) => ({ genre, PS4, XOne }));
+ const { axisOrder } = transform(
+ {
+ x_axis_sort: 'count',
+ xAxisSort: 'count',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ timeseries_limit_metric: undefined,
+ },
+ {},
+ valueRows,
+ );
+ // PS4 + XOne totals: Action 50, Puzzle 10, Sports 50.
+ expect(axisOrder).toEqual(['Action', 'Sports', 'Puzzle']);
+});
+
+test('permutes percentage-threshold labels along with the sorted rows', () => {
+ // Action/Puzzle/Sports stacked totals (PS4 + XOne) are 50/10/50, so a 50%
+ // threshold requires 25/5/25 respectively. Sorting by SUM(na_sales) desc
+ // reorders the axis to Puzzle, Action, Sports (see the sort-only-metric
+ // test above); each row's threshold must travel with it rather than
+ // staying keyed to its pre-sort position.
+ const { series } = transform({
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ onlyTotal: false,
+ percentageThreshold: 50,
+ });
+ const xOneSeries = series.find(({ name }) => name === 'XOne')!;
+ const labels = xOneSeries.data.map((value, dataIndex) =>
+ xOneSeries.label.formatter({ value, dataIndex, seriesIndex: 0 }),
+ );
+ // XOne values in sorted order: Puzzle 5, Action 20, Sports 10. Only
+ // Puzzle's own value (5) clears its own threshold (5); Action's (20) and
+ // Sports' (10) both fall short of theirs (25 each).
+ expect(labels).toEqual(['5', '', '']);
+});
+
+test('sorts the axis by the sort-only metric with a time comparison', () => {
+ // With a time comparison the backend also returns the shifted value
+ // metric, which `renameOperator` relabels to the bare offset, and
+ // `label_map` leads those entries with the offset. The sort-only metric
+ // is pivoted like the value metric; its own shifted column is dropped by
+ // the pivot, so it never reaches the response.
+ const comparisonRows: DataRecord[] = [
+ {
+ genre: 'Action',
+ 'count, PS4': 30,
+ 'count, XOne': 20,
+ '1 year ago, PS4': 25,
+ '1 year ago, XOne': 15,
+ 'SUM(na_sales), PS4': 5,
+ 'SUM(na_sales), XOne': 3,
+ },
+ {
+ genre: 'Puzzle',
+ 'count, PS4': 5,
+ 'count, XOne': 5,
+ '1 year ago, PS4': 4,
+ '1 year ago, XOne': 6,
+ 'SUM(na_sales), PS4': 20,
+ 'SUM(na_sales), XOne': 10,
+ },
+ {
+ genre: 'Sports',
+ 'count, PS4': 40,
+ 'count, XOne': 10,
+ '1 year ago, PS4': 35,
+ '1 year ago, XOne': 12,
+ 'SUM(na_sales), PS4': 1,
+ 'SUM(na_sales), XOne': 1,
+ },
+ ];
+ const comparisonLabelMap = {
+ genre: ['genre'],
+ count: ['count'],
+ 'SUM(na_sales)': ['SUM(na_sales)'],
+ 'count, PS4': ['count', 'PS4'],
+ 'count, XOne': ['count', 'XOne'],
+ '1 year ago, PS4': ['1 year ago', 'PS4'],
+ '1 year ago, XOne': ['1 year ago', 'XOne'],
+ 'SUM(na_sales), PS4': ['SUM(na_sales)', 'PS4'],
+ 'SUM(na_sales), XOne': ['SUM(na_sales)', 'XOne'],
+ };
+ 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,
+ comparison_type: 'values',
+ comparisonType: 'values',
+ time_compare: ['1 year ago'],
+ timeCompare: ['1 year ago'],
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ } as Partial<EchartsTimeseriesFormData>,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: comparisonRows,
+ label_map: comparisonLabelMap,
+ } as unknown as ChartDataResponseResult,
+ ],
+ });
+ const series = transformProps(chartProps).echartOptions.series as Series[];
+
+ // SUM(na_sales) totals: Puzzle 30, Action 8, Sports 2.
+ series.forEach(entry => {
+ expect(entry.data.map(([genre]) => genre)).toEqual([
+ 'Puzzle',
+ 'Action',
+ 'Sports',
+ ]);
+ });
+ // The value metric and its comparison are rendered per platform; neither
+ // the sort-only metric's columns nor a shifted variant of them are.
+ expect(series.map(({ name }) => name).sort()).toEqual([
+ '1 year ago, PS4',
+ '1 year ago, XOne',
+ 'count, PS4',
+ 'count, XOne',
+ ]);
+});
+
+test('does not hide a two-dimension series whose truncated label collides with
the sort-only metric', () => {
+ // With two dimensions (`platform`, `region`) and a single displayed
+ // metric, `truncate_metric` drops the metric prefix from the displayed
+ // series, so their `label_map` entries are just the two dimension values
+ // (2 parts). The sort-only metric keeps its own prefix, so its pivoted
+ // columns always carry 3 parts (metric + the two dimension values). A
+ // dimension value that happens to read like the sort-only metric's label
+ // ("SUM(na_sales)") must not make a genuine 2-part displayed column look
+ // like one of the sort-only metric's 3-part columns.
+ const twoDimRows: DataRecord[] = [
+ {
+ genre: 'Action',
+ 'SUM(na_sales), US': 30,
+ 'PS4, US': 20,
+ 'SUM(na_sales), SUM(na_sales), US': 5,
+ 'SUM(na_sales), PS4, US': 3,
+ },
+ {
+ genre: 'Puzzle',
+ 'SUM(na_sales), US': 5,
+ 'PS4, US': 5,
+ 'SUM(na_sales), SUM(na_sales), US': 20,
+ 'SUM(na_sales), PS4, US': 10,
+ },
+ {
+ genre: 'Sports',
+ 'SUM(na_sales), US': 40,
+ 'PS4, US': 10,
+ 'SUM(na_sales), SUM(na_sales), US': 1,
+ 'SUM(na_sales), PS4, US': 1,
+ },
+ ];
+ const twoDimLabelMap = {
+ genre: ['genre'],
+ 'SUM(na_sales), US': ['SUM(na_sales)', 'US'],
+ 'PS4, US': ['PS4', 'US'],
+ 'SUM(na_sales), SUM(na_sales), US': [
+ 'SUM(na_sales)',
+ 'SUM(na_sales)',
+ 'US',
+ ],
+ 'SUM(na_sales), PS4, US': ['SUM(na_sales)', 'PS4', 'US'],
+ };
+ 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', 'region'],
+ timeseries_limit_metric: naSales,
+ truncate_metric: true,
+ stack: true,
+ showValue: true,
+ onlyTotal: true,
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ } as Partial<EchartsTimeseriesFormData>,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: twoDimRows,
+ label_map: twoDimLabelMap,
+ } as unknown as ChartDataResponseResult,
+ ],
+ });
+ const series = transformProps(chartProps).echartOptions.series as Series[];
+
+ // The two displayed (2-part) series render and keep their values; the
+ // 3-part sort-only columns are not among them.
+ expect(series.map(({ name }) => name).sort()).toEqual([
+ 'PS4, US',
+ 'SUM(na_sales), US',
+ ]);
+ // SUM(na_sales) totals (3-part sort-only columns): Puzzle 30, Action 8,
+ // Sports 2.
+ series.forEach(entry => {
+ expect(entry.data.map(([genre]) => genre)).toEqual([
+ 'Puzzle',
+ 'Action',
+ 'Sports',
+ ]);
+ });
+ // Stacked totals use only the displayed (2-part) series: Action 50,
+ // Puzzle 10, Sports 50, read off in the sorted order above.
+ const stackedTotals = series
+ .flatMap((entry, seriesIndex) =>
+ entry.data.map((value, dataIndex) =>
+ entry.label.formatter({ value, dataIndex, seriesIndex }),
+ ),
+ )
+ .filter(label => label !== '');
+ expect(stackedTotals).toEqual(['10', '50', '50']);
+});
+
+test('rolls up a MAX sort metric by MAX rather than summing per-dimension
maxima', () => {
+ // The ungrouped MAX of a metric equals the MAX of its per-dimension
+ // maxima, not their sum. Category A has per-platform maxima 100 and 0
+ // (overall max 100); category B has 60 and 60 (overall max 60). Without
+ // a dimension, A sorts first (100 > 60); summing the pivoted columns
+ // instead of taking their max would reverse that (100 vs 120).
+ const maxMetric: QueryFormMetric = {
+ expressionType: 'SIMPLE',
+ aggregate: 'MAX',
+ column: { column_name: 'na_sales' },
+ label: 'MAX(na_sales)',
+ };
+ const maxRows: DataRecord[] = [
+ {
+ genre: 'A',
+ PS4: 10,
+ XOne: 10,
+ 'MAX(na_sales), PS4': 100,
+ 'MAX(na_sales), XOne': 0,
+ },
+ {
+ genre: 'B',
+ PS4: 10,
+ XOne: 10,
+ 'MAX(na_sales), PS4': 60,
+ 'MAX(na_sales), XOne': 60,
+ },
+ ];
+ const maxLabelMap = {
+ genre: ['genre'],
+ PS4: ['PS4'],
+ XOne: ['XOne'],
+ 'MAX(na_sales), PS4': ['MAX(na_sales)', 'PS4'],
+ 'MAX(na_sales), XOne': ['MAX(na_sales)', 'XOne'],
+ };
+ 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: maxMetric,
+ truncate_metric: true,
+ x_axis_sort: 'MAX(na_sales)',
+ xAxisSort: 'MAX(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ } as Partial<EchartsTimeseriesFormData>,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: maxRows,
+ label_map: maxLabelMap,
+ } as unknown as ChartDataResponseResult,
+ ],
+ });
+ const series = transformProps(chartProps).echartOptions.series as Series[];
+ series.forEach(entry => {
+ expect(entry.data.map(([genre]) => genre)).toEqual(['A', 'B']);
+ });
+});
+
+test('rolls up a MIN sort metric by MIN rather than summing per-dimension
minima', () => {
+ // The ungrouped MIN of a metric equals the MIN of its per-dimension
+ // minima, not their sum. Category A has per-platform minima 0 and 100
+ // (overall min 0); category B has 40 and 40 (overall min 40). Ascending,
+ // A sorts first (0 < 40); summing the pivoted columns instead would
+ // reverse that (100 vs 80).
+ const minMetric: QueryFormMetric = {
+ expressionType: 'SIMPLE',
+ aggregate: 'MIN',
+ column: { column_name: 'na_sales' },
+ label: 'MIN(na_sales)',
+ };
+ const minRows: DataRecord[] = [
+ {
+ genre: 'A',
+ PS4: 10,
+ XOne: 10,
+ 'MIN(na_sales), PS4': 0,
+ 'MIN(na_sales), XOne': 100,
+ },
+ {
+ genre: 'B',
+ PS4: 10,
+ XOne: 10,
+ 'MIN(na_sales), PS4': 40,
+ 'MIN(na_sales), XOne': 40,
+ },
+ ];
+ const minLabelMap = {
+ genre: ['genre'],
+ PS4: ['PS4'],
+ XOne: ['XOne'],
+ 'MIN(na_sales), PS4': ['MIN(na_sales)', 'PS4'],
+ 'MIN(na_sales), XOne': ['MIN(na_sales)', 'XOne'],
+ };
+ 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: minMetric,
+ truncate_metric: true,
+ x_axis_sort: 'MIN(na_sales)',
+ xAxisSort: 'MIN(na_sales)',
+ x_axis_sort_asc: true,
+ xAxisSortAsc: true,
+ } as Partial<EchartsTimeseriesFormData>,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: minRows,
+ label_map: minLabelMap,
+ } as unknown as ChartDataResponseResult,
+ ],
+ });
+ const series = transformProps(chartProps).echartOptions.series as Series[];
+ series.forEach(entry => {
+ expect(entry.data.map(([genre]) => genre)).toEqual(['A', 'B']);
+ });
+});
+
+test('excludes forecast-derived columns from a sort-only metric sum', () => {
+ // `label_map` is built from the cached DataFrame's final columns, after
+ // Prophet has run, so a forecast's `<metric>, <dim>__yhat` column has the
+ // same shape as one of the sort-only metric's own pivoted columns. If it
+ // were summed in, the axis order would depend on the forecast trend
+ // rather than the selected metric.
+ const forecastRows: DataRecord[] = [
+ {
+ genre: 'Action',
+ US: 30,
+ EU: 20,
+ 'SUM(na_sales), US': 5,
+ 'SUM(na_sales), EU': 3,
+ 'SUM(na_sales), US__yhat': 1000,
+ },
+ {
+ genre: 'Puzzle',
+ US: 5,
+ EU: 5,
+ 'SUM(na_sales), US': 20,
+ 'SUM(na_sales), EU': 10,
+ 'SUM(na_sales), US__yhat': 1,
+ },
+ {
+ genre: 'Sports',
+ US: 40,
+ EU: 10,
+ 'SUM(na_sales), US': 1,
+ 'SUM(na_sales), EU': 1,
+ 'SUM(na_sales), US__yhat': 2000,
+ },
+ ];
+ const forecastLabelMap = {
+ genre: ['genre'],
+ US: ['US'],
+ EU: ['EU'],
+ 'SUM(na_sales), US': ['SUM(na_sales)', 'US'],
+ 'SUM(na_sales), EU': ['SUM(na_sales)', 'EU'],
+ 'SUM(na_sales), US__yhat': ['SUM(na_sales)', 'US__yhat'],
+ };
+ 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: ['region'],
+ timeseries_limit_metric: naSales,
+ truncate_metric: true,
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ } as Partial<EchartsTimeseriesFormData>,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: forecastRows,
+ label_map: forecastLabelMap,
+ } as unknown as ChartDataResponseResult,
+ ],
+ });
+ const series = transformProps(chartProps).echartOptions.series as Series[];
+ // SUM(na_sales) totals excluding the forecast column: Puzzle 30, Action
+ // 8, Sports 2. Including the forecast column would instead put Sports
+ // first (2000 + 2 = 2002).
+ series.forEach(entry => {
+ expect(entry.data.map(([genre]) => genre)).toEqual([
Review Comment:
This test only asserts category order, so it would still pass if `{
includeForecast: true }` were removed from the `extraMetricLabels` lookup. With
that option gone, the sort-only metric's `SUM(na_sales), US__yhat` column would
be rendered as a series and counted in the stacked totals. Could it also assert
that no series is named `SUM(na_sales), US__yhat` (or that the totals exclude
it)?
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts:
##########
@@ -0,0 +1,853 @@
+/**
+ * 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,
+): {
+ series: Series[];
+ seriesNames: string[];
+ axisOrder: string[];
+ stackedTotals: string[];
+} {
+ 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']);
+});
+
+test('sorts the axis by the sole displayed metric when truncate_metric drops
its label', () => {
+ // With a single displayed metric and `truncate_metric` on, the backend
+ // pivots columns straight to the dimension values (`PS4`/`XOne`) with no
+ // `count, ` prefix, so `label_map` never marks them as belonging to
+ // `count`. Selecting the displayed metric itself as the sort target must
+ // still sum those columns rather than leaving the axis in query order.
+ const valueRows = rows.map(({ genre, PS4, XOne }) => ({ genre, PS4, XOne }));
+ const { axisOrder } = transform(
+ {
+ x_axis_sort: 'count',
+ xAxisSort: 'count',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ timeseries_limit_metric: undefined,
+ },
+ {},
+ valueRows,
+ );
+ // PS4 + XOne totals: Action 50, Puzzle 10, Sports 50.
+ expect(axisOrder).toEqual(['Action', 'Sports', 'Puzzle']);
+});
+
+test('permutes percentage-threshold labels along with the sorted rows', () => {
+ // Action/Puzzle/Sports stacked totals (PS4 + XOne) are 50/10/50, so a 50%
+ // threshold requires 25/5/25 respectively. Sorting by SUM(na_sales) desc
+ // reorders the axis to Puzzle, Action, Sports (see the sort-only-metric
+ // test above); each row's threshold must travel with it rather than
+ // staying keyed to its pre-sort position.
+ const { series } = transform({
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ onlyTotal: false,
+ percentageThreshold: 50,
+ });
+ const xOneSeries = series.find(({ name }) => name === 'XOne')!;
+ const labels = xOneSeries.data.map((value, dataIndex) =>
+ xOneSeries.label.formatter({ value, dataIndex, seriesIndex: 0 }),
+ );
+ // XOne values in sorted order: Puzzle 5, Action 20, Sports 10. Only
+ // Puzzle's own value (5) clears its own threshold (5); Action's (20) and
+ // Sports' (10) both fall short of theirs (25 each).
+ expect(labels).toEqual(['5', '', '']);
+});
+
+test('sorts the axis by the sort-only metric with a time comparison', () => {
Review Comment:
Two time-comparison shapes the new code handles have no test that would fail
on a revert. (1) A hidden sort metric labeled `sales, 1 year ago`,
actual-values comparison, and rows with sales 1 and 10: an unrestricted suffix
match would treat its base column as a derived comparison series and show it,
and nothing asserts it stays hidden with the descending order reversed. (2) One
displayed metric `count`, a hidden sort metric, and Split stack by `region`,
where the tuples are `['count, 1 year ago', 'East']`: the existing stacking
tests use multiple displayed metrics, so the old metric-count-based index would
still pass. Could cases for both be added, asserting the sort order, hidden
series, and per-region stack ids?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
