sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4321110581
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/renameOperator.ts:
##########
@@ -75,7 +91,9 @@ export const renameOperator:
PostProcessingFactory<PostProcessingRename> = (
: [formData.comparison_type, metricOnly, metricWithOffset].join(
TIME_COMPARISON_SEPARATOR,
),
- metrics.length > 1 ? `${metricOnly}, ${offsetLabel}` : offsetLabel,
+ metricCountWithExtras > 1
Review Comment:
With one displayed metric `count`, dimension `region`, Split stack by
`region`, an actual-values comparison, and a Sort By metric set, the comparison
columns now come out as `count, 1 year ago`, so their `label_map` tuples are
`['count, 1 year ago', 'East']`. The stack-id lookup in `transformProps` still
reads dimension index 0 for a single displayed metric (the old bare-offset
tuple was shifted to expose the region), so every comparison series gets stack
id `count, 1 year ago` and different regions stack together. Could the
dimension index account for this metric-prefixed tuple?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -484,6 +561,95 @@ 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),
Review Comment:
With one displayed metric `count`, a dimension, Truncate Metric on, and
forecasting enabled, this fallback sums every non-axis column, including
`US__yhat` and its bounds. Rows `{US: 20, US__yhat: 1}` and `{US: 10, US__yhat:
100}` sort descending as 21 vs 110, so the axis follows the prediction instead
of observed `count`. Could this reuse the observation-only filter that
`pivotedColumnsOf` applies?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -654,16 +714,16 @@ export function sortRows(
}
});
if (
- xAxisSortSeries === SortSeriesType.Avg &&
+ aggregation === SortSeriesType.Avg &&
entries > 0 &&
aggregate !== undefined
) {
aggregate /= entries;
}
const value =
- xAxisSortSeries === SortSeriesType.Name
- ? typeof sortKey === 'string'
+ aggregation === SortSeriesType.Name
Review Comment:
With a categorical x-axis `category`, a dimension, and X-Axis Sort By set to
`category` ascending, a NULL category has already been rewritten to `<NULL>` in
`extractSeries`, so it sorts as a string and lands first (`['<NULL>', 'A']`),
while the ungrouped backend sort keeps NULL last (`['A', '<NULL>']`). Could the
raw NULL stay last in both directions here, as the metric path does?
--
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]
