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(dashboard): Let users download full CSV of a table #15046

Merged
merged 12 commits into from
Jun 14, 2021

Conversation

m-ajay
Copy link
Contributor

@m-ajay m-ajay commented Jun 8, 2021

SUMMARY

Lets user download the full CSV of a table.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

export full csv option

TESTING INSTRUCTIONS

Manual and CI

ADDITIONAL INFORMATION

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

@nytai
Copy link
Member

nytai commented Jun 8, 2021

Exporting a full CSV could cause the server to run out of memory and/or compute, how should this be handled? I think and ideal solution would be to stream the results down to the client, but that would be a significantly larger effort. Perhaps adding a config key to optionally enable this feature might be enough to mitigate this concern.

cc @betodealmeida

@junlincc junlincc added the need:design-review Requires input/approval from a Designer label Jun 8, 2021
@junlincc
Copy link
Member

junlincc commented Jun 8, 2021

thanks for the contribution! @rusackas this dropdown list is getting a bit long, we may need to restructure it a bit.

@etr2460
Copy link
Member

etr2460 commented Jun 8, 2021

Exporting a full CSV could cause the server to run out of memory and/or compute, how should this be handled? I think and ideal solution would be to stream the results down to the client, but that would be a significantly larger effort. Perhaps adding a config key to optionally enable this feature might be enough to mitigate this concern.

100% agree with the perf question, in this case i think a feature flag/config option would resolve this. Streaming results seems especially difficult to do with our current infra (maybe easier once async everywhere is ready for primetime). For context for our use case, we're ok with the performance issues and potential OOMs that this might introduce since everything is in k8s and autoscalable anyway.

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #15046 (7482744) into master (cc2b4fe) will decrease coverage by 0.27%.
The diff coverage is 65.49%.

❗ Current head 7482744 differs from pull request most recent head 513274a. Consider uploading reports for the commit 513274a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15046      +/-   ##
==========================================
- Coverage   77.54%   77.26%   -0.28%     
==========================================
  Files         967      969       +2     
  Lines       49752    49918     +166     
  Branches     6351     6393      +42     
==========================================
- Hits        38578    38569       -9     
- Misses      10972    11146     +174     
- Partials      202      203       +1     
Flag Coverage Δ
hive ?
mysql 81.71% <81.48%> (-0.01%) ⬇️
postgres 81.73% <81.48%> (-0.01%) ⬇️
python 81.82% <81.48%> (-0.30%) ⬇️
sqlite 81.35% <81.48%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/dashboard/actions/hydrate.js 1.72% <ø> (ø)
...ard/components/FiltersBadge/DetailsPanel/index.tsx 86.20% <ø> (ø)
.../FilterBar/CascadeFilters/CascadePopover/index.tsx 61.76% <ø> (ø)
...Filters/FilterBar/FilterControls/FilterControl.tsx 100.00% <ø> (ø)
...perset-frontend/src/dashboard/containers/Chart.jsx 100.00% <ø> (ø)
...nd/src/dashboard/containers/DashboardComponent.jsx 92.30% <ø> (ø)
...-frontend/src/dashboard/reducers/dashboardState.js 73.33% <0.00%> (ø)
...et-frontend/src/filters/components/Select/types.ts 100.00% <ø> (ø)
superset-frontend/src/views/CRUD/utils.tsx 63.86% <ø> (-1.26%) ⬇️
superset/config.py 91.15% <ø> (ø)
... and 54 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 cc2b4fe...513274a. Read the comment docs.

@@ -87,7 +87,8 @@ const SHOULD_UPDATE_ON_PROP_CHANGES = Object.keys(propTypes).filter(
);
const OVERFLOWABLE_VIZ_TYPES = new Set(['filter_box']);
const DEFAULT_HEADER_HEIGHT = 22;

// superset backend supports upto a million rows
const MAXIMUM_NUMBER_OF_ROWS_IN_CSV = 1000000;

Choose a reason for hiding this comment

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

I have a little concern about too many configs in different places. Is possible to read this parameter from backend and carry it to frontend as part of bootstrap data. check this place:

FRONTEND_CONF_KEYS = (

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM

@graceguo-supercat graceguo-supercat merged commit 045fa1b into apache:master Jun 14, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* - Convert SliceHeader to TSX in progress
- Add menu option to download full CSV. Probably will change it

* Add Download Full CSV feature, and tests

* Added more tests, more TS fixes

* Added feature flag

* Update @superset-ui package versions

* Update @superset-ui packages versions

* use backend config instead of hardcoding number of rows

* Update tests

* front end test fix

* Lint fixes and test fixes
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* - Convert SliceHeader to TSX in progress
- Add menu option to download full CSV. Probably will change it

* Add Download Full CSV feature, and tests

* Added more tests, more TS fixes

* Added feature flag

* Update @superset-ui package versions

* Update @superset-ui packages versions

* use backend config instead of hardcoding number of rows

* Update tests

* front end test fix

* Lint fixes and test fixes
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* - Convert SliceHeader to TSX in progress
- Add menu option to download full CSV. Probably will change it

* Add Download Full CSV feature, and tests

* Added more tests, more TS fixes

* Added feature flag

* Update @superset-ui package versions

* Update @superset-ui packages versions

* use backend config instead of hardcoding number of rows

* Update tests

* front end test fix

* Lint fixes and test fixes
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 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 need:design-review Requires input/approval from a Designer size/L 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants