sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4341990512
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts:
##########
@@ -22,15 +22,29 @@ import {
QueryFormData,
QueryFormMetric,
} from '@superset-ui/core';
+import { SortSeriesType } from '../../types';
export function extractExtraMetrics(
formData: QueryFormData,
): QueryFormMetric[] {
- const { groupby, timeseries_limit_metric, x_axis_sort, metrics } = formData;
+ const { timeseries_limit_metric, x_axis_sort, metrics, groupby } = formData;
const extra_metrics: QueryFormMetric[] = [];
- const limitMetric = ensureIsArray(timeseries_limit_metric)[0];
+ const [limitMetric] = ensureIsArray(timeseries_limit_metric);
+ // The "Sort By" limit metric is only queried when the axis is sorted by it
+ // and it is not already a value metric. This applies with dimensions too:
+ // the pivot then yields one `<limit metric>, <dimension values>` column
+ // per series, which the chart sums client-side to order the axis (the
+ // backend sort operator only handles the single-series case).
+ // With several series, `x_axis_sort` naming a `SortSeriesType` aggregate
+ // means that aggregate (the control drops a colliding metric), so a limit
+ // metric sharing its label is not a sort target and must not be queried.
+ const isMultiSeries =
+ ensureIsArray(groupby).length > 0 || ensureIsArray(metrics).length > 1;
+ const isAggregateSort =
+ isMultiSeries &&
+ Object.values<string>(SortSeriesType).includes(x_axis_sort as string);
if (
- !(groupby || []).length &&
+ !isAggregateSort &&
Review Comment:
With `x_axis: 'genre'`, `groupby: ['platform']`, `metrics: ['count']` and a
Sort By SQL metric labeled `genre`, the control dedupes the `genre` option to
the axis column, so the stored `x_axis_sort` is `'genre'`. Here
`getMetricLabel(limitMetric) === x_axis_sort` still holds, so the metric is
appended and the request carries both column `genre` and metric `genre`; the
backend rejects that with a duplicate column/metric label error and the chart
fails to load. Should this also skip the metric when `x_axis_sort` matches the
x-axis column, the same precedence the control applies?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +488,73 @@ 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
+ ? []
+ : [
+ // with dimensions the data is pivoted, so a sort-only metric has no
+ // bare column of its own; a bare key whose label-map tuple is a
+ // plain dimension tuple is a displayed series whose dimension value
+ // happens to read like the metric label and must stay visible
+ ...(groupBy.length > 0 && labelMap[label]?.length === groupBy.length
Review Comment:
With one displayed metric, dimension `platform`, Truncate Metric on, a Sort
By metric `sales` whose verbose name is `Revenue`, and a platform value that is
also `Revenue`, the displayed column is `Revenue` with `label_map['Revenue'] =
['Revenue']`. This guard looks up `labelMap[label]` (`sales`), not the key that
is actually pushed (`verboseMap[label] ?? label`, i.e. `Revenue`), so it misses
the dimension tuple and adds `Revenue` to `extraMetricLabels`. The displayed
Revenue series then disappears from the chart, legend and totals; with
displayed values Revenue=100 and Other=1 the stacked total becomes 1 instead of
101. Should the collision check use the same resolved key?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +488,73 @@ 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
+ ? []
+ : [
+ // with dimensions the data is pivoted, so a sort-only metric has no
+ // bare column of its own; a bare key whose label-map tuple is a
+ // plain dimension tuple is a displayed series whose dimension value
+ // happens to read like the metric label and must stay visible
+ ...(groupBy.length > 0 && labelMap[label]?.length === groupBy.length
+ ? []
+ : [verboseMap[label] ?? label]),
+ // forecast columns of a sort-only metric stay hidden too, even
+ // though the sort key itself ignores them
+ ...pivotedColumnsOf(label, { includeForecast: true }),
Review Comment:
With one displayed metric `count`, dimension `region`, an actual-values
comparison of `1 year ago`, and a Sort By metric labeled exactly `1 year ago`,
the hidden column's `label_map` tuple is `['1 year ago', 'East']`. The first
branch in the `labelMap` reduce (the `timeCompare.includes(entry[1][0])` one)
claims it as a derived comparison and shifts it, so the new
`metricOffsetLabels` branch is never reached and `pivotedColumnsOf` finds
nothing. The hidden column then renders as an extra series, counts toward
stacked totals, and cannot order the axis: with hidden values A=1 and B=10
descending, the categories stay A/B instead of B/A. Could the offset-leading
case be told apart from a hidden metric that shares the offset's label? The new
test at `xAxisSortWithDimensions.test.ts` ('keeps a hidden sort metric labeled
like a comparison series hidden') uses `groupby: []`, so client-side sorting is
off and a bare hidden column is excluded regardless; it would still pass if
this matching
were reverted. A grouped fixture with `['sales, 1 year ago', 'East']` tuples
asserting B/A order and no hidden series would cover it.
--
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]
