sadpandajoe commented on code in PR #36214:
URL: https://github.com/apache/superset/pull/36214#discussion_r4261500429
##########
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:
This guard skips the natural sort only when `x_axis_sort` names a metric,
but `x_axis_sort` equal to the x-axis column is a valid choice that
`sortOperator` honors with its `x_axis_sort_asc` direction. With a single
metric, no groupby, `x_axis_sort: 'dim'`, `x_axis_sort_asc: false`, the backend
returns `C, B, A` and this sort turns it back into `A, B, C`, so an explicit
descending order is silently discarded (same for a force-categorical numeric
axis). With multiple metrics, `extractSeries` also re-sorts rows by name
afterwards, so the percent-change baseline picked here no longer matches the
first displayed category. Should the sort be skipped whenever
`x_axis_sort`/`x_axis_sort_asc` are set, and only apply when the user hasn't
chosen an order?
##########
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:
This fixture can't tell numeric from lexicographic ordering: `202401`,
`202402`, `202410` are the same width, so plain `localeCompare` also yields
`202401, 202402, 202410` (the comment above is inaccurate: lexicographically
`202402` sorts before `202410`). The headline case in the PR description (`1,
2, 10`) is never asserted in integer mode, so replacing the BigInt comparison
with a string compare would keep every test green. Could this use
variable-width values like `['10', '2', '1']` and assert `['1', '2', '10']`? A
descending `x_axis_sort` on the x column would also be worth a case here.
--
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]
