rusackas commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4369448530
##########
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:
Good catch @sadpandajoe. The contribution step now gets an explicit
`columns` list (displayed metrics only, including the offset variants for time
comparison) whenever a sort-only metric is queried, so it no longer lands in
the row denominator. Added operator tests for it.
##########
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:
Added a case with a hidden sort metric labeled `1 year ago` under a Values
comparison. It fails if the guard is dropped, and the axis orders B/A with only
the two displayed series.
--
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]
