sadpandajoe commented on code in PR #36214:
URL: https://github.com/apache/superset/pull/36214#discussion_r4130747485
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -311,6 +311,44 @@ function getMaxStackedValueByStack(
return max;
}
+// A plain integer (optionally signed) is only exactly representable as a JS
+// number up to Number.MAX_SAFE_INTEGER (2^53 - 1); beyond that, Number()
+// can silently collapse distinct values to the same float, e.g.
+// "9007199254740993" and "9007199254740992" - so naturalCompare below
+// switches such pairs to BigInt comparison instead.
+const INTEGER_LIKE = /^-?\d+$/;
+
+// ----- natural sort helper -----
+// Try numeric comparison first for numeric-like strings, fallback to
localeCompare.
+function naturalCompare(a: any, b: any): number {
+ const sa = a === undefined || a === null ? '' : String(a);
+ const sb = b === undefined || b === null ? '' : String(b);
+
+ // Handle empty strings explicitly so they are not treated as 0
+ if (sa === '' && sb === '') return 0;
+ if (sa === '') return -1;
+ if (sb === '') return 1;
+
+ if (INTEGER_LIKE.test(sa) && INTEGER_LIKE.test(sb)) {
Review Comment:
This comparator isn't transitive across the three branches it can take: for
the values `"2"`, `"10"`, and `"15x"` (all plausible raw values of one
categorical dimension), it returns `"2" < "10"` (both match `INTEGER_LIKE`,
compared as `BigInt`), then `"10" < "15x"` and `"15x" < "2"` (either side fails
the regex, so both fall through to `localeCompare`) — a 3-way cycle.
`Array.prototype.sort` has no defined result for a non-transitive comparator,
so the on-screen order for a category axis mixing purely-numeric-looking values
with other strings depends on the sort algorithm's internal comparison sequence
rather than a consistent rule, and can still connect non-adjacent categories —
the exact symptom #35853 reports. Would falling back to a single ordering
strategy per sort (e.g. only using the numeric/BigInt path when it can be
applied consistently across every pair, otherwise always comparing as strings)
restore a real total order?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -454,12 +492,43 @@ export default function transformProps(
const rebasePercentChange = Boolean(
(formData as { rebasePercentChange?: boolean }).rebasePercentChange,
);
- const rebasedData = rebasePercentChange
+ const unsortedRebasedData = rebasePercentChange
Review Comment:
`rebaseToPercentChange` (called here, before the natural sort below) picks
each series' 0%-baseline as its first non-null value in the original, pre-sort
row order returned by the backend. Once the category axis gets naturally sorted
afterward for display, the baseline row is often no longer the
leftmost/first-displayed category, so the chart's visually-first point won't
actually read 0% — inconsistent with the interactive drag-to-rebase
(`rebaseSeriesData` in this plugin's `percentChange.ts`), which does pick its
baseline by displayed x-position. Should the baseline be computed from the
already-sorted data instead, to match what's on screen?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -454,12 +492,43 @@ export default function transformProps(
const rebasePercentChange = Boolean(
(formData as { rebasePercentChange?: boolean }).rebasePercentChange,
);
- const rebasedData = rebasePercentChange
+ const unsortedRebasedData = rebasePercentChange
? // the same temporal-alias fallback extractSeries applies, so a chart
// with no explicit x-axis cannot have its x column rebased as data
rebaseToPercentChange(forecastRebasedData, xAxisLabel || DTTM_ALIAS)
: forecastRebasedData;
const isHorizontal = orientation === OrientationType.Horizontal;
+ const xAxisDataType = dataTypes?.[xAxisLabel] ?? dataTypes?.[xAxisOrig];
+ const xAxisType = getAxisType(
+ stack,
+ xAxisForceCategorical,
+ xAxisDataType,
+ seriesType,
+ );
+ // 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, is the single
+ // point of truth for every consumer downstream (series data in every
+ // shape extractSeries/transformSeries can produce, stacked totals, the
+ // legend) 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 rebasedData =
+ xAxisType === AxisType.Category &&
Review Comment:
This sort runs unconditionally for every Category-axis, non-Bar series,
including when the query was built with `x_axis_sort` set to a metric.
`sortOperator` (wired into this same plugin's `buildQuery.ts`, for every series
type here, not only Bar) already orders the backend rows by that metric when
configured. Once the rows reach this natural sort, though, they get silently
re-ordered by x-value instead, discarding that explicit "sort categories by
metric" choice for any Line/Area/Scatter chart that uses it. Should this sort
skip when `x_axis_sort` names something other than the x-axis column itself?
--
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]
