sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4384192245
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -408,16 +411,42 @@ export default function transformProps(
// derived comparison row from a base row whose dimension value happens to
read like
// the offset, and it is gone from `labelMap` by the time the formatters run.
const derivedComparisonSeries = new Set<string>();
+ // The `<metric>, <offset>` labels `renameOperator` generates when more than
one
+ // metric (displayed or sort-only) is pivoted with an actual-values
comparison.
+ const sortOnlyMetrics = extractExtraMetrics(chartProps.rawFormData);
+ const metricOffsetLabels = new Set(
+ [...ensureIsArray(metrics), ...sortOnlyMetrics].flatMap(metric =>
+ ensureIsArray(timeCompare).map(
+ offset => `${getMetricLabel(metric)}, ${offset}`,
+ ),
+ ),
+ );
const labelMap: { [key: string]: string[] } = Object.entries(
label_map,
).reduce((acc, entry) => {
if (
entry[1].length > groupBy.length &&
Array.isArray(timeCompare) &&
+ // a sort-only metric means more than one metric is pivoted, so the
+ // offset never leads the entry; a leading offset-like part is then a
+ // hidden metric that happens to share the offset's label
+ sortOnlyMetrics.length === 0 &&
timeCompare.includes(entry[1][0])
) {
derivedComparisonSeries.add(entry[0]);
entry[1].shift();
+ } else if (
+ entry[1].length > groupBy.length &&
+ Array.isArray(timeCompare) &&
+ (metricOffsetLabels.has(entry[1][0]) ||
+ // without dimensions the flat `<metric>, <offset>` label is split
+ // into a tuple, so the whole label is the key
+ (groupBy.length === 0 && metricOffsetLabels.has(entry[0])))
+ ) {
+ // `renameOperator` emits `<metric>, <offset>` as the leading part when
more
+ // than one metric is pivoted (including a hidden sort-only metric), so
the
+ // offset trails the metric instead of leading the entry.
+ derivedComparisonSeries.add(entry[0]);
Review Comment:
With one displayed metric `count`, a dimension `platform`, an actual-values
`1 year ago` comparison, and a Sort By metric set, the hidden sort metric makes
`sortOnlyMetrics` non-empty, so the first branch no longer shifts the offset
and this branch keeps the prefix: the comparison series' `labelMap` entry stays
`['count, 1 year ago', 'PS4']` instead of `['PS4']`. The context-menu handler
in `EchartsTimeseries.tsx` builds the Drill to detail filters positionally from
`labelMap[seriesName]` without skipping metric prefixes, so right-clicking the
comparison bar for Action/PS4 sends `platform = 'count, 1 year ago'` and the
detail pane comes back empty. Could the prefix be dropped from `labelMap` here
(keeping `derivedComparisonSeries` as the marker), or the drill filters made to
skip it?
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts:
##########
@@ -0,0 +1,1235 @@
+/**
+ * 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 { StackControlsValue } from '../../src/constants';
+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[];
+ // The sort-only metric's forecast column stays hidden: it is neither
+ // rendered as a series nor counted in the stacked totals.
+ expect(series.map(({ name }) => name)).not.toContain(
Review Comment:
This assertion can't fail for the leak it describes: `transformSeries` names
a series with `forecastSeries.name`, which strips the `__yhat` suffix, so a
leaked forecast column would be named `SUM(na_sales), US` and
`not.toContain('SUM(na_sales), US__yhat')` still passes. The sort assertions
below only prove the forecast column is excluded from the sort key, not that it
is hidden from the rendered series or the stacked totals. If `{
includeForecast: true }` were dropped from the hidden-column lookup, this test
would stay green. Could it assert the full series ids are only `EU` and `US`
(and, with stacking on, that the displayed-only totals come out as `[10, 50,
50]` in sorted order)?
--
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]
