codeant-ai-for-open-source[bot] commented on code in PR #41976:
URL: https://github.com/apache/superset/pull/41976#discussion_r3566566970
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -98,9 +108,9 @@ const TimeTable = ({
...acc,
[columnConfig.key]: (
<ValueCell
- valueField={valueField}
+ value={value}
+ errorMsg={errorMsg}
column={columnConfig}
- reversedEntries={reversedEntries}
/>
Review Comment:
**Suggestion:** The sort helper still derives sortable values from
`cell.props.valueField` and `cell.props.reversedEntries`, but this refactor now
renders `ValueCell` with only `value`, `errorMsg`, and `column`. As a result
`sortNumberWithMixedTypes` cannot reconstruct comparison values and falls back
to returning 0, so sorting for these columns becomes incorrect/no-op. Keep the
previous sort contract (pass the required props) or update the sort helper to
sort directly from the precomputed numeric value. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical ๐จ</summary>
```mdx
- โ Time-series Table numeric columns do not sort correctly.
- โ ๏ธ Users see unchanged ordering when clicking computed headers.
- โ ๏ธ Comparison/contribution/average metrics show misleading ordering.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Configure a chart with viz type "time_table", which is implemented by the
backend
TimeTableViz (superset/viz.py:757โ763, viz_type = "time_table") and the
frontend TimeTable
plugin (superset-frontend/src/visualizations/TimeTable/index.ts:29โ36, which
loads
./TimeTable as the chart component).
2. When the Time-series Table renders, the TimeTable component at
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:50โ77 builds its
memoizedColumns array, setting `sortType: sortNumberWithMixedTypes` for each
data column
(line 75). TableView
(packages/superset-ui-core/src/components/TableView/TableView.tsx:23โ34)
passes this
sortType into react-table so header clicks invoke `sortNumberWithMixedTypes`.
3. For each row, TimeTable computes `memoizedRows` at TimeTable.tsx:81โ126.
Inside the
reducer, it calls `calculateCellValue(valueField, columnConfig,
reversedEntries)` (lines
88โ92) and then renders a ValueCell with the result: `<ValueCell
value={value}
errorMsg={errorMsg} column={columnConfig} />` at lines 109โ114. The ValueCell
implementation in components/ValueCell/ValueCell.tsx:24โ37 defines props `{
value,
errorMsg, column }` only; it no longer exposes `valueField`, `entries`, or
`reversedEntries`.
4. When a user clicks a computed numeric column header to sort, react-table
calls
`sortNumberWithMixedTypes` from utils/sortUtils/sortUtils.ts:57โ107. That
function reads
`row.values[columnId]` and assumes the underlying React element has props `{
valueField,
column, reversedEntries or entries }` (see comments and propsA/propsB shape
at lines
65โ83). For ValueCell, `propsA.valueField`, `propsA.reversedEntries`, and
`propsA.entries`
are all undefined, so `reversedEntriesA`/`reversedEntriesB` are undefined
and the guard at
lines 91โ93 returns `0`. Returning 0 for every comparison makes sorting a
no-op, so
Time-series Table numeric/computed columns cannot actually be reordered
despite the UI
allowing sort toggles.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=39a0175a0a6b4104814ea612a3f9568d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=39a0175a0a6b4104814ea612a3f9568d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/visualizations/TimeTable/TimeTable.tsx
**Line:** 110:114
**Comment:**
*Api Mismatch: The sort helper still derives sortable values from
`cell.props.valueField` and `cell.props.reversedEntries`, but this refactor now
renders `ValueCell` with only `value`, `errorMsg`, and `column`. As a result
`sortNumberWithMixedTypes` cannot reconstruct comparison values and falls back
to returning 0, so sorting for these columns becomes incorrect/no-op. Keep the
previous sort contract (pass the required props) or update the sort helper to
sort directly from the precomputed numeric value.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41976&comment_hash=835a6da3f9249b32c318a9020a0f024efd6dff62d5d447df616bb406a8353531&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41976&comment_hash=835a6da3f9249b32c318a9020a0f024efd6dff62d5d447df616bb406a8353531&reaction=dislike'>๐</a>
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -81,6 +85,12 @@ const TimeTable = ({
const valueField = row.label || row.metric_name || '';
const cellValues = columnConfigs.reduce<Record<string, ReactNode>>(
(acc, columnConfig) => {
+ const { value, errorMsg } = calculateCellValue(
+ valueField,
+ columnConfig,
+ reversedEntries,
+ );
+
if (columnConfig.colType === 'spark') {
Review Comment:
**Suggestion:** Cell value computation now runs for every column before
checking `colType`, so sparkline columns pay an unnecessary
`calculateCellValue` call whose result is discarded. On large tables this adds
avoidable per-row/per-column work; move calculation inside the non-spark branch
so only rendered numeric cells are computed. [performance]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Time-series Table with sparklines does redundant per-cell work.
- โ ๏ธ Large time_table charts incur unnecessary CPU overhead.
- โ ๏ธ No functional bug, only performance inefficiency.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Render a Time-series Table visualization using the TimeTable plugin
(superset-frontend/src/visualizations/TimeTable/index.ts:29โ36), configured
with at least
one sparkline column (the primary design of this viz type as described in
its metadata at
index.ts:8โ18).
2. In the TimeTable component, `memoizedRows` is computed at
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:81โ126. For
each row, it
reduces over `columnConfigs` (lines 86โ118) to build the per-column cell
values.
3. Inside the reducer, the code unconditionally computes `{ value, errorMsg
} =
calculateCellValue(valueField, columnConfig, reversedEntries)` at lines
88โ92, even before
checking `columnConfig.colType`. This calls into the shared value
computation logic in
utils/valueCalculations/valueCalculations.ts:142โ160 for every column,
including sparkline
columns.
4. Immediately afterward, the code checks `if (columnConfig.colType ===
'spark')` at line
94 and, for such columns, returns a `Sparkline` cell built from `valueField`,
`columnConfig`, and `entries` (lines 94โ103), discarding the previously
computed `{ value,
errorMsg }`. As a result, for any Time-series Table that includes sparkline
columns, every
render performs an extra, unused `calculateCellValue` call per row per
sparkline column,
adding avoidable CPU overhead without affecting correctness.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=70de0dce50974f8484cbef65882fa400&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=70de0dce50974f8484cbef65882fa400&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/visualizations/TimeTable/TimeTable.tsx
**Line:** 88:94
**Comment:**
*Performance: Cell value computation now runs for every column before
checking `colType`, so sparkline columns pay an unnecessary
`calculateCellValue` call whose result is discarded. On large tables this adds
avoidable per-row/per-column work; move calculation inside the non-spark branch
so only rendered numeric cells are computed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41976&comment_hash=28e0848355124d894cae7810d9466322575cd936cbf1cdae403446f68b346415&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41976&comment_hash=28e0848355124d894cae7810d9466322575cd936cbf1cdae403446f68b346415&reaction=dislike'>๐</a>
--
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]
