sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4323514916
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -706,22 +885,29 @@ export default function transformProps(
// before calling extractShowValueIndexes so each group's topmost series is
// tracked independently (fixing the bug where only the last series across
// all groups was flagged to show the total label).
- // When metrics.length > 1 the label-map tuple is [metric, dim0, dim1, ...],
- // so the stackDimension sits at offset 1 + groupby.indexOf(stackDimension).
- // When there is a single metric the tuple is [dim0, dim1, ...] with no
- // metric prefix, so the offset is just groupby.indexOf(stackDimension).
+ // The label-map tuple is [metric, dim0, dim1, ...] when it carries a metric
+ // (or offset) prefix and [dim0, dim1, ...] when the metric label was
+ // truncated. The prefix length is derived per series below, so this index
+ // is only the position of the stackDimension within the groupby list.
const idxSelectedDimension =
stack === StackControlsValue.Stack &&
stackDimension &&
chartProps.rawFormData?.groupby &&
(chartProps.rawFormData.groupby as string[]).includes(stackDimension)
- ? ((metrics || []).length > 1 ? 1 : 0) +
- (chartProps.rawFormData.groupby as string[]).indexOf(stackDimension)
+ ? (chartProps.rawFormData.groupby as string[]).indexOf(stackDimension)
Review Comment:
With two displayed metrics (`sales`, `profit`), dimensions `dept, region`,
and Split stack by `region`, this index is now just the dimension's position in
`groupby` (1), but the stack-assignment loop further down
(`columnsArr?.[idxSelectedDimension]`, around line 1360) still reads it against
the full label tuple. For `['sales', 'HR', 'East']` that yields `HR`, so every
series is stacked by department instead of region, while the total labels
computed from `seriesStackIds` still group by region. This happens without any
axis sorting, so it also changes existing multi-metric stacked charts. Should
that loop reuse the same per-series prefix-aware lookup used for
`seriesStackIds`?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -484,6 +561,99 @@ export default function transformProps(
);
const isMultiSeries = groupBy.length || metrics?.length > 1;
+ // `x_axis_sort` stores either a series aggregate (a `SortSeriesType`) or
+ // the label of the x-axis column or of a metric. A single-series chart is
+ // sorted by the backend sort operator; with several series the rows are
+ // ordered here instead: by axis value, by an aggregate over every series,
+ // or by the sum of the chosen metric's columns. The field is carried
+ // through as-is, so a column or metric that happens to be named like an
+ // aggregate is never guessed at.
+ const sortSeriesTypes = new Set<string>(Object.values(SortSeriesType));
+ const resolveXAxisSortSeries = (): XAxisSortSeries | undefined => {
+ if (!isMultiSeries || typeof xAxisSort !== 'string') {
+ return undefined;
+ }
+ if (sortSeriesTypes.has(xAxisSort)) {
+ return xAxisSort as SortSeriesType;
+ }
+ if (
+ xAxisSort === xAxisLabel ||
+ xAxisSort === getXAxisLabel(chartProps.rawFormData)
+ ) {
+ return { axisValue: true };
+ }
+ const dataColumns = new Set(Object.keys(rebasedData[0] ?? {}));
+ // `truncate_metric` drops the metric label from its own pivoted columns
+ // when it is the sole displayed metric (see the comment on
+ // `pivotedColumnsOf`), so that lookup finds nothing when a chart's only
+ // metric is also the chosen sort target. With a single displayed metric,
+ // every remaining series column is that metric's own pivoted value, so
+ // fall back to summing them directly.
+ const soleValueMetricLabel = isMultiSeries
+ ? ensureIsArray(metrics).length === 1
+ ? getMetricLabel(ensureIsArray(metrics)[0])
+ : undefined
+ : undefined;
+ const isSoleValueMetricSort =
+ isDefined(soleValueMetricLabel) &&
+ (verboseMap[soleValueMetricLabel!] ?? soleValueMetricLabel) ===
+ (verboseMap[xAxisSort] ?? xAxisSort);
+ const truncatedMetricColumns =
+ isSoleValueMetricSort && !pivotedColumnsOf(xAxisSort).length
+ ? Object.keys(rebasedData[0] ?? {}).filter(
+ column =>
+ column !== xAxisLabel &&
+ !extraMetricLabels.includes(column) &&
+ // forecast trend and bounds never drive the sort key
+ extractForecastSeriesContext(column).type ===
Review Comment:
The new forecast exclusion here has no test that reaches it. The existing
forecast case sorts by a sort-only metric, so it goes through
`pivotedColumnsOf`, not this truncated-metric fallback. With one displayed
metric `count`, a dimension, Truncate Metric on and forecasting enabled, rows
like `{US: 20, US__yhat: 1}` and `{US: 10, US__yhat: 100}` sorted by `count`
descending should keep the observation order (20 before 10); if this filter
were reverted, the `__yhat` values would be summed in and flip it. Could you
add a `transformProps` case with that shape?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -676,11 +736,23 @@ export function sortRows(
};
});
- return orderBy(
- sortedRows,
- ['value'],
- [xAxisSortSeriesAscending ? 'asc' : 'desc'],
- ).map(({ row, totalStackedValue }) => ({ row, totalStackedValue }));
+ // Rows with no aggregate (every sorted column is NULL) stay last in both
+ // directions, matching the ungrouped sort.
+ const isMissing = (value: unknown) =>
+ aggregation === SortSeriesType.Name
+ ? byAxisValue && value === NULL_STRING
Review Comment:
The NULL-last handling for axis-value sorting isn't covered at the
`extractSeries` boundary. Axis values `A`, `null` and `B` sorted by the x-axis
column should come out `['A', 'B', '<NULL>']` ascending and `['B', 'A',
'<NULL>']` descending; the current NULL test only checks the conversion and the
axis-sort tests use plain strings only, so removing this branch (which puts
`<NULL>` first when ascending) would still pass. Could you add that case?
--
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]
