rusackas commented on code in PR #36214:
URL: https://github.com/apache/superset/pull/36214#discussion_r4356693073
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -546,6 +585,85 @@ export default function transformProps(
xAxisDataType,
seriesType,
);
+ // sortOperator (buildQuery.ts) already orders the backend rows, for every
+ // series type here (not just Bar), whenever x_axis_sort/x_axis_sort_asc
+ // are both set, groupby is empty, and x_axis_sort names either a metric
+ // or the x-axis column itself - the latter via `is_sort_index`, honoring
+ // x_axis_sort_asc's direction. Natural-sorting by x-value on top of
+ // either of those would silently discard that explicit choice (including
+ // an explicit descending order on the x-axis column itself), so skip it
+ // under the exact same condition sortOperator uses to decide whether it
+ // applied a sort.
+ const xAxisSortAppliedByBackend =
+ isDefined(xAxisSort) &&
+ isDefined(xAxisSortAsc) &&
+ isEmpty(groupBy) &&
+ [
+ rawXAxisLabel,
+ ...ensureIsArray(metrics).map(getMetricLabel),
+ ...extractExtraMetrics(chartProps.rawFormData).map(getMetricLabel),
+ ].includes(xAxisSort);
+ // A category axis renders points in data-array order, not by sorted
+ // x-value: the backend can return string (and numeric-like-string, e.g.
+ // "202401") dimensions in an arbitrary order, which drew line segments
+ // that jumped between non-adjacent categories (#35853). Natural-sorting
+ // here, before extractSeries builds any per-series data and before
+ // rebaseToPercentChange below picks its baseline row, is the single
+ // point of truth for every consumer downstream (series data in every
+ // shape extractSeries/transformSeries can produce, stacked totals, the
+ // legend, and the percent-change baseline) rather than re-sorting each
+ // series' already-shaped data separately later. Time axes are
+ // unaffected: they already carry a meaningful numeric order and
+ // getAxisType never returns Category for them.
+ // Bar is excluded: discrete bars have no line-connection artifact to fix,
+ // and Bar's category order is already a deliberate, source-preserving
+ // contract independent of legend display sorting (see "should preserve
+ // source order for color-by-primary-axis legends when label sorting is
+ // enabled" in Bar/transformProps.test.ts) - forcing a natural sort here
+ // would silently override that.
+ const shouldNaturalSort =
+ xAxisType === AxisType.Category &&
+ seriesType !== EchartsTimeseriesSeriesType.Bar &&
Review Comment:
Agreed. Pushed d5f499a52: the natural sort is skipped whenever `x_axis_sort`
is set on a multi-series chart (same condition that passes `xAxisSortSeries` to
`extractSeries`), so rows keep the order the name sort and the neighbor-zero
fill expect. I didn't add a dedicated stacked-area test for this.
--
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]
