sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4368897665
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/utils/extractExtraMetrics.ts:
##########
@@ -18,20 +18,38 @@
*/
import {
ensureIsArray,
+ getColumnLabel,
getMetricLabel,
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:
Now that the sort-only metric is queried with dimensions too, it is pivoted
alongside the displayed metric and reaches `contributionOperator` before
`transformProps` hides it. In row mode the backend divides by the sum of every
numeric column, so the hidden metric is in the denominator. For example, with
`count` grouped by `platform` (X=30, Y=20 for one category), Row contribution,
and a Sort By metric `sales` (X=300, Y=200), the displayed contributions become
5.45%/3.64% instead of 60%/40%, just from changing the axis sort. Could the
sort-only columns be excluded from the contribution step (for instance via the
operator's `columns` option) and a test added with row contribution plus a
dimension and sort-by metric?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -429,16 +432,42 @@ export default function transformProps(
// derived comparison row from a base row whose dimension value happens to
read like
// the offset, and it is gone from `labelMap` by the time the formatters run.
const derivedComparisonSeries = new Set<string>();
+ // The `<metric>, <offset>` labels `renameOperator` generates when more than
one
+ // metric (displayed or sort-only) is pivoted with an actual-values
comparison.
+ const sortOnlyMetrics = extractExtraMetrics(chartProps.rawFormData);
+ const metricOffsetLabels = new Set(
+ [...ensureIsArray(metrics), ...sortOnlyMetrics].flatMap(metric =>
+ ensureIsArray(timeCompare).map(
+ offset => `${getMetricLabel(metric)}, ${offset}`,
+ ),
+ ),
+ );
const labelMap: { [key: string]: string[] } = Object.entries(
label_map,
).reduce((acc, entry) => {
if (
entry[1].length > groupBy.length &&
Array.isArray(timeCompare) &&
+ // a sort-only metric means more than one metric is pivoted, so the
+ // offset never leads the entry; a leading offset-like part is then a
+ // hidden metric that happens to share the offset's label
+ sortOnlyMetrics.length === 0 &&
Review Comment:
This `sortOnlyMetrics.length === 0` guard has no test that fails without it.
In `xAxisSortWithDimensions.test.ts`, a grouped fixture with displayed `count`,
a hidden sort metric labeled exactly `1 year ago`, a Values comparison against
`1 year ago`, and label_map tuples `['count','East']`, `['count, 1 year
ago','East']`, `['1 year ago','East']` would cover it. With hidden values A=1,
B=10 it could assert axis order B/A and that only the two displayed series
render. Today's "sorts the axis by the sort-only metric with a time comparison"
case uses `SUM(na_sales)`, so it keeps passing if this guard is dropped. Could
that case be added?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +493,74 @@ 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[verboseMap[label] ?? label]?.length === groupBy.length
Review Comment:
The `verboseMap[label] ?? label` key lookup in this collision guard is not
exercised by any test. The existing verbose-name sort test has no colliding
dimension value, and the dimension-collision tests have no `verboseMap`, so
reverting this lookup to `labelMap[label]` would still pass. A case with two
dimensions, `truncate_metric`, hidden saved metric `sales` with `verboseMap: {
sales: 'PS4, US' }`, and a displayed `PS4, US` series (label_map
`['PS4','US']`) could assert that series stays visible and the axis orders by
the hidden totals. Could one be added?
--
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]
