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(SQL Lab): Make SQL Lab explore use the default viz from the config file #20056

Merged
merged 9 commits into from
Jun 22, 2022

Conversation

cccs-Dustin
Copy link
Contributor

@cccs-Dustin cccs-Dustin commented May 13, 2022

SUMMARY

In Superset's config file, there is a config option called DEFAULT_VIZ_TYPE which you can use to define which viz you would like to use in the chart explorer by default. This is a very useful config option, especially when you have a custom viz you would like to use instead of the Superset Table viz. However, when you are in SQL Lab and you want to create and explore a dataset, there is currently no way to to change the default viz which will be used.

This PR allows for the DEFAULT_VIZ_TYPE config option to also be used by SQL Lab when you want to explore a dataset. This allows for consistency between the chart explorer, and exploring through SQL Lab.

The changes include modifying the front-end index file so that instead of hard coding the table viz, it drops the viz_type variable entirely. This way, we can set the now undefined viz_type to the DEFAULT_VIZ_TYPE within the back-end. Another change that was made was passing the selectedColumns object to the back-end, and only populating the all_columns variable if the DEFAULT_VIZ_TYPE is a table.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

  1. Modify the config file so that the DEFAULT_VIZ_TYPE is a different viz from the standard Table (e.g., pivot_table).
  2. Within Superset, select "SQL Lab" from the top menu.
  3. Select "SQL Editor" from the drop down menu which was opened during step 2.
  4. On the left side of the page, select a database and schema that you would like to run the test query on.
  5. For testing purposes, add some basic SQL to run against a table (e.g., SELECT * FROM table_1;).
  6. Select the Run button which will run the query.
  7. Once the results are displayed under the "Results" tab, select the Explore button to explore the result set in the data exploration view.
  8. Once the exploration view opens, you should see the viz type you defined in step 1 selected in the "Visualization Type" section.

ADDITIONAL INFORMATION

  • Has associated issue: Make SQL Lab explore use the default viz from the config file #20057
  • 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 May 13, 2022

Codecov Report

Merging #20056 (32395da) into master (a833674) will decrease coverage by 0.13%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master   #20056      +/-   ##
==========================================
- Coverage   66.74%   66.61%   -0.14%     
==========================================
  Files        1739     1732       -7     
  Lines       65130    64952     -178     
  Branches     6898     6858      -40     
==========================================
- Hits        43472    43268     -204     
- Misses      19908    19928      +20     
- Partials     1750     1756       +6     
Flag Coverage Δ
hive 53.72% <0.00%> (-0.02%) ⬇️
mysql ?
postgres 82.39% <80.00%> (-0.02%) ⬇️
presto 53.57% <0.00%> (-0.02%) ⬇️
python 82.78% <80.00%> (-0.06%) ⬇️
sqlite 82.18% <80.00%> (-0.01%) ⬇️
unit 50.56% <0.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...d/src/SqlLab/components/SaveDatasetModal/index.tsx 34.66% <ø> (ø)
superset-frontend/src/SqlLab/types.ts 57.14% <ø> (ø)
superset/config.py 91.41% <ø> (ø)
superset/views/core.py 76.91% <80.00%> (-0.44%) ⬇️
...t-frontend/src/dashboard/containers/SliceAdder.jsx 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/dashboard/actions/sliceEntities.js 15.38% <0.00%> (-38.47%) ⬇️
...-ui-chart-controls/src/utils/defineSavedMetrics.ts 66.66% <0.00%> (-33.34%) ⬇️
...mponents/DataTablesPane/components/SamplesPane.tsx 69.76% <0.00%> (-27.91%) ⬇️
...ilters/FiltersConfigModal/FilterTitleContainer.tsx 57.14% <0.00%> (-22.27%) ⬇️
...erset-ui-chart-controls/src/utils/columnChoices.ts 80.00% <0.00%> (-20.00%) ⬇️
... and 125 more

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 a833674...32395da. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

A few thoughts. Did you also notice EXPLORE_CHART_DEFAULT on line 62 which defines viz_type: 'table'?

@cccs-Dustin cccs-Dustin requested a review from villebro May 30, 2022 18:28
@zhaoyongjie
Copy link
Member

zhaoyongjie commented Jun 16, 2022

@cccs-Dustin Thanks for the contribution.

Many charts need to provide how to aggregate, in other words, you have to provide appropriate metric(s) or appropriate dimension. This is why datasets created from SQLLab are displayed in non-aggregated tables by default.

a specific example: a dataset without a time dimension cannot generate a timeseries chart.

@rusackas
Copy link
Member

@cccs-Dustin Thanks for the contribution.

Many charts need to provide how to aggregate, in other words, you have to provide appropriate metric(s) or appropriate dimension. This is why datasets created from SQLLab are displayed in non-aggregated tables by default.

a specific example: a dataset without a time dimension cannot generate a timeseries chart.

I think this is OK for their use case... they want to replace the table with a different non-aggregated chart. This should allow for that use case without affecting other users/installations of Superset.

If people want to set some other aggregated chart as their default, it'll be more difficult indeed, but that's not the intent of the PR here.

@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment spinning up at http://35.89.223.60:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Seems to work fine on the ephemeral, and LGTM!

@rusackas rusackas merged commit daded10 into apache:master Jun 22, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@cccs-Dustin cccs-Dustin deleted the feature/CLDN-1312 branch June 22, 2022 17:46
akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022
…ig file (apache#20056)

* Modified SQL Lab so it uses the default viz to explore a new dataset with

* Added a comment to the config file

* Remove trailing whitespace

* [CLDN-1312] Removed the 'viz_type' variable so that when it is undefinded in the back-end, it gets set to the default viz

* [CLDN-1312] Only sets the 'all_columns' variable if the default viz type is a 'table'

* Will only pop 'selectedColumns' out of 'form_data' if it is present

* [CLDN-1312] Updated files so that the default viz is now being used again in SQL Lab Explore
@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/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants