EnxDev commented on PR #37396:
URL: https://github.com/apache/superset/pull/37396#issuecomment-4809539195
## EnxDev's Review Agent โ apache/superset#37396 ยท HEAD d6ea42f
comment โ clean propagation and a good index-alignment fix; one reachable
`ValueError` in `extract_display_labels` and a behavior change to confirm
before merge.
Context: adds `collabels` (display labels) to chart-data results and threads
them through to DataTable headers, replacing the old `columnDisplayNames`
frontend cleanup. The `DataTableHeaderCell`/`useTableColumns` work from the
description was rebased out โ the live diff is the backend
`extract_display_labels` + `collabels` plumbing and `useGridColumns`. The
earlier bot comments about `useTableColumns:339` misalignment and the
untranslated `"Column name:"` string target code that's no longer in this diff.
### ๐ด Functional
- **`superset/utils/core.py:1880-1886`** โ `extract_display_labels` stores
raw dict columns in `columns_by_name`, then calls
`get_column_name(columns_by_name[col])`. For physical-column dicts
(`column_name` set, no `label`/`sqlExpression`) `get_column_name` falls through
to `raise ValueError("Missing label")` (core.py:1335), 500-ing `_get_full` for
any datasource exposing dict columns. Copilot flagged this and you acknowledged
it, but HEAD is unchanged and every test uses a SqlaTable (object columns) or
`None`, so the dict branch is untested. Store the resolved name for dicts โ
`columns_by_name[column_name] = column.get("verbose_name") or column_name` โ or
skip dicts so it falls back to `col`? (test: parametrize
`extract_display_labels` with a `list[dict]` column.)
### ๐ก Should-fix
- **`useGridResultTable.tsx`** (deletion) โ this drops the frontend
`__contribution` โ `(contribution)` suffix and JSON-encoded adhoc-metric
`getMetricLabel` handling (and the `DataTablesPane.test.tsx` case covering
them), delegating entirely to backend `collabels`/`label_map`. The new backend
test covers neither contribution nor adhoc-metric columns. Confirm those still
render readable headers via `label_map` rather than raw
`revenue__contribution`, or add coverage.
- **`packages/superset-ui-core/src/query/types/QueryResponse.ts:57` /
`DataTablesPane/types.ts:87`** โ `collabels` is typed required (`string[]`) but
only `_get_full` populates it; other/cached response producers may omit it.
Runtime is guarded (`ensureIsArray`, `collabels?.[โฆ]`), so the type is just
optimistic โ `collabels?: string[]` matches reality and your SamplesPane
backward-compat thread.
### ๐ต Nits
- `useGridResultTable.tsx:49` โ `headerName: headerLabel` leans on ag-grid's
implicit fallback to `field` when a label is missing; `headerLabel ?? key`
documents the "fall back to column name" intent the PR describes.
- `tests/integration_tests/utils_tests.py:498` โ `extract_display_labels` is
pure logic; a `tests/unit_tests/` test (Superset's preferred location) covering
the dict-column and label_map branches is cheaper and would catch the finding
above.
### ๐ Praise
- `useGridResultTable.tsx:42-47` โ capturing `originalIndex` before
`.filter()` keeps `coltypes`/`collabels` aligned with the visible columns,
fixing the filtered-index misalignment the old code had (and that bots flagged
on the removed `useTableColumns`).
<!-- enxdev-review-agent:d6ea42f -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD d6ea42f._
--
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]
