fix(sqllab): key overwrite-dataset options by id, not table name - #42620
fix(sqllab): key overwrite-dataset options by id, not table name#42620anthonyhungnguyen wants to merge 6 commits into
Conversation
The "Overwrite existing" dropdown in the Save or Overwrite Dataset modal built its options with `value: r.table_name`. Table names are not unique across schemas, so a user who can edit several datasets that share a table name (e.g. an `task_instance` saved from a staging and a prod schema) hit three problems: - duplicate `value`s collide on a single Select key, so the listbox intermittently rendered the same row several times - identical labels made the datasets indistinguishable - the resulting selection was ambiguous, so Overwrite could target the wrong dataset Key the options by the dataset id, which is unique, and render a schema-qualified label so same-named datasets can be told apart. The autocomplete filter now matches on the label, since the value is no longer a string. Adds a regression test covering two same-named datasets in different schemas: each renders once, and overwriting the selected row PUTs to that dataset's id.
dosubot
Bot
added
change:frontend
Code Review Agent Run #ab9164Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42620 +/- ##
=======================================
Coverage 65.44% 65.45%
=======================================
Files 2810 2810
Lines 159335 159336 +1
Branches 36362 36363 +1
=======================================
+ Hits 104282 104287 +5
+ Misses 53011 53006 -5
- Partials 2042 2043 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Types a schema prefix into the existing-dataset combobox so the regression test also exercises filterAutocompleteOption, which now matches on the rendered label rather than the option value.
sadpandajoe
left a comment
sadpandajoe
left a comment
There was a problem hiding this comment.
Found one autocomplete gap in the qualified-label path.
| inputValue: string, | ||
| option: DatasetOverwriteOption, | ||
| ) => option.value.toLowerCase().includes(inputValue.toLowerCase()); | ||
| ) => option.label.toLowerCase().includes(inputValue.toLowerCase()); |
There was a problem hiding this comment.
When the editable dataset list spans more than one page, typing the new schema prefix still sends prod. to the API as a table_name filter, so the async response replaces the options with an empty list even when prod.task_instance exists. Could the server query search schema/qualified labels too, or otherwise keep schema filtering client-side?
There was a problem hiding this comment.
Good catch — you're right, and the regression test I'd written was masking it: the SupersetClient.get stub ignored the request, so the search string never had to match anything server-side.
Fixed in c800f64. The search string is now split on the first dot and sent as two filters, schema and table_name (both are in the dataset API's search_columns), so prod.task_ reaches the server as schema ct 'prod' + table_name ct 'task_' instead of a table_name filter that can never match. filterAutocompleteOption mirrors the same split, so the local pass — which only exists to hide options left over from an earlier search — can't hide a row the API deliberately returned (e.g. prod. vs. schema production).
The test now pads the fixture past one API page and applies the rison filters the way the API does. I checked it's not vacuous: reverting just the server-side split makes it fail on exactly the scenario you described (the response comes back empty and the prod.task_instance row disappears).
|
The pull request addresses this issue by changing how dataset options are keyed and filtered in the superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx |
The overwrite options are labelled `schema.table_name`, but the search string was sent to the API as a table_name filter only, so typing the schema prefix matched nothing server-side. When the editable dataset list spans more than one page the target row is never fetched, so a client-side filter cannot recover it. Split the search on the first dot and filter on `schema` and `table_name` separately, and mirror that split in the local filterOption so it cannot hide a row the API returned.
Code Review Agent Run #983633Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| // 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, |
There was a problem hiding this comment.
Datasets are unique by database/catalog/schema/table, so editable datasets from different databases or catalogs can still render the same label here and leave the user unable to tell which one will be overwritten. Could the label and autocomplete search include the database/catalog identity as well?
There was a problem hiding this 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.
Datasets are unique by database, catalog, schema and table name, so two editable datasets could still render the same `schema.table_name` label and leave the user unable to tell which one they were overwriting. Label the options with every part that is set, and match the search against those parts. Only the table part can be pushed to the API — the qualifiers span three columns, one of which is a relationship the list endpoint cannot match on by name — so the qualifiers are narrowed client-side over the rows the table search returned.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| @@ -342,9 +385,19 @@ export const SaveDatasetModal = ({ | |||
| endpoint: `/api/v1/dataset/?q=${queryParams}`, | |||
There was a problem hiding this comment.
Suggestion: The loader ignores the page and pageSize arguments required by AsyncSelect and never sends pagination parameters to the dataset API. When more than one page matches the table-name filter, subsequent scroll requests retrieve the same first page, so editable datasets beyond the first page cannot be selected. Accept the pagination arguments and include the corresponding page and page-size query parameters. [api mismatch]
Severity Level: Major ⚠️
- ❌ Large overwrite lists cannot expose later datasets.
- ❌ SQL Lab users cannot select some editable datasets.
- ⚠️ AsyncSelect pagination repeatedly fetches page one.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx
**Line:** 367:385
**Comment:**
*Api Mismatch: The loader ignores the `page` and `pageSize` arguments required by `AsyncSelect` and never sends pagination parameters to the dataset API. When more than one page matches the table-name filter, subsequent scroll requests retrieve the same first page, so editable datasets beyond the first page cannot be selected. Accept the pagination arguments and include the corresponding page and page-size query parameters.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| col: 'table_name', | ||
| opr: 'ct', | ||
| value: input, | ||
| value: tableSearch, |
There was a problem hiding this comment.
Suggestion: When the input contains a separator, only the table-name suffix is sent to the API and qualifier filtering is deferred to the browser. The async select receives only the first API page, so if more than one page of editable datasets shares that table-name suffix, a matching database/schema dataset on a later page is never loaded and cannot be selected. Apply the qualifier filters server-side, or request and merge all relevant pages before client-side filtering. [logic error]
Severity Level: Major ⚠️
- ⚠️ Large table-name result sets hide editable datasets.
- ❌ Users cannot overwrite matching datasets beyond page one.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx
**Line:** 366:366
**Comment:**
*Logic Error: When the input contains a separator, only the table-name suffix is sent to the API and qualifier filtering is deferred to the browser. The async select receives only the first API page, so if more than one page of editable datasets shares that table-name suffix, a matching database/schema dataset on a later page is never loaded and cannot be selected. Apply the qualifier filters server-side, or request and merge all relevant pages before client-side filtering.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| return parseQualifiedSearch(inputValue.toLowerCase()).parts.every(part => | ||
| label.includes(part), | ||
| ); |
There was a problem hiding this comment.
Suggestion: The client-side filter treats every qualified-search component as an unordered substring, so a search such as foo.bar can match a dataset whose label contains bar.foo or where one component merely occurs inside another value. This can present unrelated datasets and make the selection ambiguous. Match the qualified components in their actual database/catalog/schema/table order, or use structured fields for filtering. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Qualified searches can show unrelated datasets.
- ⚠️ SQL Lab overwrite selection remains ambiguous.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx
**Line:** 477:479
**Comment:**
*Incorrect Condition Logic: The client-side filter treats every qualified-search component as an unordered substring, so a search such as `foo.bar` can match a dataset whose label contains `bar.foo` or where one component merely occurs inside another value. This can present unrelated datasets and make the selection ambiguous. Match the qualified components in their actual database/catalog/schema/table order, or use structured fields for filtering.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Code Review Agent Run #42577fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
The "Overwrite existing" dropdown in SQL Lab's Save or Overwrite Dataset modal builds its options with
value: r.table_name:Table names are not unique — the same table name can exist in several schemas, and a user can be an editor on datasets for all of them (Airflow metadata tables such as
task_instancesaved from a staging and a prod schema are a common case). When that happens:values collide on one Select key, so the listbox intermittently renders the same row several times (reported as "sometimes I get 8 identical rows")This PR keys the options by the dataset
id, which is unique, and renders a schema-qualified label so same-named datasets can be told apart:schemais already part of the dataset API'slist_columns, so no backend change is needed.filterAutocompleteOptionnow matches on the label rather than the value, since the value is no longer a string, andDatasetOptionAutocomplete.valueis typednumber. The overwrite path itself is unaffected — it already useddatasetToOverwrite.datasetId.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: with three editable datasets named
task_instancein different schemas, the dropdown shows repeated, identicaltask_instancerows and there is no way to tell which one Overwrite will hit.After: one row per dataset, each labelled
staging.task_instance,prod.task_instance, etc., and Overwrite targets the row that was clicked.TESTING INSTRUCTIONS
cd superset-frontend npx jest src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx20/20 pass. Added a regression test —
distinguishes datasets that share a table name and overwrites the selected one— that stubs two editable datasets sharing the table nametask_instanceacross astagingand aprodschema, asserts each renders exactly once under its schema-qualified label, that typing theprod.prefix filters the staging row out (the autocomplete filter now matches on the label), and that selecting theprodrow issues the PUT against that dataset's id. The test fails onmaster(both labels resolve to two rendered nodes) and passes with this change.The three existing test call sites that looked up
'coolest table 0'were updated to the new qualified label.prettier --checkandoxlintare clean on the touched files;tsc --noEmitreports no new errors.Manual check: open SQL Lab → run a query → Save → Save or Overwrite Dataset → Overwrite existing, with an account that can edit two datasets sharing a table name in different schemas.
ADDITIONAL INFORMATION