You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2023/03/25 23:52:25 UTC
[GitHub] [superset] justinpark opened a new pull request, #23488: refactor(sqllab): Remove tableOptions from redux state
justinpark opened a new pull request, #23488:
URL: https://github.com/apache/superset/pull/23488
### SUMMARY
Similar to #23257, this commit migrates the tableOptions state from redux to react-query to reduce the complexity of the queryEditor state.
(The tableOptions state only used in the editor for autocomplete so useQuery is a lighter way to share the state)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
<img width="523" alt="Screenshot 2023-03-25 at 4 47 31 PM" src="https://user-images.githubusercontent.com/1392866/227747790-7c3d0f0c-398a-4099-bf00-eac2bee90d85.png">
<img width="364" alt="Screenshot 2023-03-25 at 4 47 57 PM" src="https://user-images.githubusercontent.com/1392866/227747801-98d35f79-ba58-4c51-8d6d-888545b2057d.png">
Before:
<img width="362" alt="Screenshot_2023-03-25_at_4_48_50_PM" src="https://user-images.githubusercontent.com/1392866/227747806-cd1d3436-3111-4a25-a31b-c749e3f08358.png">
<img width="489" alt="Screenshot_2023-03-25_at_4_50_35_PM" src="https://user-images.githubusercontent.com/1392866/227747811-516cd6f3-48d7-453c-9e13-ebfd5e30502e.png">
### TESTING INSTRUCTIONS
Go to sqllab and select a database to get the associated schemas
Type some part of the table name on the editor and check the autocomplete including the fetched schemas
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] 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
--
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 #23488: refactor(sqllab): Remove tableOptions from redux state
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23488:
URL: https://github.com/apache/superset/pull/23488#issuecomment-1485583282
## [Codecov](https://codecov.io/gh/apache/superset/pull/23488?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 [#23488](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (937c6f8) into [master](https://codecov.io/gh/apache/superset/commit/8f846b7ad0e0b08201a3543f7680c071b3d996f6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8f846b7) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 937c6f8 differs from pull request most recent head 12f21f5. Consider uploading reports for the commit 12f21f5 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23488 +/- ##
=======================================
Coverage 67.64% 67.65%
=======================================
Files 1908 1909 +1
Lines 73739 73735 -4
Branches 7988 7985 -3
=======================================
+ Hits 49879 49883 +4
+ Misses 21814 21813 -1
+ Partials 2046 2039 -7
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.84% <100.00%> (+0.01%)` | :arrow_up: |
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/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...frontend/src/SqlLab/components/SaveQuery/index.tsx](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVRdWVyeS9pbmRleC50c3g=) | `75.55% <ø> (ø)` | |
| [...d/src/SqlLab/components/SqlEditorLeftBar/index.tsx](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIvaW5kZXgudHN4) | `53.73% <ø> (-0.56%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/fixtures.ts](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `37.28% <ø> (+0.20%)` | :arrow_up: |
| [superset-frontend/src/SqlLab/types.ts](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi90eXBlcy50cw==) | `57.14% <ø> (ø)` | |
| [...et-frontend/src/components/TableSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGFibGVTZWxlY3Rvci9pbmRleC50c3g=) | `78.37% <ø> (-0.85%)` | :arrow_down: |
| [...ontrols/src/operators/utils/extractExtraMetrics.ts](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL29wZXJhdG9ycy91dGlscy9leHRyYWN0RXh0cmFNZXRyaWNzLnRz) | `100.00% <100.00%> (ø)` | |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `64.32% <100.00%> (ø)` | |
| [...d/src/SqlLab/components/AceEditorWrapper/index.tsx](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIvaW5kZXgudHN4) | `63.09% <100.00%> (+1.64%)` | :arrow_up: |
| [...c/SqlLab/components/RunQueryActionButton/index.tsx](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uL2luZGV4LnRzeA==) | `78.78% <100.00%> (ø)` | |
| ... and [3 more](https://codecov.io/gh/apache/superset/pull/23488?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] justinpark commented on pull request #23488: refactor(sqllab): Remove tableOptions from redux state
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23488:
URL: https://github.com/apache/superset/pull/23488#issuecomment-1495098486
> Hi justinpark! What are plans long term for redux here? Are you looking to remove it completely and replace it with react-query or just for select places? I know for a while now we've had state in both redux and in hooks, and haven't ever come to a decision on where frontend state should reside.
@eschutho As I state in #21240, the long term plan is replacing existing api related state (queries, tables, validationResult and functionNames in queryEditors) in redux store to a hook based state (using react-query was the first proposal and finally it will migrate to redux toolkit **query** (which is a similar solution of react-query) once #23460 landed.). Redux will be remained the editor's state(queryEditors, tabHistory, alerts, unsavedQueryEditor) that is only made by the user inputs.
The benefit of this redux cleanup work is
1. Can reduce many similar action/reducer logic for fetching state (as shown in this PR) and easier way to access the date by provided helpers
2. Implicitly avoid the duplicate api request (i.e. shown examples in #21240)
3. consolidate/manage the result data in a single place
Please let me know if you have any concerns with this redux cleanup 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] justinpark commented on pull request #23488: refactor(sqllab): Remove tableOptions from redux state
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23488:
URL: https://github.com/apache/superset/pull/23488#issuecomment-1517033932
> justinpark Maybe this tool can help us generate the API given that we use OpenAPI.
I will take a look compatibility with fab API
--
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 pull request #23488: refactor(sqllab): Remove tableOptions from redux state
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23488:
URL: https://github.com/apache/superset/pull/23488#issuecomment-1503435426
> RTK Query has [a (currently experimental) code-gen tool](https://github.com/reduxjs/redux-toolkit/tree/master/packages/rtk-query-codegen-openapi) that will take an OpenAPI spec or GraphQL schema and give you a typed API client, as well as provide methods for enhancing the generated client after the fact.
@justinpark Maybe this tool can help us generate the API given that we use OpenAPI.
--
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 pull request #23488: refactor(sqllab): Remove tableOptions from redux state
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23488:
URL: https://github.com/apache/superset/pull/23488#issuecomment-1503408108
@justinpark @eschutho I would like to add my 2 cents about the topic. The motivation for using react-query/RTK Query is the following (copied from [RTK website](https://redux-toolkit.js.org/rtk-query/overview)):
> Over the last couple years, the React community has come to realize that "data fetching and caching" is really a different set of concerns than "state management". While you can use a state management library like Redux to cache data, the use cases are different enough that it's worth using tools that are purpose-built for the data fetching use case.
Originally @justinpark was planning to use [react-query](https://tanstack.com/query/v3/) to manage data fetching and caching. Given that RTK has a tool in the same domain called RTK Query, I advised to [use that instead ](https://github.com/apache/superset/pull/23257#issuecomment-1473898703)to preserve library compatibility.
> Hi @justinpark! What are plans long term for redux here? Are you looking to remove it completely and replace it with react-query or just for select places? I know for a while now we've had state in both redux and in hooks, and haven't ever come to a decision on where frontend state should reside.
RTK Query is built on top of Redux as you can see in another quote from the same RTK website:
> The data fetching and caching logic is built on top of Redux Toolkit's createSlice and createAsyncThunk APIs
What RTK Query does is to automatically generate the actions, reducers, slices, and hooks to manage data fetching and caching. So it's a complement to Redux, not a replacement.
@justinpark volunteered to convert existing `react-query` hooks to RTK Query hooks and remove `react-query` dependency.
@justinpark I would advise to complete some steps before further modifying existing Redux state:
- Merge existing PR to [introduce Redux Toolkit](https://github.com/apache/superset/pull/23460)
- Go back to previous PRs that used `react-query` and replace them with RTK Query
- Remove `react-query` dependency
- Before removing any more state from the Redux store, I would model the endpoints based on the API itself (not its current use), to correctly define the parameters and return types, which would give us one of the best benefits of using RTK Query and Typescript: API type safety.
Lastly, thank you so much @justinpark for driving this effort. It's a really nice improvement, long overdue, that will really simplify our use of Redux and introduce many quality checks during development.
--
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] eschutho commented on pull request #23488: refactor(sqllab): Remove tableOptions from redux state
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #23488:
URL: https://github.com/apache/superset/pull/23488#issuecomment-1495006729
Hi @justinpark! What are plans long term for redux here? Are you looking to remove it completely and replace it with react-query or just for select places? I know for a while now we've had state in both redux and in hooks, and haven't ever come to a decision on where frontend state should reside.
--
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] justinpark merged pull request #23488: refactor(sqllab): Remove tableOptions from redux state
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark merged PR #23488:
URL: https://github.com/apache/superset/pull/23488
--
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] justinpark commented on pull request #23488: refactor(sqllab): Remove tableOptions from redux state
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #23488:
URL: https://github.com/apache/superset/pull/23488#issuecomment-1485958481
@michael-s-molina could you help the code review?
--
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