aminghadersohi commented on PR #42655:
URL: https://github.com/apache/superset/pull/42655#issuecomment-5187555170
Thanks @bito-code-review — worked through all five suggestions. All are
addressed in `79f2704`, and chasing the dedup one uncovered a genuine bug in
this PR.
### The bug the review led to
`_append_table_columns()` built its patch with
`map_config_to_form_data(TableChartConfig(columns=...))`, which **infers
`query_mode` from the columns it's handed**:
| appended columns | inferred mode | `groupby` |
|---|---|---|
| plain only | `raw` | `None` |
| metric only | `aggregate` | `None` |
| mixed | `aggregate` | `['region']` |
So a dimension-only append compiled to a *raw* patch, `patch.get("groupby")`
was `None`, and `.extend(... or [])` appended nothing — **adding a plain column
to an aggregate table silently did nothing.** Confirmed against the unmodified
branch:
```
groupby -> ['employer'] # expected ['employer', 'region']
```
Existing tests missed it because they either appended a metric
(`aggregate="MIN"`) or asserted only on `datasource_id`. Columns are now routed
by `ColumnRef.is_metric`, so dimensions reach `groupby` and metrics reach
`metrics` regardless of the mix, and the chart is never flipped out of
aggregate mode.
### Suggestions 1 & 2 — test coverage
`add_columns` was already covered behaviorally in `test_update_chart.py`
(not `test_chart_schemas.py`, hence the miss). The **validator was genuinely
untested** — both error branches now are. Replied on both threads.
### Suggestions 3 & 4 — deduplication
Agreed on the substance, with one correction: the proposed `set()` approach
doesn't work here.
```python
>>> set(['count', {'label': 'Earliest Go Live Date', 'aggregate': 'MIN'}])
TypeError: unhashable type: 'dict'
```
`metrics` mixes saved-metric names with adhoc metric **dicts**, so a set
raises — and it would also discard the ordering that drives table column
layout. Dedup is instead keyed on a stable serialization and preserves
first-occurrence order, applied to `all_columns`, `groupby`, and `metrics`.
Re-adding an existing column is now a no-op rather than a silent duplicate.
### Suggestion 5 — docs
Added an example combining `chart_name` with `add_columns`, noting that
repeating an existing column is safe.
### Verification
`tests/unit_tests/mcp_service/` — **3283 passed**; `chart/` — **1184
passed**. Ruff format + check clean on the pinned 0.9.7. Six new tests,
including regression coverage for the dropped-column bug.
--
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]
