sadpandajoe commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4291083608
##########
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 currency metric `revenue`, a dimension `region`, a
Percentage time comparison, and the Sort By metric set to a different metric,
counting the hidden sort metric here renames the comparison column to `revenue,
1 year ago` instead of the bare offset. Its `label_map` entry becomes
`["revenue, 1 year ago", "East"]`, which no longer leads with the offset, so
`isDerivedComparisonSeries` in `transformProps` returns false and the tooltip
applies the currency formatter: a change from 100 to 125 shows `$0.25` instead
of `25%`. The new sort-with-comparison fixture in
`xAxisSortWithDimensions.test.ts` also injects the old `1 year ago, PS4` shape,
so it would not catch this. Should the derived-series check recognize the
`<metric>, <offset>` shape (or the rename keep the bare offset when the sort
metric is the only extra), with a Percentage case in that fixture using the
shape this operator actually emits?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -484,6 +540,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 SortSeriesType.Name;
Review Comment:
Mapping an explicit column selection to `SortSeriesType.Name` reuses the
aggregate's comparator, which lowercases string keys in `sortRows`, while the
ungrouped path sorts via the backend, case-sensitively. For a saved chart with
`x_axis_sort` set to the x-axis column, ascending, two metrics, no dimension,
and categories `B` and `a`, the order changes from `B, a` to `a, B` after this
lands; a single-metric chart keeps `B, a`, so adding a dimension or metric
still reorders the axis, contrary to the PR's stated invariant. Should the
column sort use its own comparator that matches the backend ordering instead of
the `Name` aggregate?
--
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]
