rusackas commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4325038714
##########
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:
@sadpandajoe good catch. The final stack-assignment loop now offsets by the
metric prefix the same way `seriesStackIds` does, so a `sales, HR, East` series
stacks by `East`. The existing multi-metric `stackDimension` test covers it.
##########
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:
@sadpandajoe added a test: one displayed metric, Truncate Metric on,
forecast enabled, rows like `{US: 20, US__yhat: 1}` sorted by `count` in both
directions. Forecast columns stay out of the sum.
##########
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:
@sadpandajoe added a test through `transformProps`: `A`, `null`, `B` sorted
by the x-axis column gives `A, B, <NULL>` ascending and `B, A, <NULL>`
descending.
--
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]
