diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.tsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.tsx index 8e0f9f16ae8..3bb8c6d9da4 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.tsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.tsx @@ -951,3 +951,167 @@ test('filters the subject select by column verbose_name as well as column_name', expect(within(dropdown).getByText('total_count')).toBeInTheDocument(); expect(within(dropdown).queryByText('Full Name')).not.toBeInTheDocument(); }); + +const COLUMN_VALUES_ENDPOINT = + 'glob:*/api/v1/datasource/*/column/value/values/*'; + +let columnValues: { result: unknown[]; limit: number } = { + result: [], + limit: 10000, +}; +fetchMock.get(COLUMN_VALUES_ENDPOINT, () => columnValues); + +const setupWithFilterValues = (result: unknown[], limit = 10000) => { + columnValues = { result, limit }; + const onChange = jest.fn(); + const validHandler = jest.fn(); + const spy = jest.spyOn(redux, 'useSelector'); + spy.mockReturnValue({}); + const props = { + adhocFilter: new AdhocFilter({ + expressionType: ExpressionTypes.Simple, + subject: 'value', + operatorId: Operators.In, + operator: OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.In].operation, + comparator: [], + clause: Clauses.Where, + }), + onChange, + options, + datasource: { + ...TestDataset, + columns: [{ column_name: 'value', type: 'VARCHAR', id: 3 }], + filter_select: true, + }, + partitionColumn: 'test', + validHandler, + }; + render( + , + ); + return props; +}; + +const openComparator = async () => { + const comparator = screen.getByRole('combobox', { + name: 'Comparator option', + }); + userEvent.click(comparator); + return comparator; +}; + +test('loads comparator values from the server', async () => { + setupWithFilterValues(['alpha', 'beta']); + await openComparator(); + expect(await screen.findByTitle('alpha')).toBeInTheDocument(); +}); + +test('sends the typed text to the server rather than filtering the loaded page', async () => { + // The loaded page is bounded, so matching client-side cannot reach a value + // beyond the row limit. The search has to reach the database. + setupWithFilterValues(['alpha']); + const comparator = await openComparator(); + userEvent.type(comparator, 'gamma'); + + await waitFor( + () => { + const searched = fetchMock.callHistory + .calls(COLUMN_VALUES_ENDPOINT) + .map(call => String(call.url)); + expect(searched.some(url => url.includes('q=gamma'))).toBe(true); + }, + { timeout: 3000 }, + ); +}); + +test('lets a value the server did not return still be selected', async () => { + // Even with server-side search a match can fall outside the page; typing the + // exact value has to remain a way through. + setupWithFilterValues([]); + const comparator = await openComparator(); + userEvent.type(comparator, 'not-in-the-page'); + expect(await screen.findByTitle('not-in-the-page')).toBeInTheDocument(); +}); + +test('does not query for values when the dataset disables them', async () => { + fetchMock.clearHistory(); + setup({ + adhocFilter: new AdhocFilter({ + expressionType: ExpressionTypes.Simple, + subject: 'value', + operatorId: Operators.In, + operator: OPERATOR_ENUM_TO_OPERATOR_TYPE[Operators.In].operation, + comparator: [], + clause: Clauses.Where, + }), + }); + await openComparator(); + expect(fetchMock.callHistory.calls(COLUMN_VALUES_ENDPOINT)).toHaveLength(0); +}); + +test('stores the picked value, not the option object', async () => { + // AsyncSelect is labelInValue: taking its argument at face value puts + // {label, value} into the comparator, and the engine then fails to render it + // as a literal. + const props = setupWithFilterValues(['Michael']); + await openComparator(); + userEvent.click(await screen.findByTitle('Michael')); + + await waitFor(() => expect(props.onChange).toHaveBeenCalled()); + const [filter] = props.onChange.mock.calls.at(-1); + expect(filter.comparator).toEqual(['Michael']); +}); + +test('can remove a value that was saved earlier', async () => { + // Reopening the popover restores the comparator from the saved filter, and + // the value is not in the freshly loaded page. Removing it has to still work. + columnValues = { result: [], limit: 10000 }; + const onChange = jest.fn(); + const validHandler = jest.fn(); + jest.spyOn(redux, 'useSelector').mockReturnValue({}); + render( + , + ); + + // Remove it the way a user does: the tag's own close control. + userEvent.click(await screen.findByLabelText('close')); + + await waitFor(() => expect(onChange).toHaveBeenCalled()); + const [filter] = onChange.mock.calls.at(-1); + expect(filter.comparator).toEqual([]); +}); + +test('says the list is partial when the server capped it', async () => { + setupWithFilterValues(['alpha', 'beta'], 2); + await openComparator(); + expect( + await screen.findByText(/Only the first 2 values are listed/), + ).toBeInTheDocument(); +}); + +test('does not say the list is partial when it is complete', async () => { + setupWithFilterValues(['alpha', 'beta'], 10000); + await openComparator(); + expect(await screen.findByTitle('alpha')).toBeInTheDocument(); + expect(screen.queryByText(/Only the first/)).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx index 692d84b3135..631d743bd3c 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx @@ -16,13 +16,25 @@ * specific language governing permissions and limitations * under the License. */ -import { FC, ChangeEvent, useEffect, useState, useRef } from 'react'; +import { + FC, + ChangeEvent, + useCallback, + useEffect, + useMemo, + useState, + useRef, +} from 'react'; import { + AsyncSelect, Input, InputRef, Select, Tooltip, + type AsyncSelectRef, + type LabeledValue, + type SelectOptionsTypePage, type SelectValue, } from '@superset-ui/core/components'; import { t } from '@apache-superset/core/translation'; @@ -57,7 +69,7 @@ import { useDatePickerInAdhocFilter } from '../utils'; import { useDefaultTimeFilter } from '../../DateFilterControl/utils'; import { Clauses, ExpressionTypes } from '../types'; -const SelectWithLabel = styled(Select)<{ labelText: string }>` +const SelectWithLabel = styled(AsyncSelect)<{ labelText: string }>` .ant-select-content::after { content: ${({ labelText }) => labelText || '\\A0'}; display: inline-block; @@ -67,6 +79,30 @@ const SelectWithLabel = styled(Select)<{ labelText: string }>` } `; +// The server answers with one bounded page, not an offset window: paging would +// need a stable ORDER BY, and ordering a high-cardinality column is the full +// scan this search exists to avoid. A page size no response can reach keeps +// AsyncSelect from asking for a second page. +const COMPARATOR_PAGE_SIZE = 1_000_000; + +const toLabeledValue = (value: unknown): LabeledValue => ({ + value: value as LabeledValue['value'], + label: optionLabel(value as null | number | boolean | string), +}); + +// The reverse of toLabeledValue: what AsyncSelect emits is labelled, and the +// comparator has to be the raw value or the engine cannot render it as a +// literal. +const unwrapComparator = (value: unknown): unknown => { + if (Array.isArray(value)) { + return value.map(unwrapComparator); + } + if (value !== null && typeof value === 'object' && 'value' in value) { + return (value as LabeledValue).value; + } + return value; +}; + export interface SimpleExpressionType { expressionType: keyof typeof ExpressionTypes; column: ColumnMeta; @@ -347,11 +383,9 @@ const AdhocFilterEditPopoverSimpleTabContent: FC = props => { } = useSimpleTabFilterProps(props); const [comparator, setComparator] = useState(props.adhocFilter.comparator); const comparatorInputRef = useRef(null); - const [suggestions, setSuggestions] = useState< - Record<'label' | 'value', any>[] - >([]); - const [loadingComparatorSuggestions, setLoadingComparatorSuggestions] = - useState(false); + const comparatorSelectRef = useRef(null); + const [loadedOptionCount, setLoadedOptionCount] = useState(0); + const [optionsTruncated, setOptionsTruncated] = useState(false); const [hasFocusedComparator, setHasFocusedComparator] = useState(false); @@ -387,18 +421,8 @@ const AdhocFilterEditPopoverSimpleTabContent: FC = props => { /> ); - const getOptionsRemaining = () => { - // if select is multi/value is array, we show the options not selected - const valuesFromSuggestionsLength = Array.isArray(comparator) - ? comparator.filter(v => suggestions.includes(v)).length - : 0; - return suggestions ? suggestions.length - valuesFromSuggestionsLength : 0; - }; - const createSuggestionsPlaceholder = () => { - const optionsRemaining = getOptionsRemaining(); - const placeholder = t('%s option(s)', optionsRemaining); - return optionsRemaining ? placeholder : ''; - }; + const createSuggestionsPlaceholder = () => + loadedOptionCount ? t('%s option(s)', loadedOptionCount) : ''; const handleSubjectChange = (subject: string) => { setComparator(undefined); @@ -455,21 +479,63 @@ const AdhocFilterEditPopoverSimpleTabContent: FC = props => { operatorId !== undefined && DISABLE_INPUT_OPERATORS.includes(operatorId as Operators); + const canSuggestComparatorValues = Boolean( + subjectString && + props.datasource?.filter_select && + props.adhocFilter.clause !== Clauses.Having, + ); + const hasComparatorOptions = (operatorId && MULTI_OPERATORS.has(operatorId as Operators)) || - suggestions.length > 0; + canSuggestComparatorValues; + + // AsyncSelect is labelInValue, so the value it is given has to be labelled + // too. Handed a bare value it still renders, but `handleOnDeselect` then + // compares `element.value` against entries that have no `.value`, matches + // nothing, and the tag cannot be removed. + // + // Memoised because AsyncSelect resets its internal selection whenever the + // identity of `value` changes. A fresh array every render would wipe out + // each pick as soon as it was made. + const comparatorSelectValue = useMemo( + () => + Array.isArray(comparator) + ? comparator.map(toLabeledValue) + : isDefined(comparator) && comparator !== '' + ? toLabeledValue(comparator) + : undefined, + [comparator], + ); + + const handleComparatorChange = useCallback( + (value: unknown) => { + onComparatorChange(unwrapComparator(value) as string); + }, + // eslint-disable-next-line react-hooks/exhaustive-deps + [props.adhocFilter, props.onChange], + ); const comparatorSelectProps = { allowClear: true, allowNewOptions: true, ariaLabel: t('Comparator option'), + pageSize: COMPARATOR_PAGE_SIZE, + // A capped list reads as the whole set unless it says otherwise, so an + // absent value looks like a value that does not exist. Only shown when the + // list is actually cut short. + helperText: optionsTruncated + ? t( + 'Only the first %s values are listed. Type to search all of them, ' + + 'or enter a value that is not listed.', + loadedOptionCount, + ) + : undefined, mode: operatorId && MULTI_OPERATORS.has(operatorId as Operators) ? ('multiple' as const) : ('single' as const), - loading: loadingComparatorSuggestions, - value: comparator as SelectValue, - onChange: onComparatorChange, + value: comparatorSelectValue as SelectValue, + onChange: handleComparatorChange, notFoundContent: t('Type a value here'), placeholder: createSuggestionsPlaceholder(), }; @@ -495,76 +561,89 @@ const AdhocFilterEditPopoverSimpleTabContent: FC = props => { onChange: onDatePickerChange, }); - useEffect(() => { - const refreshComparatorSuggestions = () => { - const { datasource } = props; - const col = props.adhocFilter.subject; - const having = props.adhocFilter.clause === Clauses.Having; + // Element-level array operators (Contains any / Contains all) search inside + // the array, so suggest individual elements; whole-array operators (=, In, …) + // keep the default distinct-array suggestions. + const arrayElements = + props.adhocFilter.operatorId === Operators.ContainsAny || + props.adhocFilter.operatorId === Operators.ContainsAll; - if (col && datasource && datasource.filter_select && !having) { - const controller = new AbortController(); - const { signal } = controller; - if (loadingComparatorSuggestions) { - controller.abort(); - } - // Element-level array operators (Contains any / Contains all) search - // inside the array, so suggest individual elements; whole-array - // operators (=, In, …) keep the default distinct-array suggestions. - const { operatorId } = props.adhocFilter; - const arrayElements = - operatorId === Operators.ContainsAny || - operatorId === Operators.ContainsAll; - setLoadingComparatorSuggestions(true); - SupersetClient.get({ - signal, - endpoint: `/api/v1/datasource/${datasource.type}/${datasource.id}/column/${col}/values/${ - arrayElements ? '?array_elements=true' : '' - }`, - }) - .then(({ json }) => { - setSuggestions( - json.result.map((suggestion: unknown) => { - // Complex column values arrive as JS arrays or objects: whole - // arrays for MULTI_VALUE columns (e.g. [5, 6, 7]) and Map/Tuple - // objects for nested-container columns (e.g. {"a": ["x","y"]}). - // A raw array/object is neither a valid single-select value - // (antd collapses an array to its first element) nor renderable - // as a React child (an object throws). Render it as its literal - // string, which is also exactly what the backend's - // parse_array_literal expects for the whole-array operators. - if (suggestion !== null && typeof suggestion === 'object') { - const literal = JSON.stringify(suggestion); - return { value: literal, label: literal }; - } - return { - value: suggestion as null | number | boolean | string, - label: optionLabel( - suggestion as null | number | boolean | string, - ), - }; - }), - ); - setLoadingComparatorSuggestions(false); - }) - .catch(() => { - setSuggestions([]); - setLoadingComparatorSuggestions(false); - }); + // AsyncSelect throws away every loaded option when the identity of its + // `options` callback changes, so this depends on plain values rather than on + // `props.datasource`, whose identity the parent does not guarantee. + const datasourceType = props.datasource?.type; + const datasourceId = props.datasource?.id; + + const loadComparatorOptions = useCallback( + async (search: string): Promise => { + const col = subjectString; + if (!col || !canSuggestComparatorValues) { + return { data: [], totalCount: 0 }; } - }; - if (!datePicker) { - refreshComparatorSuggestions(); - } - // loadingComparatorSuggestions intentionally omitted - set inside effect, would cause infinite loop - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ - props.adhocFilter.subject, - props.adhocFilter.clause, - props.adhocFilter.operatorId, - props.datasource, - datePicker, - ]); + const params = new URLSearchParams(); + if (arrayElements) { + params.set('array_elements', 'true'); + } + if (search) { + params.set('q', search); + } + const query = params.toString(); + + try { + const { json } = await SupersetClient.get({ + endpoint: + `/api/v1/datasource/${datasourceType}/${datasourceId}` + + `/column/${encodeURIComponent(col)}/values/${query ? `?${query}` : ''}`, + }); + const data = json.result.map((suggestion: unknown) => { + // Complex column values arrive as JS arrays or objects: whole arrays + // for MULTI_VALUE columns (e.g. [5, 6, 7]) and Map/Tuple objects for + // nested-container columns (e.g. {"a": ["x","y"]}). A raw + // array/object is neither a valid single-select value (antd collapses + // an array to its first element) nor renderable as a React child (an + // object throws). Render it as its literal string, which is also + // exactly what the backend's parse_array_literal expects for the + // whole-array operators. + if (suggestion !== null && typeof suggestion === 'object') { + const literal = JSON.stringify(suggestion); + return { value: literal, label: literal }; + } + return { + value: suggestion as null | number | boolean | string, + label: optionLabel(suggestion as null | number | boolean | string), + }; + }); + + setLoadedOptionCount(data.length); + setOptionsTruncated(isDefined(json.limit) && data.length >= json.limit); + + // The count has to exceed what was returned. AsyncSelect treats + // `loaded >= totalCount` as "that is every value", sets allValuesLoaded + // and from then on serves searches by filtering the loaded page + // client-side -- which is the behaviour this whole change exists to + // replace. Pagination is held off by COMPARATOR_PAGE_SIZE instead. + return { data, totalCount: data.length + 1 }; + } catch { + setLoadedOptionCount(0); + setOptionsTruncated(false); + return { data: [], totalCount: 0 }; + } + }, + [ + subjectString, + canSuggestComparatorValues, + datasourceType, + datasourceId, + arrayElements, + ], + ); + + // Options are cached per search term inside AsyncSelect; a different column + // or a switch to element-level suggestions invalidates all of them. + useEffect(() => { + comparatorSelectRef.current?.clearCache(); + }, [subjectString, arrayElements]); useEffect(() => { if (isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)) { @@ -670,11 +749,12 @@ const AdhocFilterEditPopoverSimpleTabContent: FC = props => { } > diff --git a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx index 39d1ed3756b..bca8c575490 100644 --- a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx +++ b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx @@ -879,10 +879,41 @@ describe('SelectFilterPlugin', () => { expect(await screen.findByTitle('brand-new')).toBeInTheDocument(); }); - test('does not show create option when searchAllOptions is true', () => { + test('says the list is capped when it hits the row limit', async () => { + // 3 rows of data against a limit of 3: the user is looking at a page, not + // at every value the column has. + getWrapper({ rowLimit: 3 }); + userEvent.click(screen.getAllByRole('combobox')[0]); + expect( + await screen.findByText(/Only the first 3 values are listed/), + ).toBeInTheDocument(); + }); + + test('offers the ways out that the filter actually supports', async () => { + getWrapper({ rowLimit: 3, creatable: true, searchAllOptions: true }); + userEvent.click(screen.getAllByRole('combobox')[0]); + expect( + await screen.findByText(/Type to search all of them/), + ).toBeInTheDocument(); + expect( + screen.getByText(/You can enter a value that is not listed/), + ).toBeInTheDocument(); + }); + + test('says nothing when the whole column fits under the limit', async () => { + getWrapper(); + userEvent.click(screen.getAllByRole('combobox')[0]); + expect(await screen.findByRole('combobox')).toBeInTheDocument(); + expect(screen.queryByText(/Only the first/)).not.toBeInTheDocument(); + }); + + test('shows create option when searchAllOptions is true', async () => { + // Server-side search returns a bounded page, so a value that exists in the + // data can still be missing from the dropdown. Suppressing the create + // option there leaves the user with no way to apply it at all. getWrapper({ creatable: true, searchAllOptions: true }); userEvent.type(screen.getByRole('combobox'), 'brand-new'); - expect(screen.queryByTitle('brand-new')).not.toBeInTheDocument(); + expect(await screen.findByTitle('brand-new')).toBeInTheDocument(); }); }); diff --git a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx index d1739c857e6..0737e7c7ef9 100644 --- a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx +++ b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx @@ -271,7 +271,10 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { type: 'ownState', ownState: { coltypeMap: initialColtypeMap, - search, + // The dropdown offers `stripSurroundingQuotes(search)` as the + // creatable option, so the server has to be asked for the same + // string or the two disagree about what was searched for. + search: stripSurroundingQuotes(search).trim(), }, }); } @@ -281,8 +284,10 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { const handleBlur = useCallback(() => { unsetFocusedFilter(); - onSearch(''); - }, [onSearch, unsetFocusedFilter]); + if (search) { + onSearch(''); + } + }, [onSearch, search, unsetFocusedFilter]); const handleChange = useCallback( (value?: SelectValue | number | string) => { @@ -304,6 +309,25 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { ? t('No data') : tn('%s option', '%s options', data.length, data.length); + // A capped list reads as the whole set, so a value sitting past the row + // limit looks like a value that does not exist. Each sentence is only added + // when it is actually true of this filter's configuration. + const rowLimit = Number(formData.rowLimit) || 0; + const helperText = useMemo(() => { + if (!rowLimit || data.length < rowLimit) { + return undefined; + } + return [ + t('Only the first %s values are listed.', data.length), + searchAllOptions ? t('Type to search all of them.') : undefined, + creatable !== false + ? t('You can enter a value that is not listed.') + : undefined, + ] + .filter(Boolean) + .join(' '); + }, [creatable, data.length, rowLimit, searchAllOptions]); + const formItemExtra = useMemo(() => { if (filterState.validateMessage) { return ( @@ -336,7 +360,6 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { const unquotedSearch = stripSurroundingQuotes(search); if ( unquotedSearch && - !searchAllOptions && creatable !== false && !hasOption(unquotedSearch, uniqueOptions, true) ) { @@ -346,7 +369,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { ]; } return uniqueOptions; - }, [search, uniqueOptions, creatable, searchAllOptions]); + }, [search, uniqueOptions, creatable]); const sortComparator = useCallback( (a: LabeledValue, b: LabeledValue) => { @@ -617,7 +640,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { name={formData.nativeFilterId} allowClear autoClearSearchValue - allowNewOptions={!searchAllOptions && creatable !== false} + allowNewOptions={creatable !== false} allowNewOptionsOnPaste={multiSelect && searchAllOptions} allowSelectAll={!searchAllOptions} value={multiSelect ? filterState.value || [] : filterState.value} @@ -626,6 +649,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { showSearch={showSearch} mode={multiSelect ? 'multiple' : 'single'} placeholder={placeholderText} + helperText={helperText} onClear={() => onSearch('')} onSearch={onSearch} onBlur={handleBlur} diff --git a/superset-frontend/src/filters/components/Select/buildQuery.test.ts b/superset-frontend/src/filters/components/Select/buildQuery.test.ts index 2870543ee6c..a3557df4463 100644 --- a/superset-frontend/src/filters/components/Select/buildQuery.test.ts +++ b/superset-frontend/src/filters/components/Select/buildQuery.test.ts @@ -117,6 +117,38 @@ describe('Select buildQuery', () => { ]); }); + test('should not sort by the searched column', () => { + // Ordering by a high-cardinality column makes the engine sort every match + // before applying the row limit; the dropdown re-sorts the page anyway. + const queryContext = buildQuery( + { ...formData, sortAscending: true }, + { + ownState: { + search: 'abc', + coltypeMap: { my_col: GenericDataType.String }, + }, + }, + ); + const [query] = queryContext.queries; + expect(query.orderby).toEqual([]); + }); + + test('should keep the sort metric while searching', () => { + // A sort metric decides which rows come back, so dropping it would change + // the result set rather than just its order. + const queryContext = buildQuery( + { ...formData, sortMetric: 'my_metric', sortAscending: false }, + { + ownState: { + search: 'abc', + coltypeMap: { my_col: GenericDataType.String }, + }, + }, + ); + const [query] = queryContext.queries; + expect(query.orderby).toEqual([['my_metric', false]]); + }); + test('should add text search parameter for numeric to query filter', () => { const queryContext = buildQuery(formData, { ownState: { diff --git a/superset-frontend/src/filters/components/Select/buildQuery.ts b/superset-frontend/src/filters/components/Select/buildQuery.ts index 1e80a32483a..e9b404119ac 100644 --- a/superset-frontend/src/filters/components/Select/buildQuery.ts +++ b/superset-frontend/src/filters/components/Select/buildQuery.ts @@ -54,6 +54,13 @@ const buildQuery: BuildQuery = ( } const sortColumns = sortMetric ? [sortMetric] : columns; + // Sorting by the searched column makes the engine scan and sort every + // match before applying the row limit, which is the dominant cost of + // search-as-you-type on a high-cardinality column. The dropdown re-sorts + // the returned page client-side, so the server sort buys nothing here. A + // sort metric is different: it selects *which* rows come back, so it has + // to stay. + const skipOrderBy = !!search && !sortMetric; const query: QueryObject[] = [ { ...baseQueryObject, @@ -61,7 +68,7 @@ const buildQuery: BuildQuery = ( metrics: sortMetric ? [sortMetric] : [], filters: filters.concat(extraFilters), orderby: - sortMetric || sortAscending !== undefined + !skipOrderBy && (sortMetric || sortAscending !== undefined) ? sortColumns.map(column => [column, !!sortAscending]) : [], }, diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index b053cc63561..2409c2ffae4 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -964,6 +964,7 @@ class AnnotationDatasource(BaseDatasource): limit: int = 10000, denormalize_column: bool = False, array_elements: bool = False, + search: str | None = None, ) -> list[Any]: raise NotImplementedError() diff --git a/superset/datasource/api.py b/superset/datasource/api.py index 4e09e3edad8..a9f3bc9d351 100644 --- a/superset/datasource/api.py +++ b/superset/datasource/api.py @@ -41,6 +41,9 @@ from superset.views.base_api import BaseSupersetApi, statsd_metrics logger = logging.getLogger(__name__) +# Cache lifetime for search-filtered column values, in seconds. +SEARCH_CACHE_TIMEOUT = 60 + class DatasourceRestApi(BaseSupersetApi): allow_browser_login = True @@ -87,6 +90,14 @@ class DatasourceRestApi(BaseSupersetApi): type: string name: column_name description: The name of the column to get values for + - in: query + schema: + type: string + name: q + description: >- + Optional case-insensitive substring; only values containing it are + returned. Lets the client search the full column rather than the + truncated first page. responses: 200: description: A List of distinct values for the column @@ -136,6 +147,10 @@ class DatasourceRestApi(BaseSupersetApi): # Element-level operators (Contains any / Contains all) request the # distinct array *elements* rather than distinct whole arrays. array_elements = parse_boolean_string(request.args.get("array_elements")) + # Server-side search. Without it the client can only match against the + # bounded first page, so a value beyond ``FILTER_SELECT_ROW_LIMIT`` is + # unfindable on a high-cardinality column. + search = (request.args.get("q") or "").strip() or None # Cache distinct column-value results so a dashboard with many filters # backed by the same (often heavy) virtual dataset doesn't re-execute @@ -169,6 +184,7 @@ class DatasourceRestApi(BaseSupersetApi): "limit": row_limit, "denorm": denormalize_column, "elements": array_elements, + "q": search, "rls": security_manager.get_rls_cache_key(datasource), "changed_on": str(getattr(datasource, "changed_on", "")), }, @@ -184,7 +200,7 @@ class DatasourceRestApi(BaseSupersetApi): logger.debug( "column-values cache HIT: uid=%s col=%s", datasource.uid, column_name ) - response = self.response(200, result=cached) + response = self.response(200, result=cached, limit=row_limit) response.headers["X-Cache-Status"] = "HIT" return response @@ -194,6 +210,7 @@ class DatasourceRestApi(BaseSupersetApi): limit=row_limit, denormalize_column=denormalize_column, array_elements=array_elements, + search=search, ) except KeyError: return self.response( @@ -225,11 +242,15 @@ class DatasourceRestApi(BaseSupersetApi): timeout = datasource.cache_timeout or app.config.get( "CACHE_DEFAULT_TIMEOUT", 300 ) + if search: + # Every distinct search term is its own key, so a few users typing + # would otherwise pin one entry per keystroke for the full timeout. + timeout = min(timeout, SEARCH_CACHE_TIMEOUT) cache_manager.data_cache.set(cache_key, payload, timeout=timeout) logger.debug( "column-values cache MISS: uid=%s col=%s", datasource.uid, column_name ) - response = self.response(200, result=payload) + response = self.response(200, result=payload, limit=row_limit) response.headers["X-Cache-Status"] = "MISS" return response diff --git a/superset/models/helpers.py b/superset/models/helpers.py index e4dc83f0552..257a8344ef4 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -198,6 +198,41 @@ def get_effective_hours_offset( R_SUFFIX = "__right_suffix" +# Escape character for LIKE patterns built from user-supplied search text. +# Deliberately not a backslash: dialects that escape backslashes when rendering +# string literals would emit a two-character ESCAPE clause, which is a syntax +# error on engines that honour standard-conforming strings. +LIKE_ESCAPE_CHAR = "!" + + +def escape_like_pattern(value: str) -> str: + """ + Neutralize LIKE wildcards in user-supplied search text. + + Without this a user typing ``%`` or ``_`` would match every row, which is + both wrong and, on a large table, a scan the search was meant to avoid. + """ + return ( + value.replace(LIKE_ESCAPE_CHAR, LIKE_ESCAPE_CHAR * 2) + .replace("%", f"{LIKE_ESCAPE_CHAR}%") + .replace("_", f"{LIKE_ESCAPE_CHAR}_") + ) + + +def build_like_predicate( + expr: ColumnElement[Any], + search: str, +) -> ColumnElement[Any]: + """ + Build a case-insensitive containment predicate for ``expr``. + + ``lower(expr) LIKE lower('%term%')`` is used rather than ``ILIKE`` because + the latter is not portable across engines. + """ + pattern = f"%{escape_like_pattern(search)}%".lower() + return sa.func.lower(expr).like(pattern, escape=LIKE_ESCAPE_CHAR) + + def _normalize_mssql_virtual_dataset_sql( sql: str, parsed_script: SQLScript, engine: str ) -> str: @@ -4010,6 +4045,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods limit: int = 10000, denormalize_column: bool = False, array_elements: bool = False, + search: str | None = None, ) -> list[Any]: # denormalize column name before querying for values # unless disabled in the dataset configuration @@ -4047,6 +4083,9 @@ class ExploreMixin: # pylint: disable=too-many-public-methods .select_from(tbl) .distinct() ) + if search: + qry = qry.where(build_like_predicate(value_expr, search)) + if limit: qry = qry.limit(limit) diff --git a/tests/integration_tests/datasource/api_tests.py b/tests/integration_tests/datasource/api_tests.py index 4f9f7d85dcf..241fc7faed4 100644 --- a/tests/integration_tests/datasource/api_tests.py +++ b/tests/integration_tests/datasource/api_tests.py @@ -155,6 +155,7 @@ class TestDatasourceApi(SupersetTestCase): limit=10000, denormalize_column=False, array_elements=False, + search=None, ) @pytest.mark.usefixtures("app_context", "virtual_dataset") @@ -170,6 +171,79 @@ class TestDatasourceApi(SupersetTestCase): ) assert values_for_column_mock.call_args.kwargs["array_elements"] is True + @pytest.mark.usefixtures("app_context", "virtual_dataset") + def test_get_column_values_search_filters_server_side(self): + """``?q=`` narrows the values in the database rather than client-side, + which is what makes a value beyond the row limit reachable at all.""" + self.login(ADMIN_USERNAME) + table = self.get_virtual_dataset() + rv = self.client.get( + f"api/v1/datasource/table/{table.id}/column/col2/values/?q=b" + ) + assert rv.status_code == 200 + assert json.loads(rv.data.decode("utf-8"))["result"] == ["b"] + + @pytest.mark.usefixtures("app_context", "virtual_dataset") + def test_get_column_values_search_is_case_insensitive(self): + self.login(ADMIN_USERNAME) + table = self.get_virtual_dataset() + rv = self.client.get( + f"api/v1/datasource/table/{table.id}/column/col2/values/?q=B" + ) + assert rv.status_code == 200 + assert json.loads(rv.data.decode("utf-8"))["result"] == ["b"] + + @pytest.mark.usefixtures("app_context", "virtual_dataset") + def test_get_column_values_search_escapes_wildcards(self): + """A literal ``%`` must not be treated as "match everything".""" + self.login(ADMIN_USERNAME) + table = self.get_virtual_dataset() + rv = self.client.get( + f"api/v1/datasource/table/{table.id}/column/col2/values/?q=%25" + ) + assert rv.status_code == 200 + assert json.loads(rv.data.decode("utf-8"))["result"] == [] + + @pytest.mark.usefixtures("app_context", "virtual_dataset") + @patch("superset.models.helpers.ExploreMixin.values_for_column") + def test_get_column_values_blank_search_is_ignored(self, values_for_column_mock): + """Whitespace is not a search term; it must not narrow the list.""" + values_for_column_mock.return_value = [] + self.login(ADMIN_USERNAME) + table = self.get_virtual_dataset() + self.client.get( + f"api/v1/datasource/table/{table.id}/column/col2/values/?q=%20%20" + ) + assert values_for_column_mock.call_args.kwargs["search"] is None + + @pytest.mark.usefixtures("app_context", "virtual_dataset") + def test_get_column_values_returns_applied_limit(self): + """The client needs the limit to tell a short list from a truncated + one, so it can say the list is partial instead of implying it is whole.""" + self.login(ADMIN_USERNAME) + table = self.get_virtual_dataset() + rv = self.client.get(f"api/v1/datasource/table/{table.id}/column/col2/values/") + assert rv.status_code == 200 + assert json.loads(rv.data.decode("utf-8"))["limit"] == 10000 + + @pytest.mark.usefixtures("app_context", "virtual_dataset") + @patch("superset.models.helpers.ExploreMixin.values_for_column") + def test_get_column_values_cache_isolated_per_search(self, values_for_column_mock): + """Search terms must partition the cache; sharing one entry would serve + the results of somebody else's search.""" + cache_manager.data_cache.clear() + values_for_column_mock.return_value = ["x"] + self.login(ADMIN_USERNAME) + table = self.get_virtual_dataset() + url = f"api/v1/datasource/table/{table.id}/column/col2/values/" + + self.client.get(url) + self.client.get(f"{url}?q=a") + self.client.get(f"{url}?q=b") + self.client.get(f"{url}?q=a") + + assert values_for_column_mock.call_count == 3 + @pytest.mark.usefixtures("app_context", "virtual_dataset") @patch("superset.db_engine_specs.base.BaseEngineSpec.denormalize_name") def test_get_column_values_not_denormalize_column(self, denormalize_name_mock): @@ -191,6 +265,7 @@ class TestDatasourceApi(SupersetTestCase): limit=10000, denormalize_column=True, array_elements=False, + search=None, ) @pytest.mark.usefixtures("app_context", "virtual_dataset") diff --git a/tests/unit_tests/models/helpers_test.py b/tests/unit_tests/models/helpers_test.py index ca89ad7e622..47f97f0239c 100644 --- a/tests/unit_tests/models/helpers_test.py +++ b/tests/unit_tests/models/helpers_test.py @@ -105,6 +105,86 @@ def test_values_for_column(database: Database) -> None: assert table.values_for_column("a") == [1, None] +@pytest.mark.parametrize( + "raw,expected", + [ + ("plain", "plain"), + ("50%", "50!%"), + ("a_b", "a!_b"), + ("wow!", "wow!!"), + ("!%_", "!!!%!_"), + ], +) +def test_escape_like_pattern(raw: str, expected: str) -> None: + """Wildcards typed by a user are data, not pattern syntax.""" + from superset.models.helpers import escape_like_pattern + + assert escape_like_pattern(raw) == expected + + +def test_build_like_predicate_is_case_insensitive_and_escaped() -> None: + import sqlalchemy as sa + + from superset.models.helpers import build_like_predicate + + compiled = str( + build_like_predicate(sa.column("c"), "50%").compile( + dialect=sa.dialects.registry.load("postgresql")(), + compile_kwargs={"literal_binds": True}, + ) + ).replace("%%", "%") + + assert compiled == "lower(c) LIKE '%50!%%' ESCAPE '!'" + + +def test_values_for_column_search(database: Database) -> None: + """``search`` narrows the distinct-value query in the database.""" + import pandas as pd + + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[TableColumn(column_name="a")], + ) + + with patch( + "pandas.read_sql_query", + return_value=pd.DataFrame({"column_values": ["Alice"]}), + ) as read_sql_query: + assert table.values_for_column("a", search="ali") == ["Alice"] + + sql = str(read_sql_query.call_args.kwargs["sql"]) + assert "LIKE" in sql + assert "'%ali%'" in sql + + +def test_values_for_column_without_search_has_no_predicate( + database: Database, +) -> None: + """The unsearched list must stay a plain bounded DISTINCT scan.""" + import pandas as pd + + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[TableColumn(column_name="a")], + ) + + with patch( + "pandas.read_sql_query", + return_value=pd.DataFrame({"column_values": ["Alice"]}), + ) as read_sql_query: + table.values_for_column("a") + + assert "LIKE" not in str(read_sql_query.call_args.kwargs["sql"]) + + def test_values_for_column_passes_catalog_and_schema( mocker: MockerFixture, session: Session,