sadpandajoe commented on code in PR #36214:
URL: https://github.com/apache/superset/pull/36214#discussion_r4333049563
##########
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 &&
+ !xAxisSortAppliedByBackend;
+ const naturalSortMode = classifyNaturalSortMode(
+ forecastRebasedData.map(row => row[xAxisLabel]),
+ );
+ const sortedForecastRebasedData = shouldNaturalSort
Review Comment:
Reordering here happens after the backend has already computed
order-dependent post-processing (cumulative sum and rolling windows, via
`rollingWindowOperator`) over the pivot's lexicographic row order. With an
ad-hoc string x-axis (categories `'1'`, `'10'`, `'2'`, metric `1` each) and
cumulative sum enabled, the backend returns `('1',1), ('10',2), ('2',3)`; this
sort now renders `('1',1), ('2',3), ('10',2)`, so a running total of positive
values visibly decreases and the rolling values no longer correspond to the
displayed neighbors. Should the reorder be skipped when a rolling/cumulative
operator is active (or the ordering moved ahead of that computation)?
##########
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:
`extractSeries` imputes the stacked-area neighbor zero (`fillNeighborValue`)
by indexing `rows[idx ± 1]` while the output follows `sortedRows` from
`sortRows`. Previously both orders were lexicographic for string categories, so
they agreed; now `rows` is naturally sorted while an explicit multi-series
`x_axis_sort_series` by name re-sorts lexicographically. For stacked area with
groupby, categories `1, 2, 10`, series A = `[null, 5, null]` with sort by name,
the zero-fill lands on `'1'` instead of `'10'` (A renders `[0, null, 5]` rather
than `[null, 0, 5]`), so the area is drawn incorrectly. Should the natural sort
be skipped when `xAxisSortSeries` is set for a multi-series chart, as it is for
the backend sort?
##########
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 &&
+ !xAxisSortAppliedByBackend;
+ const naturalSortMode = classifyNaturalSortMode(
+ forecastRebasedData.map(row => row[xAxisLabel]),
+ );
+ const sortedForecastRebasedData = shouldNaturalSort
+ ? [...forecastRebasedData].sort((row1, row2) =>
+ naturalCompare(row1[xAxisLabel], row2[xAxisLabel], naturalSortMode),
+ )
+ : forecastRebasedData;
+ // rebaseToPercentChange picks each series' 0%-baseline as its first row
+ // in whatever order it receives - running it after the natural sort
+ // above means that baseline matches the sorted, on-screen leftmost
+ // category, consistent with the interactive drag-to-rebase
+ // (rebaseSeriesData in this plugin's percentChange.ts), which rebases
+ // from the displayed x-position too.
+ const rebasedData = rebasePercentChange
Review Comment:
After this sort the displayed category order is natural, but the interactive
drag-to-rebase (`rebaseSeriesData` in `percentChange.ts`) still finds its
baseline with a string comparison (`x <= baselineX`). With data `['1',0],
['2',1], ['10',9]` and a drag to `'2'`, `'10' <= '2'` is also true
lexicographically, so `'10'` wins as the baseline: `'2'` shows -80% instead of
0% and `'10'` shows 0% instead of +400%. Before this change the data order
matched the lexicographic comparison, so it worked. Could the rebase use the
same natural comparison, and could a test cover the `transformProps` →
`rebaseSeriesData` boundary with `'1'`, `'2'`, `'10'` (rebasing to `'2'` should
give `-0.5, 0, 4`)?
--
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]
