You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "nireknj (via GitHub)" <gi...@apache.org> on 2026/03/24 13:47:26 UTC
[PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
nireknj opened a new pull request, #38828:
URL: https://github.com/apache/superset/pull/38828
### SUMMARY
The **Time-series Table** visualization was previously locked to sorting only by the first column (dimensions). Clicking on metric headers (time-series values) displayed a sort indicator but did not actually re-order the rows.
This was caused by a failure in `react-table` value resolution when using complex/nested accessor keys (e.g., date-prefixed metric keys). The custom [sortNumberWithMixedTypes](cci:1://file:///c:/Users/Nirek%20Jaiswal/superset/superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:50:0-110:1) utility was returning `0` (equality) because it couldn't resolve the cell data from the standard `row.values` lookup.
**Changes introduced in this PR:**
1. **Metric Resolution Fix:** Updated [sortNumberWithMixedTypes](cci:1://file:///c:/Users/Nirek%20Jaiswal/superset/superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:50:0-110:1) to resolve cell values from `row.original` as a fallback. This reliably sidesteps nested dot-notation parsing issues in React Table.
2. **String Fallback:** Enhanced the [compareValues](cci:1://file:///c:/Users/Nirek%20Jaiswal/superset/superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:20:0-48:1) helper to include an alphabetical fallback for dimension columns and mixed string-number data, preventing them from being treated as equivalent `NaN` values.
3. **Cross-Platform CI Support:** Fixed a regex in [jest.config.js](cci:7://file:///c:/Users/Nirek%20Jaiswal/superset/superset-frontend/jest.config.js:0:0-0:0) that used leading slashes incompatible with Windows-style paths, allowing unit tests to run correctly on all OS environments.
### TESTING INSTRUCTIONS
1. Open a "Time-series Table" chart in Explore or a Dashboard.
2. Click any metric column header (e.g., a date-prefixed metric).
3. Verify that the table correctly re-orders rows numerically.
4. Toggle the sort order (ASC/DESC) and verify consistency.
5. **Automated Verification:** All 14 tests (original + regression suite) passed. Run:
`npm run test -- src/visualizations/TimeTable/utils/sortUtils/`
### ADDITIONAL INFORMATION
- [x] Changes UI
- [x] Includes unit tests
Signed-off-by: Nirek Jaiswal <ni...@gmail.com>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38828:
URL: https://github.com/apache/superset/pull/38828#issuecomment-5563915693
Agreed with @gabotorresruiz's read here, the sorting rework in #38579 and #41976 already replaced what this touches, and the dotted accessor keys this targets can't come from a real chart config anyway. Closing this out, but if you can still reproduce it on current `master`, happy to reopen with repro steps.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on PR #38828:
URL: https://github.com/apache/superset/pull/38828#issuecomment-4137352981
Hey @nireknj thanks for getting into this. Can you please add some steps to reproduce this bug? I wasn't able to reproduce this in `master` so I'm not sure if this fix is 100% necessary at this moment. Maybe is an old bug that was already fixed? Please check and let me know
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #38828: fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics
URL: https://github.com/apache/superset/pull/38828
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on PR #38828:
URL: https://github.com/apache/superset/pull/38828#issuecomment-5294676349
Hi @nireknj, following up on my earlier question since this has been open for a while. I did a deeper pass and I believe this PR has been overtaken by fixes that already landed on master:
- The sorting rework in #38579 (merged 2026-03-19, a few days before this PR was opened) and #41976 (merged 2026-07-20) rewrote `sortUtils.ts` and `ValueCell`. `ValueCell` now receives a precomputed `value` prop instead of `{ valueField, column, reversedEntries }`, so the shape your regression tests mock no longer exists. I verified it on this branch: all 5 tests in `sortUtils.regression.test.ts` pass here but fail when run against master's current `sortUtils.ts`, while master's own 9-test suite passes. Both `sortUtils.ts` and `jest.config.js` also conflict with master at this point.
- The Windows `testRegex` fix landed separately in #41863 (2026-07-21), keeping the `superset-frontend` prefix mandatory, so the `jest.config.js` change here is no longer needed. I think that also settles @rusackas's question above.
- On the root cause: Time-series Table column ids come from `CollectionControl`'s `itemGenerator`, which is `nanoid(11)` (`src/explore/components/controls/CollectionControl/index.tsx`), and that alphabet contains no dots. So the dotted accessor keys like `2020-01-01.metric` that this fix targets can't be produced by a real chart config, which matches me not being able to reproduce the bug back in March.
- Small note in case any of this gets reused: `pre-commit` is red because `sortUtils.ts` ends with two blank lines and prettier rejects that.
Given all of the above I'd suggest closing this one. If you can still reproduce broken sorting on current master, please share the repro steps and the chart config and I'll happily dig in with you. Thanks for the work you put into this!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #38828:
URL: https://github.com/apache/superset/pull/38828#discussion_r2989594105
##########
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:
##########
@@ -36,7 +36,12 @@ function compareValues(
const isAValid = numA !== null && numA !== undefined && !Number.isNaN(numA);
const isBValid = numB !== null && numB !== undefined && !Number.isNaN(numB);
- if (!isAValid && !isBValid) return 0;
+ if (!isAValid && !isBValid) {
+ if (typeof a === 'string' && typeof b === 'string') {
+ return a < b ? -1 : a > b ? 1 : 0;
+ }
+ return 0;
Review Comment:
`compareValues` can return `0` for mixed "invalid numeric" pairs like `(a: 'Apple', b: null)` because only the string–string case is ordered. That makes the comparator non-transitive (e.g. 'Apple' < 'Banana', but both equal `null`), which can lead to unstable/incorrect ordering in `react-table`. Consider defining a total order for the “both invalid” branch (e.g., rank `null/undefined` per `nanTreatment`, then compare remaining values via `String(a).localeCompare(String(b))`).
```suggestion
const isANullish = a === null || a === undefined;
const isBNullish = b === null || b === undefined;
if (isANullish || isBNullish) {
if (isANullish && isBNullish) {
return 0;
}
if (nanTreatment === 'asSmallest') {
return isANullish ? -1 : 1;
}
// For both 'asLargest' and 'alwaysLast', place nullish values after other invalids
return isANullish ? 1 : -1;
}
// For other invalid pairs, fall back to a deterministic string comparison
return String(a).localeCompare(String(b));
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #38828:
URL: https://github.com/apache/superset/pull/38828#issuecomment-4118464995
## **Sequence Diagram**
This PR fixes metric-column sorting by making the comparator read cell data from the original row when standard lookup fails, then applying numeric comparison with string fallback. As a result, clicking metric headers now correctly reorders rows for complex time-series column keys.
```mermaid
sequenceDiagram
participant User
participant TimeSeriesTable
participant ReactTable
participant SortUtils
User->>TimeSeriesTable: Click metric column header
TimeSeriesTable->>ReactTable: Request sort for selected metric column
ReactTable->>SortUtils: Compare rows for column
alt Cell found in original row
SortUtils->>SortUtils: Resolve cell from row original
else Otherwise
SortUtils->>SortUtils: Resolve cell from row values
end
SortUtils->>SortUtils: Compare as numbers with string fallback
SortUtils-->>ReactTable: Return comparison result
ReactTable-->>TimeSeriesTable: Render sorted rows
```
---
*Generated by [CodeAnt AI](https://codeant.ai)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38828:
URL: https://github.com/apache/superset/pull/38828#issuecomment-4118457722
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #38828:
URL: https://github.com/apache/superset/pull/38828#issuecomment-4128064508
@nireknj can you add before and after screenshots to your PR summary?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-table): allow numerical sorting on Time-series Table metrics [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38828:
URL: https://github.com/apache/superset/pull/38828#issuecomment-4753296542
@nireknj the sortUtils fix itself looks reasonable, but this also rewrites the global `jest.config.js` `testRegex`. Why does a sort fix need to touch the repo-wide matcher? I'd rather keep that out unless it's strictly necessary.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org