Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Improve sorting for new chart dataset select #16414

Closed
wants to merge 1 commit into from
Closed

feat: Improve sorting for new chart dataset select #16414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 33 additions & 9 deletions superset-frontend/src/addSlice/AddSliceContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export type AddSliceContainerProps = {};
export type AddSliceContainerState = {
datasource?: { label: string; value: string };
visType: string | null;
search: string;
};

const ESTIMATED_NAV_HEIGHT = 56;
Expand Down Expand Up @@ -186,6 +187,7 @@ export default class AddSliceContainer extends React.PureComponent<
super(props);
this.state = {
visType: null,
search: '',
};

this.changeDatasource = this.changeDatasource.bind(this);
Expand All @@ -194,6 +196,8 @@ export default class AddSliceContainer extends React.PureComponent<
this.newLabel = this.newLabel.bind(this);
this.loadDatasources = this.loadDatasources.bind(this);
this.handleFilterOption = this.handleFilterOption.bind(this);
Copy link
Member

@ktmud ktmud Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just replace both filterOption and filterSort with match-sorter. It has worked well for the Explore page Dataset side panel.

This basically means controlling the filtering outside of AntD Select---which should give more consistent filtering behavior across the board.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah, i haven't seen match-sorter yet. i think that'll work? although antd select has made it tricky to take control over filtering and sorting. i'll play with it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktmud Do you think we should try and add this to the default Select component in superset? just apply it across the board?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that'd be ideal. The Select component can provide a matchSorterKey prop to override the default match-sorting keys.

Copy link
Member

@michael-s-molina michael-s-molina Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktmud @etr2460 filterSort is useful when all the values are present in the select. It's less useful when we are using the async version of our select, which is the majority of the cases. In the async version, the sorting should be done on the server-side before the pagination to ensure the correct results are returned. Client-side sorting on paginated requests can produce wrong results. We can use match-sorter for the static version but so far we haven't found any case where the default sorting wasn't sufficient. I think the best way to fix this is to make the endpoint respect the sorting parameters and implement the exact match behavior on the server-side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, bumping exact matches is probably good enough for now.

It'd definitely be nice to have paginated backend searches. Hopefully we'll come up with a solution that's generalized enough even for omni-searches in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going down this path, then maybe we need elasticsearch on the backend :P mysql isn't great for search

I'd love to make this an optional search backend! 🍻

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we all agree here. Thanks @villebro ! do you think i should close this pr then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etr2460 I have a POC in mind but it would introduce a fairly significant change on FAB, so I want to discuss it first with Daniel before starting the work, and he's currently on PTO. I don't expect my POC to be ready for another few weeks (will require review and a new release), so if this is needed in the interim then we can merge this IMO unless @michael-s-molina has some objections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@villebro I don't have any objections as a temporary solution. We can come back later and remove the client-side sorting. We'll probably need to do that anyway with other selects.

Before merging, we need to also sort the values when the user opens the select as stated here #16414 (comment)

this.handleFilterSort = this.handleFilterSort.bind(this);
this.handleSearch = this.handleSearch.bind(this);
}

exploreUrl() {
Expand Down Expand Up @@ -241,6 +245,10 @@ export default class AddSliceContainer extends React.PureComponent<
);
}

handleSearch(search: string) {
this.setState({ search });
}

loadDatasources(search: string, page: number, pageSize: number) {
const query = rison.encode({
columns: ['id', 'table_name', 'description', 'datasource_type'],
Expand All @@ -256,15 +264,12 @@ export default class AddSliceContainer extends React.PureComponent<
const list: {
label: string;
value: string;
}[] = response.json.result
.map((item: Dataset) => ({
value: `${item.id}__${item.datasource_type}`,
label: this.newLabel(item),
labelText: item.table_name,
}))
.sort((a: { labelText: string }, b: { labelText: string }) =>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun fact, this didn't do anything since the selector re-sorts the options again :P Adding a filterSort prop to the selector is how you actually sort the items in the dropdown

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etr2460 filterSort only sorts when searching. If the user just opens the select the values will be unsorted if we remove these lines. We'll probably need to apply your sort function here as well 😉

Screen.Recording.2021-08-24.at.7.29.40.AM.mov

Copy link
Member

@michael-s-molina michael-s-molina Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etr2460 The ideal solution would be to disable the filterSort when the Select is in async mode and sort the values on the server-side to correctly handle the pagination. This endpoint is not respecting the order_column and order_direction props, so we would need to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, interesting, i didn't even see results rendering when clicking into the modal without typing anything. although maybe that's because in our env we have too many datasets so it didn't render fast enough. i'll see what i can do to fix the sorting for all use cases here

Copy link
Member

@michael-s-molina michael-s-molina Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etr2460 for the scenario where the paginated query with no search (100 results by default) is heavy, we created a prop called fetchOnlyOnSearch. What we need to do is make it configurable by organizations.

a.labelText.localeCompare(b.labelText),
);
}[] = response.json.result.map((item: Dataset) => ({
value: `${item.id}__${item.datasource_type}`,
label: this.newLabel(item),
labelText: item.table_name,
}));

return {
data: list,
totalCount: response.json.count,
Expand All @@ -281,6 +286,23 @@ export default class AddSliceContainer extends React.PureComponent<
return labelText.toLowerCase().includes(searchValue);
}

handleFilterSort(
a: { label: string; value: number; labelText: string },
b: { label: string; value: number; labelText: string },
) {
const optionA = a.labelText.toLowerCase();
const optionB = b.labelText.toLowerCase();
const search = this.state.search.toLowerCase();

if (optionA === search) {
return -1;
}
if (optionB === search) {
return 1;
}
return optionA.localeCompare(optionB);
}

render() {
const isButtonDisabled = this.isBtnDisabled();
return (
Expand All @@ -297,11 +319,13 @@ export default class AddSliceContainer extends React.PureComponent<
ariaLabel={t('Dataset')}
name="select-datasource"
filterOption={this.handleFilterOption}
filterSort={this.handleFilterSort}
onChange={this.changeDatasource}
options={this.loadDatasources}
placeholder={t('Choose a dataset')}
showSearch
value={this.state.datasource}
onSearch={this.handleSearch}
/>
<span>
{t(
Expand Down
2 changes: 2 additions & 0 deletions superset-frontend/src/components/Select/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ type PickedSelectProps = Pick<
| 'value'
| 'disabled'
| 'filterOption'
| 'filterSort'
| 'notFoundContent'
| 'onChange'
| 'onSearch'
| 'placeholder'
| 'showSearch'
| 'value'
Expand Down