geido commented on code in PR #43719:
URL: https://github.com/apache/superset/pull/43719#discussion_r3915701689
##########
superset/datasets/api.py:
##########
@@ -1834,6 +1834,8 @@ def get_drill_info(self, pk: int, **kwargs: Any) ->
Response:
"columns.column_name",
"columns.verbose_name",
"columns.groupby",
Review Comment:
Good catch — this is a real regression introduced by adding metrics to the
map, and your `revenue` example is exactly it. Before this PR there were no
metrics in the map, so no collision was possible.
Fixed in `createVerboseMap` by writing metrics first and letting columns
overwrite them:
```ts
// A name can appear in both lists -- uniqueness is only enforced within each
// one -- so metrics are written first and columns overwrite them. That
matches
// `SqlaTable.data_for_slices`, which builds the verbose map the dashboard's
own
// charts already render with, and keeps an unused metric from relabelling a
// column the chart actually selected.
```
I went with this over threading the chart's field list into the request for
two reasons:
1. It matches what the dashboard already does. `SqlaTable.data_for_slices` —
the datasource payload sent with a dashboard's charts — builds `verbose_map` as
`{metrics} | {columns}`, so columns win. The results grid now agrees with the
chart rendered right next to it, instead of introducing a third resolution
order. (Note `SqlaTable.verbose_map` resolves the other way; `data_for_slices`
is the dashboard path.)
2. Making the map chart-aware means passing the chart's selected fields into
`drill_info`, which keys the response per chart instead of per dataset and
multiplies requests per dashboard load — the cost @amaannawab923 flagged in
another thread.
The residual ambiguity is a *used* metric shadowed by a same-named column,
which resolves to the column's label. That's pre-existing `data_for_slices`
behaviour rather than something this PR introduces, and it's the rarer
direction — your case, an unused metric shadowing a selected column, is the one
that's now fixed.
Test: `createVerboseMap lets a column win a name collision with a metric` in
`datasets.test.ts`, using your exact fixture (`revenue` column "Revenue" +
`revenue` metric "Revenue %"). It fails on the previous ordering.
--
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]
