codeant-ai-for-open-source[bot] commented on code in PR #38579:
URL: https://github.com/apache/superset/pull/38579#discussion_r2941785824
##########
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.test.ts:
##########
@@ -58,16 +54,16 @@ describe('sortNumberWithMixedTypes', () => {
const rowA = createMockRow(10);
const rowB = createMockRow(20);
- const result = sortNumberWithMixedTypes(rowA, rowB, 'testColumn', true);
+ const result = sortNumberWithMixedTypes(rowA, rowB, 'testColumn');
- expect(result).toBeGreaterThan(0); // rowB should come before rowA in
descending
+ expect(result).toBeLessThan(0);
Review Comment:
**Suggestion:** The "descending order" test currently performs the exact
same assertion as the ascending test, so it does not validate reverse ordering
behavior and can let descending regressions pass unnoticed. Compare the larger
row against the smaller row and assert a positive comparator result to actually
exercise opposite ordering. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Descending-sort regressions can bypass CI test coverage.
- ⚠️ TimeTable sort reliability checks are partially ineffective.
```
</details>
```suggestion
const result = sortNumberWithMixedTypes(rowB, rowA, 'testColumn');
expect(result).toBeGreaterThan(0);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect TimeTable sorting entrypoint:
`superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:71` assigns
`sortType:
sortNumberWithMixedTypes`, and `TimeTable.tsx:123` sets default `desc:
true`, confirming
descending order is a real runtime mode.
2. Inspect comparator behavior in
`superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:106`;
it
returns raw `compareValues(valueA, valueB, ...)`, so sign should invert when
row argument
order is reversed.
3. Open the "descending" unit test at
`superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.test.ts:53-60`;
it calls `sortNumberWithMixedTypes(rowA, rowB, ...)` and expects `< 0`,
identical to the
ascending test at `:44-51`.
4. Run the test file; both tests pass while asserting the same ordering
direction, showing
descending-specific behavior is not actually validated and regressions in
reverse-order
handling could pass unnoticed.
```
</details>
<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/utils/sortUtils/sortUtils.test.ts
**Line:** 57:59
**Comment:**
*Logic Error: The "descending order" test currently performs the exact
same assertion as the ascending test, so it does not validate reverse ordering
behavior and can let descending regressions pass unnoticed. Compare the larger
row against the smaller row and assert a positive comparator result to actually
exercise opposite ordering.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38579&comment_hash=22b49af8f9023aae48ac6d3f9582933e369c6ee77ae78a14d5cdf12c2e94c121&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38579&comment_hash=22b49af8f9023aae48ac6d3f9582933e369c6ee77ae78a14d5cdf12c2e94c121&reaction=dislike'>👎</a>
##########
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:
##########
@@ -42,23 +44,64 @@ function compareValues(
}
/**
- * Sorts table rows with mixed data types for react-table
+ * Sorts table rows with mixed data types for react-table.
+ *
* @param rowA - First row to compare
* @param rowB - Second row to compare
* @param columnId - Column identifier for sorting
* @param descending - Whether to sort in descending order
* @returns Numeric comparison result for react-table
+ * react-table handles the asc/desc direction flip internally after calling
+ * this function, so we only return the raw comparison result.
*/
export function sortNumberWithMixedTypes(
rowA: any,
rowB: any,
columnId: string,
- descending: boolean,
) {
- const valueA = rowA.values[columnId].props['data-value'];
- const valueB = rowB.values[columnId].props['data-value'];
+ const cellA = rowA.values?.[columnId];
+ const cellB = rowB.values?.[columnId];
+
+ // Both ValueCell and Sparkline cells pass React elements here.
+ // ValueCell uses { valueField, column, reversedEntries }
+ // Sparkline/SparklineCell uses { valueField, column, entries }
+ // Normalize to reversedEntries before delegating to calculateCellValue.
+ const propsA = cellA?.props as
+ | {
+ valueField: string;
+ column: ColumnConfig;
+ reversedEntries?: Entry[];
+ entries?: Entry[];
+ }
+ | undefined;
+ const propsB = cellB?.props as
+ | {
+ valueField: string;
+ column: ColumnConfig;
+ reversedEntries?: Entry[];
+ entries?: Entry[];
+ }
+ | undefined;
+
+ const reversedEntriesA =
+ propsA?.reversedEntries ?? propsA?.entries?.slice().reverse();
+ const reversedEntriesB =
+ propsB?.reversedEntries ?? propsB?.entries?.slice().reverse();
+
+ if (!propsA || !propsB || !reversedEntriesA || !reversedEntriesB) {
+ return 0;
+ }
- const comparison = compareValues(valueA, valueB, 'asSmallest');
+ const { value: valueA } = calculateCellValue(
+ propsA.valueField,
+ propsA.column,
+ reversedEntriesA,
+ );
+ const { value: valueB } = calculateCellValue(
+ propsB.valueField,
+ propsB.column,
+ reversedEntriesB,
+ );
- return comparison * (descending ? -1 : 1);
+ return compareValues(valueA, valueB, 'asSmallest');
Review Comment:
**Suggestion:** Sparkline columns with `timeRatio` are sorted using
`calculateCellValue`, which ignores sparkline ratio logic and returns the raw
latest metric instead of the displayed ratio. This makes sort order
inconsistent with what users see in time-ratio sparklines. Compute the
comparable value from the same ratio logic used by sparkline rendering before
calling `compareValues`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Time-ratio sparkline sorting mismatches displayed plotted values.
- ⚠️ Analysts can mis-rank rows in TimeTable interactions.
```
</details>
```suggestion
const getComparableValue = (
props: { valueField: string; column: ColumnConfig },
reversedEntries: Entry[],
) => {
if (props.column.colType === 'spark' && props.column.timeRatio) {
const lag = Number(props.column.timeRatio);
const current = reversedEntries[0]?.[props.valueField];
const lagged = reversedEntries[lag]?.[props.valueField];
if (
Number.isInteger(lag) &&
lag > 0 &&
typeof current === 'number' &&
typeof lagged === 'number' &&
lagged !== 0
) {
return current / lagged;
}
return null;
}
return calculateCellValue(props.valueField, props.column,
reversedEntries).value;
};
const valueA = getComparableValue(propsA, reversedEntriesA);
const valueB = getComparableValue(propsB, reversedEntriesB);
return compareValues(valueA, valueB, 'asSmallest');
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Time Series Table chart with a sparkline time-series column
using
`timeRatio` (render path in
`superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:84-92`, where
sparkline
cells are created).
2. Click that sparkline column header to sort; `TableView` uses `sortType:
sortNumberWithMixedTypes` for every configured column
(`TimeTable.tsx:53-72`).
3. Comparator `sortNumberWithMixedTypes` computes values via
`calculateCellValue(...)`
(`sortUtils.ts:95-104`), and for `colType === 'spark'` this falls through to
raw recent
metric return (`valueCalculations.ts:151-160`), not ratio logic.
4. Sparkline rendering itself applies ratio transformation when
`column.timeRatio` exists
(`Sparkline.tsx:42`, `sparklineDataUtils.ts:68-71`), so sort order can
differ from visible
ratio trend/endpoint when raw-value ranking and ratio ranking diverge.
```
</details>
<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/utils/sortUtils/sortUtils.ts
**Line:** 95:106
**Comment:**
*Logic Error: Sparkline columns with `timeRatio` are sorted using
`calculateCellValue`, which ignores sparkline ratio logic and returns the raw
latest metric instead of the displayed ratio. This makes sort order
inconsistent with what users see in time-ratio sparklines. Compute the
comparable value from the same ratio logic used by sparkline rendering before
calling `compareValues`.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38579&comment_hash=344a90917fb46e292634db848a75ab98ad2fb287bdf7656445f19b50552b6f63&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38579&comment_hash=344a90917fb46e292634db848a75ab98ad2fb287bdf7656445f19b50552b6f63&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]
