anthonyhungnguyen commented on code in PR #42620:
URL: https://github.com/apache/superset/pull/42620#discussion_r3691111403
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -342,9 +371,17 @@ export const SaveDatasetModal = ({
endpoint: `/api/v1/dataset/?q=${queryParams}`,
}).then(response => ({
data: response.json.result.map(
- (r: { table_name: string; id: number; editors: Subject[] }) => ({
- value: r.table_name,
- label: r.table_name,
+ (r: {
+ table_name: string;
+ id: number;
+ editors: Subject[];
+ schema?: string;
+ }) => ({
+ // `id` is unique; `table_name` is not. Keying options by the table
+ // name collapses same-named datasets onto a single Select key, which
+ // renders duplicate rows and makes the overwrite target ambiguous.
+ value: r.id,
+ label: r.schema ? `${r.schema}.${r.table_name}` : r.table_name,
Review Comment:
Agreed — fixed in 9ddff12.
Options are now labelled with every part that is set:
`database.catalog.schema.table_name` (e.g.
`analytics.reporting.prod.task_instance`), which is the actual uniqueness key.
On the search: only the table part can go to the API. The qualifiers span
three columns, and `database` is a relationship the list endpoint can't match
on by name (`search_columns` exposes it, but not as a `ct` filter on
`database_name`). So the request sends the trailing part of the search as the
`table_name` filter and the qualifiers are narrowed client-side over the rows
that came back. Parts are matched positionally-independently, so
`analytics.prod.sales` still matches a dataset that also has a catalog, and a
trailing `.` is treated as qualification rather than a table name.
That does mean the qualifier narrowing only sees the rows the table search
returned — a limitation if more than a page of datasets share one table name,
though that's far rarer than having more than a page of datasets overall, which
was the case that broke before. This also replaces the server-side `schema`
filter from the previous commit: it can't be kept, since a two-part search can
just as easily be `database.table` as `schema.table`, and guessing wrong sends
a filter that matches nothing.
Test covers three datasets sharing `task_instance` across two databases and
two schemas, one with a catalog, with the fixture padded past one API page. I
checked it's not vacuous — it fails if the database is dropped from the label,
if the whole search string is sent as the `table_name` filter, or if the local
filter is stubbed out.
--
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]
