feat: Improve sorting for new chart dataset select #16414

etr2460 · 2021-08-24T00:37:01Z

SUMMARY

#16200 updated the add chart page and as a side effect started showing virtual datasets in the dropdown. This was probably a good thing all told, but at Airbnb we had a bunch of virtual datasets with similar names to physical datasets that then made it difficult/impossible to find the original physical dataset in the selector.

This change will elevate an exact match dataset name to the top of the selector if one exists.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

created datasets with very similar names, ensured that the one with an exact matching name appears first

ADDITIONAL INFORMATION

Has associated issue: [Gallery] Less-blocking creating new chart flow #16173
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

to: @michael-s-molina @ktmud @suddjian

etr2460 · 2021-08-24T00:37:47Z

superset-frontend/src/addSlice/AddSliceContainer.tsx

-          label: this.newLabel(item),
-          labelText: item.table_name,
-        }))
-        .sort((a: { labelText: string }, b: { labelText: string }) =>


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

@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

@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.

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

@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.

codecov · 2021-08-24T01:02:28Z

Codecov Report

Merging #16414 (7ea08cb) into master (c768941) will decrease coverage by 0.01%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master   #16414      +/-   ##
==========================================
- Coverage   76.55%   76.54%   -0.02%     
==========================================
  Files        1000     1000              
  Lines       53483    53495      +12     
  Branches     6818     6820       +2     
==========================================
+ Hits        40945    40947       +2     
- Misses      12300    12310      +10     
  Partials      238      238

Flag	Coverage Δ
javascript	`70.67% <14.28%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/components/Select/Select.tsx	`76.57% <ø> (ø)`
...perset-frontend/src/addSlice/AddSliceContainer.tsx	`51.31% <14.28%> (-5.61%)`	⬇️
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`44.24% <0.00%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c768941...7ea08cb. Read the comment docs.

ktmud · 2021-08-24T17:06:03Z

superset-frontend/src/addSlice/AddSliceContainer.tsx

@@ -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);


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.

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

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

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

@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.

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.

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! 🍻

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

@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.

@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)

john-bodley · 2021-10-04T17:07:42Z

@villebro based on @michael-s-molina's comment are you ok moving forward with said change, especially given—per @etr2460's PR description—the UX is severely impacted at Airbnb or likely any other installation which contains a large number of datasets etc.

stale · 2022-04-16T14:56:21Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

feat: Improve sorting for new chart dataset select

7ea08cb

pull-request-size bot added the size/M label Aug 24, 2021

etr2460 commented Aug 24, 2021

View reviewed changes

ktmud reviewed Aug 24, 2021

View reviewed changes

michael-s-molina requested a review from villebro August 24, 2021 22:50

ktmud mentioned this pull request Feb 22, 2022

feat(select): sort exact and startsWith match to first #18856

Merged

9 tasks

stale bot added the inactive Inactive for >= 30 days label Apr 16, 2022

stale bot closed this Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improve sorting for new chart dataset select #16414

feat: Improve sorting for new chart dataset select #16414

etr2460 commented Aug 24, 2021

etr2460 Aug 24, 2021

michael-s-molina Aug 24, 2021

michael-s-molina Aug 24, 2021 •

edited

Loading

etr2460 Aug 24, 2021

michael-s-molina Aug 24, 2021 •

edited

Loading

codecov bot commented Aug 24, 2021

ktmud Aug 24, 2021 •

edited

Loading

etr2460 Aug 24, 2021

etr2460 Aug 24, 2021

ktmud Aug 24, 2021

michael-s-molina Aug 24, 2021 •

edited

Loading

ktmud Aug 26, 2021

villebro Aug 26, 2021

etr2460 Aug 26, 2021

villebro Aug 31, 2021

michael-s-molina Aug 31, 2021

john-bodley commented Oct 4, 2021

stale bot commented Apr 16, 2022

feat: Improve sorting for new chart dataset select #16414

feat: Improve sorting for new chart dataset select #16414

Conversation

etr2460 commented Aug 24, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

codecov bot commented Aug 24, 2021

Codecov Report

ktmud Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Oct 4, 2021

stale bot commented Apr 16, 2022

michael-s-molina Aug 24, 2021 •

edited

Loading

michael-s-molina Aug 24, 2021 •

edited

Loading

ktmud Aug 24, 2021 •

edited

Loading

michael-s-molina Aug 24, 2021 •

edited

Loading