EnxDev commented on PR #41851:
URL: https://github.com/apache/superset/pull/41851#issuecomment-4961755514
## EnxDev's Review Agent โ apache/superset#41851 ยท HEAD 535b2ec
**comment** โ the legacy-table fix is correct and well-tested, but the
identical bug remains in the ag-grid table plugin this PR already touches, and
the UUID match may miss ClickHouse's wrapped types.
Scope check: legacy `plugin-chart-table` fix verified correct โ `columns` is
a 1:1 `visibleColumnsMeta.map(...)`, so the new `visibleColumnsMeta[index]` in
the search-option filter (`TableChart.tsx:1473-1477`) aligns index-for-index.
No misalignment there.
### ๐ก Should-fix
- **`plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:104-115`** โ Same
bug is not fixed here. ag-grid builds `searchOptions` from `col?.dataType ===
GenericDataType.String` with no UUID exclusion, so UUID columns still appear in
its **Search By** and still drive the invalid `ILIKE` (`buildQuery.ts` uses
`searchColumn`). This PR already touches ag-grid โ it propagates `type` into
ag-grid columns (`transformProps.ts:452`) and extends the memo comparator โ but
never consumes it, so those ag-grid changes are currently dead setup while the
go-forward table stays broken. Apply the same `isUuidColumn` filter (`... &&
!isUuidColumn(col)`) to this effect. **regression test:** ag-grid render test
asserting a UUID column is absent from Search By.
- **`plugin-chart-table/src/TableChart.tsx:133-135`** โ `isUuidColumn`
matches only `type.toLowerCase() === 'uuid'`. Postgres reports `UUID` (fixed),
but dialect-wrapped/nullable forms โ e.g. ClickHouse `Nullable(UUID)` โ won't
match, leaving the invalid ILIKE for one of the two DBs the PR names. Use
`type.toLowerCase().includes('uuid')` (or a regex). Couldn't confirm
ClickHouse's exact reported type string โ worth verifying against a ClickHouse
UUID dataset.
- **`TableChart.tsx:1567`** (and ag-grid `AgGridTableChart.tsx:418`) โ A
chart saved with a UUID column already selected keeps sending ILIKE against it
after the fix: `serverPaginationData?.searchColumn || searchOptions[0]?.value`
prefers the persisted value and never validates it against the now-filtered
options. Fall back to `searchOptions[0]?.value` when `searchColumn` isn't in
`searchOptions`. Low โ only affects pre-existing selections.
### ๐ต Nits
- `plugin-chart-table/src/transformProps.ts:327` โ `type` is looked up by
`column_name` only; adhoc/aliased UUID expressions (label โ dataset column) get
`type: undefined` and stay searchable. Pre-existing gap, not a regression โ
note as a known limitation.
- `TableChart.test.tsx` โ scoping the Search By combobox via
`searchByLabel.parentElement!` is brittle; the PR adds `aria-label={t('Search
by')}` to the dropdown, so `getByRole('combobox', { name: 'Search by' })` would
be more stable.
### ๐ Praise
- `utils/isEqualColumns.ts` (both plugins) โ correctly extended the memo
comparator with the column `type` arrays so a dataset type change busts the
`memoizeOne` cache; easy to miss, and it makes the earlier bot "stale-memo"
concern moot.
<!-- enxdev-review-agent:535b2ec -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 535b2ec._
--
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]
