rusackas commented on code in PR #36214:
URL: https://github.com/apache/superset/pull/36214#discussion_r4078312493
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -373,6 +390,37 @@ export default function transformProps(
}
});
+ // ----- ensure series data are sorted naturally on the x-value -----
+ // Run after all series have been created so each series.data is complete.
+ series.forEach((s: SeriesOption) => {
+ const dataArr = (s as any).data;
+ if (!Array.isArray(dataArr) || dataArr.length <= 1) return;
+
+ (s as any).data = dataArr.sort((row1: any, row2: any) => {
+ // extract the raw x values (support both [x,y] and { x, y } shapes)
+ const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x;
+ const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x;
+
+ // If this chart's x-axis is temporal, coerce to timestamps (numbers)
for sorting.
+ // Fallback to original raw values if parsing fails.
+ const getComparableX = (raw: any) => {
+ if (xAxisType === AxisType.Time) {
+ // If it's already a number, use it. Otherwise try to coerce to Date
timestamp.
+ if (typeof raw === 'number' && isFinite(raw)) return raw;
+ const parsed = new Date(String(raw)).getTime();
+ return isFinite(parsed) ? parsed : String(raw);
+ }
+ return raw;
+ };
+
+ const x1 = getComparableX(rawX1);
+ const x2 = getComparableX(rawX2);
+
+ // naturalCompare already prefers numeric comparison when possible
+ return naturalCompare(x1, x2);
+ });
Review Comment:
The whole post-hoc sort is gone now, replaced with a single sort of the raw
records before extractSeries builds anything (annotations included). Even
before this rewrite the sort ran ahead of where annotation series get pushed,
so this one was already moot, but it's a non-issue either way now.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -483,12 +531,28 @@ export default function transformProps(
xAxisDataType === GenericDataType.Temporal
? getTooltipTimeFormatter(tooltipTimeFormat)
: String;
- const xAxisFormatter =
- xAxisDataType === GenericDataType.Temporal
- ? getXAxisFormatter(xAxisTimeFormat)
- : xAxisDataType === GenericDataType.Numeric
- ? getNumberFormatter(xAxisNumberFormat)
- : String;
+
+ // For temporal x-axis, keep the existing time formatter behavior.
+ // For numeric x-axis use a number formatter. Default to SMART_NUMBER if
none set.
+ let xAxisFormatter:
+ | ((...args: any[]) => string)
+ | StringConstructor
+ | undefined;
+
+ if (xAxisDataType === GenericDataType.Temporal) {
+ xAxisFormatter = getXAxisFormatter(xAxisTimeFormat);
+ } else if (xAxisDataType === GenericDataType.Numeric) {
+ // use provided xAxisNumberFormat, fall back to SMART_NUMBER
+ const numericFormat = xAxisNumberFormat ?? NumberFormats.SMART_NUMBER;
+ const numericFormatter = getNumberFormatter(numericFormat) as any;
+ // Ensure formatter.id exists for tests that assert on it
+ if (!numericFormatter.id) {
+ numericFormatter.id = numericFormat;
+ }
+ xAxisFormatter = numericFormatter;
Review Comment:
Confirmed and fixed. Checked NumberFormatter's constructor, `id` is a
required field so it's always already set. The defensive backfill never
actually did anything, it just mutated a shared registry singleton
(SMART_NUMBER and friends are cached instances reused by every other chart) for
no reason. Deleted it.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -373,6 +390,37 @@ export default function transformProps(
}
});
+ // ----- ensure series data are sorted naturally on the x-value -----
+ // Run after all series have been created so each series.data is complete.
+ series.forEach((s: SeriesOption) => {
+ const dataArr = (s as any).data;
+ if (!Array.isArray(dataArr) || dataArr.length <= 1) return;
+
+ (s as any).data = dataArr.sort((row1: any, row2: any) => {
+ // extract the raw x values (support both [x,y] and { x, y } shapes)
+ const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x;
+ const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x;
+
+ // If this chart's x-axis is temporal, coerce to timestamps (numbers)
for sorting.
+ // Fallback to original raw values if parsing fails.
+ const getComparableX = (raw: any) => {
+ if (xAxisType === AxisType.Time) {
+ // If it's already a number, use it. Otherwise try to coerce to Date
timestamp.
+ if (typeof raw === 'number' && isFinite(raw)) return raw;
+ const parsed = new Date(String(raw)).getTime();
+ return isFinite(parsed) ? parsed : String(raw);
Review Comment:
getComparableX is gone entirely, the sort now runs on the raw records before
any temporal/shape handling, so there's no mixed number/string fallback left to
compare.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -535,6 +558,37 @@ export default function transformProps(
}
});
+ // ----- ensure series data are sorted naturally on the x-value -----
+ // Run after all series have been created so each series.data is complete.
+ series.forEach((s: SeriesOption) => {
+ const dataArr = (s as any).data;
+ if (!Array.isArray(dataArr) || dataArr.length <= 1) return;
+
+ (s as any).data = dataArr.sort((row1: any, row2: any) => {
+ // extract the raw x values (support both [x,y] and { x, y } shapes)
+ const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x;
+ const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x;
Review Comment:
Confirmed this was real, traced it through `applyColorByPrimaryAxis` in
transformers.ts: `transformSeries` already wraps points as `{ value: [x, y],
itemStyle }` before this code ever saw the series when colorByPrimaryAxis is
on, so `row?.x` was always undefined and colorByPrimaryAxis charts were never
actually reordered. Fixed by sorting the raw records before extractSeries
builds any per-series shape at all, so there's no shape left to special-case.
Added a regression test for this exact case.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -178,6 +178,29 @@ function getSymbolMarker(symbol: string, color: string) {
}
}
+// ----- 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;
+
+ const na = Number(sa);
+ const nb = Number(sb);
+
+ // If both parse as finite numbers, do numeric sort
+ if (isFinite(na) && isFinite(nb)) {
+ return na - nb;
+ }
Review Comment:
Fixed, naturalCompare now compares plain-integer strings as BigInt so two
16+ digit values don't collide into the same float. Added a test for it.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -178,6 +178,29 @@ function getSymbolMarker(symbol: string, color: string) {
}
}
+// ----- 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;
+
+ const na = Number(sa);
+ const nb = Number(sb);
+
+ // If both parse as finite numbers, do numeric sort
+ if (isFinite(na) && isFinite(nb)) {
+ return na - nb;
+ }
+
+ // Otherwise fallback to lexicographic
+ return sa.localeCompare(sb);
+}
Review Comment:
Added, covers the natural sort itself, the colorByPrimaryAxis object-shape
case, the BigInt precision case, and confirms Time/Bar axes are left untouched.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -535,6 +558,37 @@ export default function transformProps(
}
});
+ // ----- ensure series data are sorted naturally on the x-value -----
+ // Run after all series have been created so each series.data is complete.
+ series.forEach((s: SeriesOption) => {
+ const dataArr = (s as any).data;
+ if (!Array.isArray(dataArr) || dataArr.length <= 1) return;
+
+ (s as any).data = dataArr.sort((row1: any, row2: any) => {
+ // extract the raw x values (support both [x,y] and { x, y } shapes)
+ const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x;
+ const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x;
+
+ // If this chart's x-axis is temporal, coerce to timestamps (numbers)
for sorting.
+ // Fallback to original raw values if parsing fails.
+ const getComparableX = (raw: any) => {
+ if (xAxisType === AxisType.Time) {
+ // If it's already a number, use it. Otherwise try to coerce to Date
timestamp.
+ if (typeof raw === 'number' && isFinite(raw)) return raw;
+ const parsed = new Date(String(raw)).getTime();
+ return isFinite(parsed) ? parsed : String(raw);
+ }
+ return raw;
+ };
+
+ const x1 = getComparableX(rawX1);
+ const x2 = getComparableX(rawX2);
+
+ // naturalCompare already prefers numeric comparison when possible
+ return naturalCompare(x1, x2);
+ });
+ });
Review Comment:
Checked this specifically since it'd mean the whole fix doesn't work: this
codebase never sets an explicit `xAxis.data` category array anywhere in this
file, category axis type has no `data` key set. ECharts derives categories
directly from the series data order when none is given, so sorting the series
data (now the raw records feeding it) is sufficient, there's no separate array
to desync from.
--
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]
