fix(timeseries): correct x-axis ordering for string and numeric-like dimensions #36214
Conversation
Code Review Agent Run #e84187Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| @sfrike I’ve opened a clean PR with a minimal diff and passing checks. When you have time, I’d appreciate a review. |
| Hi maintainers! |
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect x-axis ordering in ECharts Timeseries charts when dimensions are strings or numeric-like strings (e.g., "202401", "10", "2"). The fix introduces a natural sorting algorithm that properly sorts numeric-like strings numerically rather than lexicographically, preventing the chart from connecting non-adjacent data points.
Key changes:
- Adds a
naturalComparehelper function for intelligent numeric-aware string sorting - Implements automatic series data sorting based on x-axis values
- Refactors xAxisFormatter logic to default to SMART_NUMBER for numeric axes
- Adds Boolean coercion for
connectNullsandshowValueprops
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | Implements natural sorting for series data, refactors x-axis number formatter to use SMART_NUMBER default, and adds explicit boolean coercion for chart props |
| superset-frontend/plugins/plugin-chart-echarts/spec/Timeseries/Scatter/transformProps.test.ts | Adds test coverage for x-axis time and number formatting behavior in Scatter charts |
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts Show resolved Hide resolved
| 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); | ||
| }); |
There was a problem hiding this comment.
The sorting operation is performed by mutating the series data arrays in-place for every series. This could have performance implications for charts with many series and large datasets. Additionally, the sort is performed after annotation series are added (lines 443-495), which means annotation data will also be sorted even though they may not need it.
Consider:
- Checking if the data is already sorted before performing the sort operation
- Excluding annotation series from sorting (they typically have their own data structure and ordering requirements)
- If this sorting is only needed for certain series types, add a conditional check
For example:
series.forEach((s: SeriesOption) => { // Skip annotation series - they have their own ordering if ((s as any).markLine || (s as any).markArea) return; const dataArr = (s as any).data; if (!Array.isArray(dataArr) || dataArr.length <= 1) return; // ... rest of sorting logic });| 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); | |
| }); | |
| // Helper to check if data is already sorted by x-value | |
| function isDataSortedByX( | |
| dataArr: any[], | |
| getComparableX: (raw: any) => any, | |
| ): boolean { | |
| for (let i = 1; i < dataArr.length; i++) { | |
| const prev = Array.isArray(dataArr[i - 1]) ? dataArr[i - 1][0] : dataArr[i - 1]?.x; | |
| const curr = Array.isArray(dataArr[i]) ? dataArr[i][0] : dataArr[i]?.x; | |
| if (naturalCompare(getComparableX(prev), getComparableX(curr)) > 0) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| series.forEach((s: SeriesOption) => { | |
| // Skip annotation series - they have their own ordering | |
| if ((s as any).markLine || (s as any).markArea) return; | |
| const dataArr = (s as any).data; | |
| if (!Array.isArray(dataArr) || dataArr.length <= 1) return; | |
| // extract the raw x values (support both [x,y] and { x, y } shapes) | |
| 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; | |
| }; | |
| if (!isDataSortedByX(dataArr, getComparableX)) { | |
| (s as any).data = dataArr.slice().sort((row1: any, row2: any) => { | |
| const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x; | |
| const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x; | |
| const x1 = getComparableX(rawX1); | |
| const x2 = getComparableX(rawX2); | |
| // naturalCompare already prefers numeric comparison when possible | |
| return naturalCompare(x1, x2); | |
| }); | |
| } |
| numericFormatter.id = numericFormat; | ||
| } | ||
| xAxisFormatter = numericFormatter; |
There was a problem hiding this comment.
Mutating the formatter object by adding an id property after creation is a side effect that could cause issues with shared formatter instances. If getNumberFormatter returns a cached or shared formatter instance, adding the id property will affect all usages of that formatter.
Consider one of these approaches:
- Create a wrapper function that includes the id:
const baseFormatter = getNumberFormatter(numericFormat); xAxisFormatter = Object.assign( (...args: any[]) => baseFormatter(...args), { id: numericFormat } );- Or check if the formatter already has an id before adding one:
if (!numericFormatter.id) { numericFormatter = Object.assign(() => numericFormatter(...arguments), { ...numericFormatter, id: numericFormat }); }| numericFormatter.id = numericFormat; | |
| } | |
| xAxisFormatter = numericFormatter; | |
| xAxisFormatter = Object.assign( | |
| (...args: any[]) => numericFormatter(...args), | |
| { id: numericFormat } | |
| ); | |
| } else { | |
| xAxisFormatter = numericFormatter; | |
| } |
superset-frontend/plugins/plugin-chart-echarts/spec/Timeseries/Scatter/transformProps.test.ts Outdated Show resolved Hide resolved
| // 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); |
There was a problem hiding this comment.
The getComparableX function can return either a number or a string, which could lead to inconsistent sorting behavior. When temporal parsing fails for one value but succeeds for another, you'll be comparing numbers with strings using naturalCompare.
For example:
raw1 = "invalid-date"→getComparableXreturns"invalid-date"(string)raw2 = "2021-01-01"→getComparableXreturns1609459200000(number)
When these are passed to naturalCompare, the number will be converted to a string ("1609459200000"), and you'll get lexicographic comparison instead of proper temporal sorting.
Consider normalizing the fallback to ensure consistent types:
const getComparableX = (raw: any) => { if (xAxisType === AxisType.Time) { if (typeof raw === 'number' && isFinite(raw)) return raw; const parsed = new Date(String(raw)).getTime(); // If parsing fails, return a very large number to sort invalid dates last, // or return 0 to sort them first return isFinite(parsed) ? parsed : Number.MAX_SAFE_INTEGER; } return raw; };| return isFinite(parsed) ? parsed : String(raw); | |
| // If parsing fails, return a very large number to sort invalid dates last | |
| return isFinite(parsed) ? parsed : Number.MAX_SAFE_INTEGER; |
| Thanks for the AI review! Once those updates are pushed I’ll ping maintainers for a human review. I feel these are the only necessary changes required. |
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
Let me know if you'd like any further adjustments! |
YousufFFFF force-pushed the fix/timeseries-xaxis-sorting-clean branch from 02b6bcd to 22bddde Compare | Hi! This PR is ready for a review. I’ve addressed the feedback, cleaned up the duplicate test file, updated the test cases, and resolved the issues noted in the comments. CI is passing as well. @villebro @sadpandajoe |
| Hi! Just checking in on this PR. @rusackas @sadpandajoe When you get a chance, could you please review this? |
1 participant
SUMMARY
Fixes #35853
This PR fixes incorrect x-axis ordering in the ECharts Timeseries chart when the dimension is a string or a numeric-like string (e.g. "202401", "10", "2").
Superset previously passed unordered data arrays to ECharts, causing curves to connect non-adjacent categories and produce visually incorrect line charts.
This PR adds a natural sorting step inside
transformProps.ts, ensuring that x-values are always sorted correctly before rendering. It also adds explicit boolean coercion (Boolean(...)) to stabilize props such asconnectNullsandshowValue, which could previously receive undefined or non-boolean values.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
attachments/assets/46ef6156-c89e-44f3-8725-948f453876fe)
BEFORE - AFTER DESCRIPTION
--> Before (current Superset behavior)
String or numeric-like string x-axis values (e.g., "202401", "202402", "202410") are sorted lexicographically, not numerically.
This causes the line chart to skip intermediate categories and draw incorrect zig-zag connections.
Example (Before)
X-axis order: 202401, 202403, 202407, 202410, 202411
Chart connects:
202401 → 202410 → 202407 → 202411
This creates visually wrong jumps and misleading trends.
-->After (with this patch)
The fix applies natural numeric sorting for non-temporal dimensions.
"202401", "202402", … "202410" now sort as proper numbers, not strings.
Example (After)
X-axis order: 202401, 202402, 202403, 202404, 202405, 202406...
Chart now connects points in correct numeric order, producing a smooth and correct line.
TESTING INSTRUCTIONS
"202401", "202402", "202403""1", "2", "10""A02", "A10", "A03"ADDITIONAL INFORMATION
Files touched:
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.tsThe change is self-contained, does not modify schemas or UI, and should have no side effects beyond improving x-axis correctness.