rusackas commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4289138664
##########
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:
This is the same contribution-mode caveat from the description, just
reachable with dimensions now since a grouped chart never queried the sort-only
metric before this PR. Expanded that note to call out the grouped case
explicitly. Isolating it properly means threading a column exclusion through
the backend contribution operator, which felt like scope creep here, so left it
for the same follow-up.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -564,13 +564,39 @@ export function sortAndFilterSeries(
).map(({ name }) => name);
}
+/**
+ * Sum of a specific set of columns, used to order the rows of a multi-series
+ * chart by one metric once dimensions have pivoted that metric into a column
+ * per series.
+ */
+export type XAxisSortBySumOfColumns = { sumOfColumns: string[] };
+
+/**
+ * How the rows (one per x-axis value) of a multi-series chart are ordered:
+ * a `SortSeriesType` aggregates every numeric column in the row, while
+ * `XAxisSortBySumOfColumns` only sums the named columns.
+ */
+export type XAxisSortSeries = SortSeriesType | XAxisSortBySumOfColumns;
+
+export function isXAxisSortBySumOfColumns(
+ sort: XAxisSortSeries,
+): sort is XAxisSortBySumOfColumns {
+ return typeof sort === 'object';
+}
+
export function sortRows(
rows: DataRecord[],
totalStackedValues: number[],
xAxis: string,
- xAxisSortSeries: SortSeriesType,
+ xAxisSortSeries: XAxisSortSeries,
xAxisSortSeriesAscending: boolean,
) {
+ const sumOfColumns = isXAxisSortBySumOfColumns(xAxisSortSeries)
+ ? new Set(xAxisSortSeries.sumOfColumns)
+ : undefined;
+ const aggregation: SortSeriesType = sumOfColumns
Review Comment:
You're right that it was always summing regardless of the metric's own
aggregate. Fixed MIN/MAX to roll up with MIN/MAX instead, which does recover
the right ungrouped order since those are decomposable the same way SUM is. AVG
and COUNT DISTINCT aren't reconstructable this way from already-aggregated
per-dimension values though, no amount of client-side rollup fixes that, so
documented it as a known limitation instead of faking a fix.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -464,14 +466,49 @@ 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.
+ const pivotedColumnsOf = (metricLabel: string): 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),
Review Comment:
Good catch, fixed. label_map is built after Prophet runs, so the
yhat/yhat_lower/yhat_upper columns have the same shape as a metric's pivoted
ones and were sneaking into the sum. Excluded them the same way they're already
excluded from rendering, with 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]
