sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4202297302
##########
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 Contribution Mode set to row, a grouped chart that sorts by a Sort By
metric now queries that metric and pivots it next to the displayed ones. The
backend contribution step runs before the chart hides those columns, so its
columns are included in each row's denominator. For example, with one dimension
and a displayed COUNT of 30 and 20 across two platforms, and a hidden sales
metric of 5 and 3, the displayed bars become 30/58 and 20/58 instead of 30/50
and 20/50, and the stack no longer adds up to 100%. Before this change a
grouped chart never queried the sort-only metric, so these values were correct.
Could the sort-only columns be kept out of the contribution step, or the metric
not be queried in row contribution mode?
##########
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:
Sorting by a metric adds up its per-series columns, which only matches the
ungrouped order for additive metrics. With Sort By set to MAX(value)
descending, category A with per-dimension maxima of 100 and 0 and category B
with 60 and 60 sort as A, B without a dimension (100 > 60). After adding the
dimension they sort as B, A (120 > 100). AVG and COUNT DISTINCT break the same
way, which contradicts the description that the order does not change when a
dimension is added. Should the sum only be used for additive aggregates, or
should the rollup follow the metric's own aggregation?
--
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]
