anthonyhungnguyen commented on code in PR #42620:
URL: https://github.com/apache/superset/pull/42620#discussion_r4372079439
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -195,6 +195,37 @@ const updateDataset = async ({
const UNTITLED = t('Untitled Dataset');
+/**
+ * Datasets are unique by database, catalog, schema and table name, so a label
+ * built from anything less can be ambiguous — e.g. `examples.public.sales`.
+ */
+const qualifiedLabel = (dataset: {
+ database?: { database_name?: string };
+ catalog?: string | null;
+ schema?: string | null;
+ table_name: string;
+}) =>
+ [
+ dataset.database?.database_name,
+ dataset.catalog,
+ dataset.schema,
+ dataset.table_name,
+ ]
+ .filter(Boolean)
Review Comment:
Fixed in e6b27e3: the label keeps the schema slot even when it's empty, so
`(catalog=reporting, schema=null)` renders as `analytics.reporting..sales` and
`(catalog=null, schema=reporting)` as `analytics.reporting.sales`. A missing
catalog is still dropped, so the common case reads as before. Search ignores
empty parts, so typing either form still matches. Added a test for that pair.
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -342,9 +385,19 @@ export const SaveDatasetModal = ({
endpoint: `/api/v1/dataset/?q=${queryParams}`,
Review Comment:
Valid, fixed in e6b27e3: the loader now takes AsyncSelect's `page` and
`pageSize` and sends them as `page` / `page_size`, so scrolling loads the next
page instead of the first one again. Added a test that checks the request.
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -321,12 +352,18 @@ export const SaveDatasetModal = ({
};
const loadDatasetOverwriteOptions = useCallback(async (input = '') => {
+ // Only the table part can be filtered server-side — `database` is a
+ // relationship the list endpoint cannot match on by name. Sending the
+ // whole search as a `table_name` filter matches nothing once the user
+ // types a separator; filterAutocompleteOption narrows the qualifiers.
+ const { tableSearch } = parseQualifiedSearch(input);
+
const queryParams = rison.encode({
filters: [
{
col: 'table_name',
opr: 'ct',
- value: input,
+ value: tableSearch,
Review Comment:
The list endpoint can't filter `database` by name, so the qualifiers can
only be matched client-side. With paging fixed (see the thread above), datasets
on later pages load as you scroll, so a qualified search no longer stops at the
first page.
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -424,7 +470,14 @@ export const SaveDatasetModal = ({
const filterAutocompleteOption = (
inputValue: string,
option: DatasetOverwriteOption,
- ) => option.value.toLowerCase().includes(inputValue.toLowerCase());
+ ) => {
+ const label = option.label.toLowerCase();
+ // Position-independent: a dataset may or may not have a catalog, and a
+ // search skipping a part (`examples.sales`) should still match.
+ return parseQualifiedSearch(inputValue.toLowerCase()).parts.every(part =>
+ label.includes(part),
+ );
Review Comment:
Leaving this as is on purpose. Matching parts regardless of position lets a
search skip the catalog (`analytics.prod.task_instance` finds
`analytics.reporting.prod.task_instance`). Each option shows its full label, so
an extra match can't be mistaken for the one being searched.
--
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]
