[GitHub] [superset] michael-s-molina commented on pull request #16483: refactor: Changes the DatabaseSelector and TableSelector to use the new Select component

[GitBox <gi...@apache.org>]

michael-s-molina commented on pull request #16483:
URL: https://github.com/apache/superset/pull/16483#issuecomment-910467906


   > 1. For db types, can we use the same tag style instead of just text?
   
   Done
   
   > 2. Loading state should show "Loading..." instead of "No data"
   
   Done in https://github.com/apache/superset/pull/16531
   
   > 3. Previously the Select will automatically trigger the initial load of the tables list if there is a selected schema and the schema list if there is a selected database (just select a table and refresh the page to test this), now you have to focus on the tables select to trigger the loading. I think we should keep this preload behavior because it can be quite slow to load these APIs.
   
   The idea here is to avoid unnecessary queries to the server. This component is used in Explore, SQL Editor, and the datasets list. Many times the user does not modify any of the selects but they are rendered. Some examples:
   - When the user enters the datasets modal from the dataset list to edit anything from the other tabs
   - When changing or just viewing a query in SQL Editor
   
   I do understand your concern though. Here we have a limitation because the tables endpoint does not currently support pagination. In talks with @villebro to add pagination support we ended up with the following:
   
   > That tables API will be horrible to paginate, as SqlAlchemy doesn’t natively support pagination of retrieval of table names.
   I can open up a discussion on the sqla repo to see if they're open to adding the optional feature, but it will probably take a long time to get it in.
   
   So one of the reasons for the slow behavior is the lack of pagination support. For cases where the queries are heavy even with pagination, we created a prop called `fetchOnlyOnSearch` that only triggers a filtered query. I think Airbnb should enable this prop for this case given that you have a schema with 30k tables.
   
   > 4. Scrolling tables list to the end will trigger an unnecessary new query----probably trying to fetch the next page?
   
   This was a result of the lack of pagination support. The component thinks that we have more results available given the page size and total results. To overcome this, while we don't add pagination support to this endpoint, I added the following:
   
   ```
   // TODO: Would be nice to add pagination in a follow-up. Needs endpoint changes.
   pageSize={Number.MAX_SAFE_INTEGER}
   ```
   
   This is just disabling the pagination in practice. We don't want to provide a prop to disable the pagination because we want to make sure all endpoints support this feature. This is especially important if we think about devices with slow connections that are using Superset endpoints.
   
   > It also freezes the whole page on our one of our schemas with 30k tables.
   
   I tested here with 50k values and the Select was able to render smoothly. Virtualization is native in Ant Design Select. In their page, we have an [example](https://ant.design/components/select/#API) with 100k items.  So I don't know if it froze because of query performance or another reason. Maybe we can test again with `fetchOnlyOnSearch` enabled.
   
   > 5. Switching schema does not clear the selected table--even though the table does not exist in the new schema.
   
   Fixed.
   
   @ktmud @etr2460 @villebro Thank you so much for the tests, reviews and help improving this feature.


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