rusackas commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4368748646
##########
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:
@sadpandajoe good catch. A sort-only metric means more than one metric is
pivoted, so the offset never leads the tuple; the leading-offset branch is now
skipped in that case and the `metricOffsetLabels` branch handles it. I did not
add the grouped fixture you described.
##########
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:
@sadpandajoe yes, the guard now looks up the same resolved key
(`verboseMap[label] ?? label`) that gets pushed. No new test for this one yet.
##########
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:
@sadpandajoe agreed, `extractExtraMetrics` now skips the metric when
`x_axis_sort` matches the x-axis column label, same precedence as the control.
Added a test.
--
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]
