rusackas commented on code in PR #36214:
URL: https://github.com/apache/superset/pull/36214#discussion_r4356692012
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -546,6 +585,84 @@ export default function transformProps(
xAxisDataType,
seriesType,
);
+ // sortOperator (buildQuery.ts) already orders the backend rows by a
+ // chosen metric when x_axis_sort names one other than the x-axis column
+ // itself, for every series type here (not just Bar) - but only when
+ // groupby is empty, the same condition it checks before emitting that
+ // sort. Natural-sorting by x-value on top of that would silently discard
+ // the explicit "sort categories by metric" choice, so skip it whenever
+ // the backend would actually have applied that sort.
+ const xAxisSortedByMetric =
+ isDefined(xAxisSort) &&
+ isDefined(xAxisSortAsc) &&
+ xAxisSort !== rawXAxisLabel &&
Review Comment:
Good catch, and this is already handled on the current head: the skip
condition now also covers `x_axis_sort` naming the x-axis column itself (same
condition `sortOperator` uses), so an explicit descending order is kept.
There's a test for it (`skips the natural sort when x_axis_sort explicitly
orders the x-axis column itself, descending`).
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -4637,3 +4637,207 @@ test('tooltip keeps the metric format on a Difference
time comparison', () => {
expect(result).toContain('$ 100');
expect(result).toContain('$ 25');
});
+
+describe('natural sort of a category x-axis (#35853)', () => {
+ function categoryChartProps(
+ records: { dim: string; metric: number }[],
+ formDataOverrides: Partial<EchartsTimeseriesFormData> = {},
+ ) {
+ return createTestChartProps({
+ formData: {
+ metrics: ['metric'],
+ groupby: [],
+ x_axis: 'dim',
+ ...formDataOverrides,
+ },
+ queriesData: [
+ createTestQueryData(records, {
+ colnames: ['dim', 'metric'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ }),
+ ],
+ });
+ }
+
+ test('reorders numeric-like string dimension values numerically, not
lexicographically', () => {
+ // Backend returned these out of order; lexicographic order would put
Review Comment:
Agreed, and the current head already has this: variable-width `['10', '2',
'1']` asserting `['1', '2', '10']`, plus the descending `x_axis_sort` on the x
column case.
##########
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:
Yep, that's a real problem. Pushed d5f499a52: the natural sort is now
skipped when `rolling_type` is set (cumsum or a rolling window), so the
backend's order-dependent results line up with what's drawn. Added a test with
a cumsum on `'1'`, `'10'`, `'2'`. I haven't been able to run jest locally on
this one, so leaning on CI.
##########
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:
Right, `rebaseSeriesData` was comparing x as strings. Pushed d5f499a52: when
the baseline x exactly matches a point (always the case for category axes), it
now finds the baseline by position in the displayed data instead of `<=`; the
old comparison stays as the fallback for continuous axes. Added a test with
`'1'`, `'2'`, `'10'` rebased to `'2'` giving `-0.5, 0, 4`. I didn't add the
full `transformProps` to `rebaseSeriesData` boundary test.
--
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]
