From e25f14880a4069a9236f59bfe8809b26eba0d7f6 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Mon, 6 Mar 2023 11:50:20 +0200 Subject: [PATCH] feat(generic-x-axis): add x sorting on series limit metric (#23274) (cherry picked from commit 1b139d074852e13c113a408a920991b2abc98387) --- .../src/operators/pivotOperator.ts | 6 +- .../src/operators/sortOperator.ts | 4 +- .../operators/utils/extractExtraMetrics.ts | 38 ++++++++ .../src/operators/utils/index.ts | 1 + .../src/shared-controls/customControls.tsx | 83 ++++++++-------- .../test/operators/pivotOperator.test.ts | 30 ++++++ .../test/operators/sortOperator.test.ts | 25 +++++ .../utils/extractExtraMetrics.test.ts | 94 +++++++++++++++++++ .../src/Timeseries/buildQuery.ts | 35 ++++--- .../src/Timeseries/transformProps.ts | 51 +++++----- .../plugin-chart-echarts/src/utils/series.ts | 3 + 11 files changed, 294 insertions(+), 76 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts create mode 100644 superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/extractExtraMetrics.test.ts diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts index 3adec29fecec..2b8a2bd2f0d5 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts @@ -24,12 +24,16 @@ import { getXAxisLabel, } from '@superset-ui/core'; import { PostProcessingFactory } from './types'; +import { extractExtraMetrics } from './utils'; export const pivotOperator: PostProcessingFactory = ( formData, queryObject, ) => { - const metricLabels = ensureIsArray(queryObject.metrics).map(getMetricLabel); + const metricLabels = [ + ...ensureIsArray(queryObject.metrics), + ...extractExtraMetrics(formData), + ].map(getMetricLabel); const xAxisLabel = getXAxisLabel(formData); const columns = queryObject.series_columns || queryObject.columns; diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/sortOperator.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/sortOperator.ts index 0650c8b57785..4d7b5deaf4f7 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/sortOperator.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/sortOperator.ts @@ -26,6 +26,7 @@ import { PostProcessingSort, } from '@superset-ui/core'; import { PostProcessingFactory } from './types'; +import { extractExtraMetrics } from './utils'; export const sortOperator: PostProcessingFactory = ( formData, @@ -34,7 +35,8 @@ export const sortOperator: PostProcessingFactory = ( // the sortOperator only used in the barchart v2 const sortableLabels = [ getXAxisLabel(formData), - ...ensureIsArray(formData.metrics).map(metric => getMetricLabel(metric)), + ...ensureIsArray(formData.metrics).map(getMetricLabel), + ...extractExtraMetrics(formData).map(getMetricLabel), ].filter(Boolean); if ( 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 new file mode 100644 index 000000000000..74928f836f86 --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts @@ -0,0 +1,38 @@ +/** + * 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 { + getMetricLabel, + QueryFormData, + QueryFormMetric, +} from '@superset-ui/core'; + +export function extractExtraMetrics( + formData: QueryFormData, +): QueryFormMetric[] { + const { groupby, timeseries_limit_metric, x_axis_sort } = formData; + const extra_metrics: QueryFormMetric[] = []; + if ( + !(groupby || []).length && + timeseries_limit_metric && + getMetricLabel(timeseries_limit_metric) === x_axis_sort + ) { + extra_metrics.push(timeseries_limit_metric); + } + return extra_metrics; +} diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts index 8d65ca1e590f..67f230dd631b 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/index.ts @@ -20,4 +20,5 @@ export { getMetricOffsetsMap } from './getMetricOffsetsMap'; export { isTimeComparison } from './isTimeComparison'; export { isDerivedSeries } from './isDerivedSeries'; +export { extractExtraMetrics } from './extractExtraMetrics'; export { TIME_COMPARISON_SEPARATOR } from './constants'; 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 979912e58f1d..8e8f4d8400ca 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 @@ -23,12 +23,16 @@ import { getColumnLabel, getMetricLabel, isDefined, - isEqualArray, QueryFormColumn, QueryFormMetric, t, } from '@superset-ui/core'; -import { ControlPanelState, ControlState, ControlStateMapping } from '../types'; +import { + ControlPanelState, + ControlState, + ControlStateMapping, + isDataset, +} from '../types'; import { isTemporalColumn } from '../utils'; export const contributionModeControl = { @@ -59,39 +63,42 @@ export const xAxisSortControl = { name: 'x_axis_sort', config: { type: 'XAxisSortControl', - label: t('X-Axis Sort By'), - description: t('Whether to sort descending or ascending on the X-Axis.'), - shouldMapStateToProps: ( - prevState: ControlPanelState, - state: ControlPanelState, - ) => { - const prevOptions = [ - getColumnLabel(prevState?.controls?.x_axis?.value as QueryFormColumn), - ...ensureIsArray(prevState?.controls?.metrics?.value).map(metric => - getMetricLabel(metric as QueryFormMetric), - ), - ]; - const currOptions = [ - getColumnLabel(state?.controls?.x_axis?.value as QueryFormColumn), - ...ensureIsArray(state?.controls?.metrics?.value).map(metric => - getMetricLabel(metric as QueryFormMetric), - ), - ]; - return !isEqualArray(prevOptions, currOptions); - }, - mapStateToProps: ( - { controls }: { controls: ControlStateMapping }, - controlState: ControlState, - ) => { - const choices = [ - getColumnLabel(controls?.x_axis?.value as QueryFormColumn), - ...ensureIsArray(controls?.metrics?.value).map(metric => - getMetricLabel(metric as QueryFormMetric), - ), + label: (state: ControlPanelState) => + state.form_data?.orientation === 'horizontal' + ? t('Y-Axis Sort By') + : t('X-Axis Sort By'), + description: t('Decides which column to sort the base axis by.'), + shouldMapStateToProps: () => true, + mapStateToProps: (state: ControlPanelState, controlState: ControlState) => { + const { controls, datasource } = state; + const dataset = isDataset(datasource) ? datasource : undefined; + const columns = [controls?.x_axis?.value as QueryFormColumn].filter( + Boolean, + ); + const metrics = [ + ...ensureIsArray(controls?.metrics?.value as QueryFormMetric), + controls?.timeseries_limit_metric?.value as QueryFormMetric, ].filter(Boolean); + const options = [ + ...columns.map(column => { + const value = getColumnLabel(column); + return { + value, + label: dataset?.verbose_map?.[value] || value, + }; + }), + ...metrics.map(metric => { + const value = getMetricLabel(metric); + return { + value, + label: dataset?.verbose_map?.[value] || value, + }; + }), + ]; + const shouldReset = !( typeof controlState.value === 'string' && - choices.includes(controlState.value) && + options.map(option => option.value).includes(controlState.value) && !isTemporalColumn( getColumnLabel(controls?.x_axis?.value as QueryFormColumn), controls?.datasource?.datasource, @@ -100,10 +107,7 @@ export const xAxisSortControl = { return { shouldReset, - options: choices.map(entry => ({ - value: entry, - label: entry, - })), + options, }; }, visibility: xAxisSortVisibility, @@ -114,9 +118,12 @@ export const xAxisSortAscControl = { name: 'x_axis_sort_asc', config: { type: 'CheckboxControl', - label: t('X-Axis Sort Ascending'), + label: (state: ControlPanelState) => + state.form_data?.orientation === 'horizontal' + ? t('Y-Axis Sort Ascending') + : t('X-Axis Sort Ascending'), default: true, - description: t('Whether to sort descending or ascending on the X-Axis.'), + description: t('Whether to sort ascending or descending on the base Axis.'), visibility: xAxisSortVisibility, }, }; diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts index 2a527a04fe54..6101fc19e54c 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/pivotOperator.test.ts @@ -185,3 +185,33 @@ test('pivot by adhoc x_axis', () => { }, }); }); + +test('pivot by x_axis with extra metrics', () => { + expect( + pivotOperator( + { + ...formData, + x_axis: 'foo', + x_axis_sort: 'bar', + groupby: [], + timeseries_limit_metric: 'bar', + }, + { + ...queryObject, + series_columns: [], + }, + ), + ).toEqual({ + operation: 'pivot', + options: { + index: ['foo'], + columns: [], + aggregates: { + 'count(*)': { operator: 'mean' }, + 'sum(val)': { operator: 'mean' }, + bar: { operator: 'mean' }, + }, + drop_missing_columns: false, + }, + }); +}); diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/operators/sortOperator.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/operators/sortOperator.test.ts index 750d726c902a..41d44b50a838 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/test/operators/sortOperator.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/sortOperator.test.ts @@ -146,3 +146,28 @@ test('should sort by axis', () => { }, }); }); + +test('should sort by extra metric', () => { + Object.defineProperty(supersetCoreModule, 'hasGenericChartAxes', { + value: true, + }); + expect( + sortOperator( + { + ...formData, + x_axis_sort: 'my_limit_metric', + x_axis_sort_asc: true, + x_axis: 'Categorical Column', + groupby: [], + timeseries_limit_metric: 'my_limit_metric', + }, + queryObject, + ), + ).toEqual({ + operation: 'sort', + options: { + by: 'my_limit_metric', + ascending: true, + }, + }); +}); 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 new file mode 100644 index 000000000000..89f4c1118119 --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/test/operators/utils/extractExtraMetrics.test.ts @@ -0,0 +1,94 @@ +/** + * 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 { QueryFormData, QueryFormMetric } from '@superset-ui/core'; +import { extractExtraMetrics } from '@superset-ui/chart-controls'; + +const baseFormData: QueryFormData = { + datasource: 'dummy', + viz_type: 'table', + metrics: ['a', 'b'], + columns: ['foo', 'bar'], + limit: 100, + metrics_b: ['c', 'd'], + columns_b: ['hello', 'world'], + limit_b: 200, +}; + +const metric: QueryFormMetric = { + expressionType: 'SQL', + sqlExpression: 'case when 1 then 1 else 2 end', + label: 'foo', +}; + +test('returns empty array if relevant controls missing', () => { + expect( + extractExtraMetrics({ + ...baseFormData, + }), + ).toEqual([]); +}); + +test('returns empty array if x_axis_sort is not same as timeseries_limit_metric', () => { + expect( + extractExtraMetrics({ + ...baseFormData, + timeseries_limit_metric: 'foo', + x_axis_sort: 'bar', + }), + ).toEqual([]); +}); + +test('returns correct column if sort columns match', () => { + expect( + extractExtraMetrics({ + ...baseFormData, + timeseries_limit_metric: 'foo', + x_axis_sort: 'foo', + }), + ).toEqual(['foo']); +}); + +test('handles adhoc metrics correctly', () => { + expect( + extractExtraMetrics({ + ...baseFormData, + timeseries_limit_metric: metric, + x_axis_sort: 'foo', + }), + ).toEqual([metric]); + + expect( + extractExtraMetrics({ + ...baseFormData, + timeseries_limit_metric: metric, + x_axis_sort: 'bar', + }), + ).toEqual([]); +}); + +test('returns empty array if groupby populated', () => { + expect( + extractExtraMetrics({ + ...baseFormData, + groupby: ['bar'], + timeseries_limit_metric: 'foo', + x_axis_sort: 'foo', + }), + ).toEqual([]); +}); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts index ad021f92b918..69a8020657b8 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/buildQuery.ts @@ -19,24 +19,25 @@ import { buildQueryContext, ensureIsArray, + getXAxisColumn, + isXAxisSet, normalizeOrderBy, PostProcessingPivot, QueryFormData, - getXAxisColumn, - isXAxisSet, } from '@superset-ui/core'; import { - rollingWindowOperator, - timeCompareOperator, + contributionOperator, + extractExtraMetrics, + flattenOperator, isTimeComparison, pivotOperator, - resampleOperator, - renameOperator, - contributionOperator, prophetOperator, - timeComparePivotOperator, - flattenOperator, + renameOperator, + resampleOperator, + rollingWindowOperator, sortOperator, + timeComparePivotOperator, + timeCompareOperator, } from '@superset-ui/chart-controls'; export default function buildQuery(formData: QueryFormData) { @@ -62,6 +63,9 @@ export default function buildQuery(formData: QueryFormData) { 2015-03-01 318.0 0.0 */ + // only add series limit metric if it's explicitly needed e.g. for sorting + const extra_metrics = extractExtraMetrics(formData); + const pivotOperatorInRuntime: PostProcessingPivot = isTimeComparison( formData, baseQueryObject, @@ -69,15 +73,16 @@ export default function buildQuery(formData: QueryFormData) { ? timeComparePivotOperator(formData, baseQueryObject) : pivotOperator(formData, baseQueryObject); + const columns = [ + ...(isXAxisSet(formData) ? ensureIsArray(getXAxisColumn(formData)) : []), + ...ensureIsArray(groupby), + ]; + return [ { ...baseQueryObject, - columns: [ - ...(isXAxisSet(formData) - ? ensureIsArray(getXAxisColumn(formData)) - : []), - ...ensureIsArray(groupby), - ], + metrics: [...(baseQueryObject.metrics || []), ...extra_metrics], + columns, series_columns: groupby, ...(isXAxisSet(formData) ? {} : { is_timeseries: true }), // todo: move `normalizeOrderBy to extractQueryFields` 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 eadded44a9ac..52f472b3ff5f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -19,21 +19,25 @@ /* eslint-disable camelcase */ import { AnnotationLayer, + AxisType, CategoricalColorNamespace, GenericDataType, + getMetricLabel, getNumberFormatter, + getXAxisLabel, + isDefined, isEventAnnotationLayer, isFormulaAnnotationLayer, isIntervalAnnotationLayer, + isPhysicalColumn, isTimeseriesAnnotationLayer, - TimeseriesChartDataResponseResult, t, - AxisType, - getXAxisLabel, - isPhysicalColumn, - isDefined, + TimeseriesChartDataResponseResult, } from '@superset-ui/core'; -import { isDerivedSeries } from '@superset-ui/chart-controls'; +import { + extractExtraMetrics, + isDerivedSeries, +} from '@superset-ui/chart-controls'; import { EChartsCoreOption, SeriesOption } from 'echarts'; import { ZRLineType } from 'echarts/types/src/util/types'; import { @@ -114,39 +118,39 @@ export default function transformProps( colorScheme, contributionMode, forecastEnabled, + groupby, legendOrientation, legendType, legendMargin, logAxis, markerEnabled, markerSize, - opacity, minorSplitLine, + onlyTotal, + opacity, + orientation, + percentageThreshold, + richTooltip, seriesType, showLegend, + showValue, + sliceId, + timeGrainSqla, stack, - truncateYAxis, - yAxisFormat, - xAxisTimeFormat, - yAxisBounds, tooltipTimeFormat, tooltipSortByMetric, - zoomable, - richTooltip, + truncateYAxis, xAxis: xAxisOrig, xAxisLabelRotation, - groupby, - showValue, - onlyTotal, - percentageThreshold, + xAxisTimeFormat, xAxisTitle, - yAxisTitle, xAxisTitleMargin, + yAxisBounds, + yAxisFormat, + yAxisTitle, yAxisTitleMargin, yAxisTitlePosition, - sliceId, - timeGrainSqla, - orientation, + zoomable, }: EchartsTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData }; const refs: Refs = {}; @@ -168,9 +172,14 @@ export default function transformProps( xAxisCol: xAxisLabel, }, ); + const extraMetricLabels = extractExtraMetrics(chartProps.rawFormData).map( + getMetricLabel, + ); + const rawSeries = extractSeries(rebasedData, { fillNeighborValue: stack && !forecastEnabled ? 0 : undefined, xAxis: xAxisLabel, + extraMetricLabels, removeNulls: seriesType === EchartsTimeseriesSeriesType.Scatter, stack, totalStackedValues, 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 c1b61233b6c6..649dedd68051 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts @@ -113,6 +113,7 @@ export function extractSeries( opts: { fillNeighborValue?: number; xAxis?: string; + extraMetricLabels?: string[]; removeNulls?: boolean; stack?: StackType; totalStackedValues?: number[]; @@ -122,6 +123,7 @@ export function extractSeries( const { fillNeighborValue, xAxis = DTTM_ALIAS, + extraMetricLabels = [], removeNulls = false, stack = false, totalStackedValues = [], @@ -135,6 +137,7 @@ export function extractSeries( return Object.keys(rows[0]) .filter(key => key !== xAxis && key !== DTTM_ALIAS) + .filter(key => !extraMetricLabels.includes(key)) .map(key => ({ id: key, name: key,