codeant-ai-for-open-source[bot] commented on code in PR #41851:
URL: https://github.com/apache/superset/pull/41851#discussion_r3576522691
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/isEqualColumns.ts:
##########
@@ -64,6 +64,10 @@ export default function isEqualColumns(
JSON.stringify(b.formData.extraFormData || null) &&
JSON.stringify(a.rawFormData.column_config || null) ===
JSON.stringify(b.rawFormData.column_config || null) &&
- JSON.stringify(descA) === JSON.stringify(descB)
+ JSON.stringify(descA) === JSON.stringify(descB) &&
+ isEqualArray(
+ (a.datasource?.columns || []).map(c => c.type),
+ (b.datasource?.columns || []).map(c => c.type),
+ )
Review Comment:
**Suggestion:** The memoization equality check only compares a flat ordered
list of column `type` values, but `processColumns` looks up types by
`column_name`. If the same type sequence is preserved while name/type
associations change, this comparator can incorrectly treat props as equal and
reuse stale memoized columns, causing wrong UUID detection in the Search By
options. Compare `{column_name, type}` pairs (or keyed types for current
`colnames`) instead of type-only arrays. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Table chart Search By options use stale column types.
- ❌ UUID columns may wrongly included/excluded from server search.
- ⚠️ Server pagination search can still emit invalid ILIKE.
- ⚠️ Dataset metadata changes silently ignored by memoization equality.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `transformProps.ts:78-207`, `processColumns` is wrapped with
`memoizeOne` and uses
`isEqualColumns` from `isEqualColumns.ts:41-72` as the custom equality
comparator;
`processColumns` builds `columnsByName` from `props.datasource.columns` and
sets `type:
columnsByName.get(key)?.type` on each `DataColumnMeta` (lines 104-105, 193).
2. First call `transformProps` (at `transformProps.ts:231-276`) with a
`TableChartProps`
instance where `datasource.columns` contains entries like `{ column_name:
'name', type:
'VARCHAR' }` and `{ column_name: 'uuid_col', type: 'UUID' }`, and
`queriesData[0].colnames` is `['name', 'uuid_col']` with matching `coltypes`;
`processColumns` computes `columns` so that the `DataColumnMeta` for
`uuid_col` has `type:
'UUID'`.
3. Next call `transformProps` again with a new `chartProps` where only
`datasource.columns` has changed: the `column_name` fields are edited so
that the UUID
type is associated with a different name (or some names are renamed), but
the ordered list
of `type` values in `datasource.columns` remains identical;
`queriesData[0].colnames`,
`coltypes`, `rawFormData`, and other fields are kept the same, so this
change only affects
the name-to-type mapping used by `columnsByName`.
4. During the second call, `memoizeOne` invokes `isEqualColumns` with
`[props2]` and
`[props1]`. At `isEqualColumns.ts:51-71`, all checks—including the new
type-only
comparison `isEqualArray((a.datasource?.columns || []).map(c => c.type),
(b.datasource?.columns || []).map(c => c.type))` at lines 68-71—evaluate to
true because
the flat `type` sequence has not changed, so `processColumns` reuses the
previously
memoized `columns` array. The `DataColumnMeta.type` values therefore stay
tied to the old
`columnsByName` mapping, and in `AgGridTableChart.tsx:73-86` the Search By
options are
built from `columns` and filtered with `!isUuidColumn(col)` (lines 12-13,
75-77), causing
UUID detection and Search By dropdown contents to be incorrect for the
updated dataset
metadata.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9664d14417e64b8a80ea0261966c79b3&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=9664d14417e64b8a80ea0261966c79b3&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/plugins/plugin-chart-ag-grid-table/src/utils/isEqualColumns.ts
**Line:** 69:71
**Comment:**
*Cache: The memoization equality check only compares a flat ordered
list of column `type` values, but `processColumns` looks up types by
`column_name`. If the same type sequence is preserved while name/type
associations change, this comparator can incorrectly treat props as equal and
reuse stale memoized columns, causing wrong UUID detection in the Search By
options. Compare `{column_name, type}` pairs (or keyed types for current
`colnames`) instead of type-only arrays.
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%2F41851&comment_hash=8c185ea6e2ebd6df57d8e20ce07f5225135d32da610cd49328f44d59abb3aed2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41851&comment_hash=8c185ea6e2ebd6df57d8e20ce07f5225135d32da610cd49328f44d59abb3aed2&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]
