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.

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: Reuse Dashboard redux data in Explore #20668

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

kgabryje
Copy link
Member

SUMMARY

Currently when user open Explore, we always send a request to /v1/explore endpoint, which assembles Explore initial data - slice, form_data and datasource.
When user goes to Explore from Dashboard (by clicking "Edit chart" or char title), most of the data needed to assemble Explore should already be available in Redux store.
This PR implements using data from Dashboard Redux store to initialize Explore without calling /v1/explore endpoint.

If user opens Explore from Dashboard, check if slice, formData, datasourceId and datasourceType are available. If they are, fetch datasource metadata and hydrate Explore using slice, formData and datasource.
If they are not available, call /v1/explore and hydrate Explore.

We can't reuse datasource from Dashboard. Datasources metadata on Dashboard is trimmed so that datasource contains only columns and metrics used by charts on that dashboard. In Explore we need to be able to access all columns and metrics, which means we must fetch those from API.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Go to dashboard
  2. Click "Edit chart"
  3. Verify that Explore has opened correctly and that we did not call /v1/explore/ endpoint
  4. Enter Explore from any other entry point (for example Charts list or Welcome page)
  5. Verify that we do call /v1/explore

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #20668 (f0bba78) into master (8d4994a) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

❗ Current head f0bba78 differs from pull request most recent head 8121f57. Consider uploading reports for the commit 8121f57 to get more accurate results

@@            Coverage Diff             @@
##           master   #20668      +/-   ##
==========================================
- Coverage   66.85%   66.84%   -0.02%     
==========================================
  Files        1753     1753              
  Lines       65825    65846      +21     
  Branches     7006     7010       +4     
==========================================
+ Hits        44010    44015       +5     
- Misses      20030    20045      +15     
- Partials     1785     1786       +1     
Flag Coverage Δ
javascript 51.93% <0.00%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
...d/packages/superset-ui-core/src/utils/isDefined.ts 100.00% <ø> (ø)
superset-frontend/src/dashboard/actions/hydrate.js 2.04% <0.00%> (-0.03%) ⬇️
.../src/dashboard/components/gridComponents/Chart.jsx 56.60% <0.00%> (ø)
superset-frontend/src/explore/ExplorePage.tsx 0.00% <0.00%> (ø)
...set-frontend/src/explore/actions/hydrateExplore.ts 56.00% <0.00%> (-4.00%) ⬇️
superset/charts/schemas.py 99.36% <100.00%> (+0.01%) ⬆️

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 8d4994a...8121f57. Read the comment docs.

@zhaoyongjie
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@zhaoyongjie Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

@zhaoyongjie Ephemeral environment creation failed. Please check the Actions logs for details.

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

leave a minor suggestion.

return fetchExploreData();
}
return SupersetClient.get({
endpoint: `/datasource/get/${datasourceType}/${datasourceId}/`,
Copy link
Member

Choose a reason for hiding this comment

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

there is a new endpoint for the dataset GET method is /api/v1/dataset/<datasource PK(a.k.a datasource id without type>. I added it at here

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 actually used GET /v1/dataset endpoint at first, but then changed it to GET /datasource/get/. As far as I know, Explore now supports (or will soon support?) other sources than datasets, such as SQL queries. Tagging @hughhhh for transparency.
Also, the /v1/explore/ endpoint uses DatasourceDAO to get datasource and the response from /v1/dataset endpoint is missing some fields compared to datasource from /v1/explore, which caused some components in Explore to crash

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 propose we stick to the legacy /datasource/get endpoint for now and create a new /v1/datasource API in the future and refactor

Copy link
Member

Choose a reason for hiding this comment

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

@kgabryje Thanks for the explanation, we will optimize it in the future.

@kgabryje kgabryje force-pushed the feat/reuse-dashboard-store branch from 6bb360e to 8121f57 Compare July 12, 2022 12:50
@kgabryje
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@kgabryje Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

@kgabryje Ephemeral environment creation failed. Please check the Actions logs for details.

@zhaoyongjie zhaoyongjie self-requested a review July 12, 2022 14:49
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@kgabryje kgabryje merged commit ff5b4bc into apache:master Jul 12, 2022
kgabryje added a commit to kgabryje/incubator-superset that referenced this pull request Jul 12, 2022
@kgabryje
Copy link
Member Author

This PR will be re-opened when linking from Dashboard to Explore is refactored (coming in a few days)

michael-s-molina pushed a commit that referenced this pull request Jul 12, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants