This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch viz-pipeline-followups in repository https://gitbox.apache.org/repos/asf/superset.git
commit a208366b4f5ac27802e2c58842d0c2d043ea6410 Author: Claude Code <[email protected]> AuthorDate: Tue Jul 28 01:03:48 2026 -0700 refactor(charts): extract a shared sort-metric/orderby helper for 3 migrated buildQuery.ts partition, paired-t-test, and parallel-coordinates each independently reimplemented the same ~15 lines of legacy-parity logic: resolve a sort metric from timeseries_limit_metric, append it to the selected metrics if missing, and build the orderby tuple. The three differ only in two real, legacy-behavior-derived ways (verified against master:superset/viz.py before extracting): whether an unset sort metric falls back to the first selected metric (partition: yes; the other two: no), and whether ordering only happens when order_desc is explicitly set (paired-t-test/ parallel-coordinates: yes; partition always orders, just flips direction). Extracted `buildSortMetricOrderby()` into @superset-ui/chart-controls/utils, parameterized on those two axes so the three charts keep their real, intentional differences rather than being forced into false uniformity. horizon was left alone -- its ordering logic is simpler (orders directly by the first metric, no timeseries_limit_metric override or metrics-list injection) and isn't actually the same shape as the other three. Also fills a test gap the extraction surfaced: paired-t-test's buildQuery tests only exercised order_desc: true with a sort metric set, never the "append to metrics but don't order" case that is the entire point of its order-gating behavior. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/utils/buildSortMetricOrderby.ts | 87 ++++++++++++++++++++ .../superset-ui-chart-controls/src/utils/index.ts | 1 + .../test/utils/buildSortMetricOrderby.test.ts | 96 ++++++++++++++++++++++ .../plugin-chart-paired-t-test/src/buildQuery.ts | 23 ++---- .../test/buildQuery.test.ts | 12 +++ .../src/buildQuery.ts | 23 ++---- .../plugin-chart-partition/src/buildQuery.ts | 22 ++--- 7 files changed, 217 insertions(+), 47 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/buildSortMetricOrderby.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/buildSortMetricOrderby.ts new file mode 100644 index 00000000000..c04d7785f2b --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/src/utils/buildSortMetricOrderby.ts @@ -0,0 +1,87 @@ +/** + * 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 { ensureIsArray, getMetricLabel } from '@superset-ui/core'; +import type { QueryFormMetric, QueryFormOrderBy } from '@superset-ui/core'; + +export interface BuildSortMetricOrderbyConfig { + /** The query's already-resolved metrics list. */ + metrics: QueryFormMetric[]; + /** The raw `timeseries_limit_metric` form-data value (single or multi). */ + timeseriesLimitMetric?: QueryFormMetric | QueryFormMetric[] | null; + order_desc?: boolean; + /** + * Falls back to the first selected metric when no sort metric is set. + * Charts ported from a legacy viz whose query_obj always had a sort + * metric (defaulting to the first one) should set this; charts whose + * legacy query_obj left ordering absent without one should not. + */ + fallbackToFirstMetric?: boolean; + /** + * When true, only order when `order_desc` is set (matching legacy vizzes + * whose query_obj left the result unordered unless the operator asked + * for descending). When false, always order (ascending unless + * order_desc), matching legacy vizzes that ordered unconditionally. + */ + orderOnlyWhenDesc?: boolean; +} + +export interface SortMetricOrderby { + /** `metrics`, with the sort metric appended if it wasn't already selected. */ + metrics: QueryFormMetric[]; + orderby: QueryFormOrderBy[]; +} + +/** + * Resolves a chart's sort metric and builds the corresponding query_obj + * `orderby`, appending the sort metric to `metrics` if it isn't already + * selected (so its value is present in the result to sort by). Several + * charts ported from the legacy chart-data pipeline share this exact + * shape with only the fallback/gating policy differing per their own + * legacy `query_obj` behavior -- see `fallbackToFirstMetric` and + * `orderOnlyWhenDesc`. + */ +export function buildSortMetricOrderby({ + metrics, + timeseriesLimitMetric, + order_desc: orderDesc, + fallbackToFirstMetric = false, + orderOnlyWhenDesc = false, +}: BuildSortMetricOrderbyConfig): SortMetricOrderby { + const sortByMetric = + ensureIsArray(timeseriesLimitMetric)[0] ?? + (fallbackToFirstMetric ? metrics[0] : undefined); + + if (!sortByMetric) { + return { metrics, orderby: [] }; + } + + const sortByLabel = getMetricLabel(sortByMetric); + const nextMetrics = metrics.some( + metric => getMetricLabel(metric) === sortByLabel, + ) + ? metrics + : [...metrics, sortByMetric]; + + const shouldOrder = orderOnlyWhenDesc ? Boolean(orderDesc) : true; + + return { + metrics: nextMetrics, + orderby: shouldOrder ? [[sortByMetric, !orderDesc]] : [], + }; +} diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts index 3a894a4d02d..1ed55852fb3 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts @@ -30,3 +30,4 @@ export * from './getTemporalColumns'; export * from './displayTimeRelatedControls'; export * from './colorControls'; export * from './metricColumnFilter'; +export * from './buildSortMetricOrderby'; diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/utils/buildSortMetricOrderby.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/utils/buildSortMetricOrderby.test.ts new file mode 100644 index 00000000000..20c002f1f7a --- /dev/null +++ b/superset-frontend/packages/superset-ui-chart-controls/test/utils/buildSortMetricOrderby.test.ts @@ -0,0 +1,96 @@ +/** + * 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 { buildSortMetricOrderby } from '../../src'; + +test('is a no-op when there is no sort metric and no fallback', () => { + const result = buildSortMetricOrderby({ + metrics: ['sum__num'], + timeseriesLimitMetric: undefined, + }); + expect(result).toEqual({ metrics: ['sum__num'], orderby: [] }); +}); + +test('falls back to the first metric when configured to', () => { + const result = buildSortMetricOrderby({ + metrics: ['sum__num', 'avg__num'], + timeseriesLimitMetric: undefined, + fallbackToFirstMetric: true, + }); + expect(result.metrics).toEqual(['sum__num', 'avg__num']); + expect(result.orderby).toEqual([['sum__num', true]]); +}); + +test('appends the sort metric when it is not already selected', () => { + const result = buildSortMetricOrderby({ + metrics: ['sum__num'], + timeseriesLimitMetric: 'count', + }); + expect(result.metrics).toEqual(['sum__num', 'count']); +}); + +test('does not duplicate the sort metric when already selected', () => { + const result = buildSortMetricOrderby({ + metrics: ['sum__num', 'count'], + timeseriesLimitMetric: 'count', + }); + expect(result.metrics).toEqual(['sum__num', 'count']); +}); + +test('unconditional ordering (orderOnlyWhenDesc: false) always orders, flipping direction', () => { + const ascending = buildSortMetricOrderby({ + metrics: ['sum__num'], + timeseriesLimitMetric: 'count', + order_desc: false, + }); + expect(ascending.orderby).toEqual([['count', true]]); + + const descending = buildSortMetricOrderby({ + metrics: ['sum__num'], + timeseriesLimitMetric: 'count', + order_desc: true, + }); + expect(descending.orderby).toEqual([['count', false]]); +}); + +test('gated ordering (orderOnlyWhenDesc: true) only orders when order_desc is set', () => { + const withoutDesc = buildSortMetricOrderby({ + metrics: ['sum__num'], + timeseriesLimitMetric: 'count', + orderOnlyWhenDesc: true, + }); + expect(withoutDesc.metrics).toEqual(['sum__num', 'count']); + expect(withoutDesc.orderby).toEqual([]); + + const withDesc = buildSortMetricOrderby({ + metrics: ['sum__num'], + timeseriesLimitMetric: 'count', + order_desc: true, + orderOnlyWhenDesc: true, + }); + expect(withDesc.orderby).toEqual([['count', false]]); +}); + +test('resolves a multi-value timeseriesLimitMetric to its first entry', () => { + const result = buildSortMetricOrderby({ + metrics: ['sum__num'], + timeseriesLimitMetric: ['count', 'avg__num'], + }); + expect(result.metrics).toEqual(['sum__num', 'count']); + expect(result.orderby).toEqual([['count', true]]); +}); diff --git a/superset-frontend/plugins/plugin-chart-paired-t-test/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-paired-t-test/src/buildQuery.ts index 20f83fe6584..f1d749cd0d3 100644 --- a/superset-frontend/plugins/plugin-chart-paired-t-test/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-paired-t-test/src/buildQuery.ts @@ -19,11 +19,10 @@ import { buildQueryContext, ensureIsArray, - getMetricLabel, QueryFormData, QueryFormMetric, - QueryFormOrderBy, } from '@superset-ui/core'; +import { buildSortMetricOrderby } from '@superset-ui/chart-controls'; /** * Mirrors the legacy PairedTTestViz.query_obj: a timeseries query grouped @@ -33,20 +32,12 @@ import { export default function buildQuery(formData: QueryFormData) { const { timeseries_limit_metric, order_desc } = formData; return buildQueryContext(formData, baseQueryObject => { - let metrics: QueryFormMetric[] = ensureIsArray(baseQueryObject.metrics); - const orderby: QueryFormOrderBy[] = []; - const sortByMetric = ensureIsArray( - timeseries_limit_metric as QueryFormMetric | QueryFormMetric[], - )[0]; - if (sortByMetric) { - const sortByLabel = getMetricLabel(sortByMetric); - if (!metrics.some(metric => getMetricLabel(metric) === sortByLabel)) { - metrics = [...metrics, sortByMetric]; - } - if (order_desc) { - orderby.push([sortByMetric, !order_desc]); - } - } + const { metrics, orderby } = buildSortMetricOrderby({ + metrics: ensureIsArray(baseQueryObject.metrics) as QueryFormMetric[], + timeseriesLimitMetric: timeseries_limit_metric, + order_desc, + orderOnlyWhenDesc: true, + }); return [ { ...baseQueryObject, diff --git a/superset-frontend/plugins/plugin-chart-paired-t-test/test/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-paired-t-test/test/buildQuery.test.ts index ecebf94b8b4..dfb595592b7 100644 --- a/superset-frontend/plugins/plugin-chart-paired-t-test/test/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-paired-t-test/test/buildQuery.test.ts @@ -47,6 +47,18 @@ test('appends the sort metric and orders when order_desc is set', () => { expect(query.orderby).toEqual([['count', false]]); }); +test('appends the sort metric but does not order when order_desc is unset', () => { + // order_desc gates whether the query orders at all here (unlike partition, + // which always orders and only flips direction) -- a sort metric with + // order_desc false/absent should still be selected, just not ordered by. + const [query] = buildQuery({ + ...formData, + timeseries_limit_metric: 'count', + }).queries; + expect(query.metrics).toEqual(['sum__num', 'count']); + expect(query.orderby).toBeUndefined(); +}); + test('ignores residual order_by_cols from other viz types', () => { const [query] = buildQuery({ ...formData, diff --git a/superset-frontend/plugins/plugin-chart-parallel-coordinates/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-parallel-coordinates/src/buildQuery.ts index 66d13206966..d4f77a95541 100644 --- a/superset-frontend/plugins/plugin-chart-parallel-coordinates/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-parallel-coordinates/src/buildQuery.ts @@ -19,11 +19,10 @@ import { buildQueryContext, ensureIsArray, - getMetricLabel, QueryFormData, QueryFormMetric, - QueryFormOrderBy, } from '@superset-ui/core'; +import { buildSortMetricOrderby } from '@superset-ui/chart-controls'; /** * Mirrors the query the legacy `para` viz built server-side: one query @@ -35,20 +34,12 @@ import { export default function buildQuery(formData: QueryFormData) { const { timeseries_limit_metric, order_desc } = formData; return buildQueryContext(formData, baseQueryObject => { - let metrics: QueryFormMetric[] = ensureIsArray(baseQueryObject.metrics); - const orderby: QueryFormOrderBy[] = []; - const sortByMetric = ensureIsArray( - timeseries_limit_metric as QueryFormMetric | QueryFormMetric[], - )[0]; - if (sortByMetric) { - const sortByLabel = getMetricLabel(sortByMetric); - if (!metrics.some(metric => getMetricLabel(metric) === sortByLabel)) { - metrics = [...metrics, sortByMetric]; - } - if (order_desc) { - orderby.push([sortByMetric, !order_desc]); - } - } + const { metrics, orderby } = buildSortMetricOrderby({ + metrics: ensureIsArray(baseQueryObject.metrics) as QueryFormMetric[], + timeseriesLimitMetric: timeseries_limit_metric, + order_desc, + orderOnlyWhenDesc: true, + }); return [ { ...baseQueryObject, diff --git a/superset-frontend/plugins/plugin-chart-partition/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-partition/src/buildQuery.ts index e4df8a1637d..ccbf4d8431f 100644 --- a/superset-frontend/plugins/plugin-chart-partition/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-partition/src/buildQuery.ts @@ -19,11 +19,10 @@ import { buildQueryContext, ensureIsArray, - getMetricLabel, QueryFormData, QueryFormMetric, - QueryFormOrderBy, } from '@superset-ui/core'; +import { buildSortMetricOrderby } from '@superset-ui/chart-controls'; /** * Mirrors the legacy PartitionViz.query_obj (via NVD3TimeSeriesViz): a @@ -35,19 +34,12 @@ import { export default function buildQuery(formData: QueryFormData) { const { time_series_option, timeseries_limit_metric, order_desc } = formData; return buildQueryContext(formData, baseQueryObject => { - let metrics: QueryFormMetric[] = ensureIsArray(baseQueryObject.metrics); - const orderby: QueryFormOrderBy[] = []; - const sortByMetric = - ensureIsArray( - timeseries_limit_metric as QueryFormMetric | QueryFormMetric[], - )[0] ?? metrics[0]; - if (sortByMetric) { - const sortByLabel = getMetricLabel(sortByMetric); - if (!metrics.some(metric => getMetricLabel(metric) === sortByLabel)) { - metrics = [...metrics, sortByMetric]; - } - orderby.push([sortByMetric, !order_desc]); - } + const { metrics, orderby } = buildSortMetricOrderby({ + metrics: ensureIsArray(baseQueryObject.metrics) as QueryFormMetric[], + timeseriesLimitMetric: timeseries_limit_metric, + order_desc, + fallbackToFirstMetric: true, + }); return [ { ...baseQueryObject,
