You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/08/24 00:37:10 UTC
[GitHub] [superset] etr2460 opened a new pull request #16414: feat: Improve sorting for new chart dataset select
etr2460 opened a new pull request #16414:
URL: https://github.com/apache/superset/pull/16414
### SUMMARY
https://github.com/apache/superset/pull/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
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue: https://github.com/apache/superset/issues/16173
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r696063364
##########
File path: 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);
Review comment:
`match-sorter`'s ranking algorithm is much more sophisticated than the simple compare sorts the built-in API AntD Select can provide. For example, `match-sorter` can filter by abbreviations---e.g. `abc_def_gh` can be matched with `adg`---and each match thresholds can be assigned with different ranking scores: exact match ranks before startsWith, startsWith ranks before abbreviations, etc....
If reproducing the same logics become too cumbersome for the backend (I'd imagine some of these have to be at the SQL level in order to be efficient), we could just let the client side keep fetching next pages until there are enough matched results to be displayed or reached the end of the list/maximum number of items allowed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695013053
##########
File path: superset-frontend/src/addSlice/AddSliceContainer.tsx
##########
@@ -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 }) =>
Review comment:
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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r694734424
##########
File path: superset-frontend/src/addSlice/AddSliceContainer.tsx
##########
@@ -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 }) =>
Review comment:
@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 😉
https://user-images.githubusercontent.com/70410625/130602788-53badf35-a202-4910-840f-2ce71746dabf.mov
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695017314
##########
File path: superset-frontend/src/addSlice/AddSliceContainer.tsx
##########
@@ -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 }) =>
Review comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r696626626
##########
File path: 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);
Review comment:
@etr2460 @ktmud do we agree that we want to paginate these requests, and hence should also handle sorting in the backend? And if so, are we ok with bumping exact matches to the top, or do we need to add some more exotic ordering logic? If this is the case, I can prepare a small POC for the `/related` endpoints to show how this could be done, after which we can consider using the same pattern in all paginated select component requests.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r699160549
##########
File path: 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);
Review comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695164550
##########
File path: 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);
Review comment:
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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695689158
##########
File path: 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);
Review comment:
This would probably be best to add to FAB to avoid having to implement this in every endpoint (I didn't check if there's a filter operator for this specific case, but I doubt it). We could do a two-phase request, where we first look for an exact match and then do the regular match after that, placing the exact match on top if present and adjusting the limit and offset of the regular match accordingly. I could potentially take a stab at doing this if we can agree on the details.
I agree with @michael-s-molina that we should really try to handle sorting server side to make the product handle arbitrarily large datasets well.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r696071451
##########
File path: 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);
Review comment:
If we're going down this path, then maybe we need elasticsearch on the backend :P mysql isn't great for search
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #16414:
URL: https://github.com/apache/superset/pull/16414#issuecomment-904237722
# [Codecov](https://codecov.io/gh/apache/superset/pull/16414?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16414](https://codecov.io/gh/apache/superset/pull/16414?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7ea08cb) into [master](https://codecov.io/gh/apache/superset/commit/c768941f2f662e1a0dfa1e1731319d22ec9ca886?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c768941) will **decrease** coverage by `0.01%`.
> The diff coverage is `14.28%`.
[](https://codecov.io/gh/apache/superset/pull/16414?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16414?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/16414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `76.57% <ø> (ø)` | |
| [...perset-frontend/src/addSlice/AddSliceContainer.tsx](https://codecov.io/gh/apache/superset/pull/16414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FkZFNsaWNlQ29udGFpbmVyLnRzeA==) | `51.31% <14.28%> (-5.61%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/16414/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `44.24% <0.00%> (-0.12%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16414?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16414?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [c768941...7ea08cb](https://codecov.io/gh/apache/superset/pull/16414?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695167256
##########
File path: 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);
Review comment:
Yes, that'd be ideal. The Select component can provide a `matchSorterKey` prop to override the default match-sorting keys.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695268134
##########
File path: 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);
Review comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r696035357
##########
File path: 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);
Review comment:
@villebro If you're open to looking into doing this on the backend, it seems like probably the future proof way to do this. Do you think the 4 of us should sync for 15ish minutes to align on the expected behavior before you start work?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695017314
##########
File path: superset-frontend/src/addSlice/AddSliceContainer.tsx
##########
@@ -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 }) =>
Review comment:
@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 this configurable by organizations.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695041578
##########
File path: 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);
Review comment:
I think we should just replace both `filterOption` and `filterSort` with [`match-sorter`](https://github.com/apache/superset/blob/master/superset-frontend/src/explore/components/DatasourcePanel/index.tsx#L203-L223). 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r696776012
##########
File path: 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);
Review comment:
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r694764064
##########
File path: superset-frontend/src/addSlice/AddSliceContainer.tsx
##########
@@ -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 }) =>
Review comment:
@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 that.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r694398626
##########
File path: superset-frontend/src/addSlice/AddSliceContainer.tsx
##########
@@ -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 }) =>
Review comment:
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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r694764064
##########
File path: superset-frontend/src/addSlice/AddSliceContainer.tsx
##########
@@ -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 }) =>
Review comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695334390
##########
File path: 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);
Review comment:
i do think not putting exact matches first is a clear case where the default sorting isn't sufficient. however, i'm not sure how best to add that to the server-side. i assume it would need to be a FAB change?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r696781799
##########
File path: 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);
Review comment:
> 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! 🍻
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r699207898
##########
File path: 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);
Review comment:
@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 https://github.com/apache/superset/pull/16414#discussion_r694734424
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r699207898
##########
File path: 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);
Review comment:
@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 https://github.com/apache/superset/pull/16414#discussion_r694734424
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r697025196
##########
File path: 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);
Review comment:
I think we all agree here. Thanks @villebro ! do you think i should close this pr then?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #16414:
URL: https://github.com/apache/superset/pull/16414#issuecomment-933681649
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r699207898
##########
File path: 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);
Review comment:
@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 https://github.com/apache/superset/pull/16414#discussion_r694734424
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r699160549
##########
File path: 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);
Review comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695041578
##########
File path: 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);
Review comment:
I think you can just replace `handleFilterOption` with [`match-sorter`](https://github.com/apache/superset/blob/master/superset-frontend/src/explore/components/DatasourcePanel/index.tsx#L203-L223). It has worked well for the Explore page Dataset side panel.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695165936
##########
File path: 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);
Review comment:
@ktmud Do you think we should try and add this to the default Select component in superset? just apply it across the board?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695041578
##########
File path: 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);
Review comment:
I think we should replace both `filterOption` and `filterSort` with [`match-sorter`](https://github.com/apache/superset/blob/master/superset-frontend/src/explore/components/DatasourcePanel/index.tsx#L203-L223). 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r695268134
##########
File path: 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);
Review comment:
@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 sync 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #16414: feat: Improve sorting for new chart dataset select
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16414:
URL: https://github.com/apache/superset/pull/16414#discussion_r699160549
##########
File path: 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);
Review comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org