feat(generic-x-axis): add x sorting on series limit metric (#23274)

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<PostProcessingPivot> = (
   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;
 

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<PostProcessingSort> = (
   formData,
@@ -34,7 +35,8 @@ export const sortOperator: PostProcessingFactory<PostProcessingSort> = (
   // 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 (

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;
+}

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';

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,
   },
 };

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,
+    },
+  });
+});

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,
+    },
+  });
+});

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([]);
+});

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`

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,
-  isTimeseriesAnnotationLayer,
-  TimeseriesChartDataResponseResult,
-  t,
-  AxisType,
-  getXAxisLabel,
   isPhysicalColumn,
-  isDefined,
+  isTimeseriesAnnotationLayer,
+  t,
+  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,
-    stack,
-    truncateYAxis,
-    yAxisFormat,
-    xAxisTimeFormat,
-    yAxisBounds,
-    tooltipTimeFormat,
-    tooltipSortByMetric,
-    zoomable,
-    richTooltip,
-    xAxis: xAxisOrig,
-    xAxisLabelRotation,
-    groupby,
     showValue,
-    onlyTotal,
-    percentageThreshold,
-    xAxisTitle,
-    yAxisTitle,
-    xAxisTitleMargin,
-    yAxisTitleMargin,
-    yAxisTitlePosition,
     sliceId,
     timeGrainSqla,
-    orientation,
+    stack,
+    tooltipTimeFormat,
+    tooltipSortByMetric,
+    truncateYAxis,
+    xAxis: xAxisOrig,
+    xAxisLabelRotation,
+    xAxisTimeFormat,
+    xAxisTitle,
+    xAxisTitleMargin,
+    yAxisBounds,
+    yAxisFormat,
+    yAxisTitle,
+    yAxisTitleMargin,
+    yAxisTitlePosition,
+    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,

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,