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: Export Chart to .xml format #14628

Closed

Conversation

exemplary-citizen
Copy link
Contributor

SUMMARY

This PR allows you to export charts to an xml file in the same manner we currently are able to with json and csv.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TEST PLAN

Added tests for the chart api in a similar flavor to the existing csv export tests.

ADDITIONAL INFORMATION

Fixes the XML part of #14415

  • Has associated issue: dashboard export in xls and xml format #14415
  • 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

cc @srinify @junlincc

@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #14628 (e67bb01) into master (74473e2) will increase coverage by 0.25%.
The diff coverage is 80.76%.

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

@@            Coverage Diff             @@
##           master   #14628      +/-   ##
==========================================
+ Coverage   77.20%   77.46%   +0.25%     
==========================================
  Files         958      958              
  Lines       48492    48514      +22     
  Branches     5691     6061     +370     
==========================================
+ Hits        37437    37579     +142     
+ Misses      10855    10730     -125     
- Partials      200      205       +5     
Flag Coverage Δ
hive 80.94% <77.77%> (?)
javascript 72.52% <87.50%> (+<0.01%) ⬆️
mysql 81.21% <77.77%> (+<0.01%) ⬆️
postgres 81.23% <77.77%> (+<0.01%) ⬆️
presto 80.92% <77.77%> (?)
python 81.75% <77.77%> (+0.47%) ⬆️
sqlite 80.85% <77.77%> (?)

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

Impacted Files Coverage Δ
superset/views/core.py 75.34% <33.33%> (-0.14%) ⬇️
...uperset-frontend/src/explore/exploreUtils/index.js 66.25% <80.00%> (+<0.01%) ⬆️
...nd/src/explore/components/ExploreActionButtons.tsx 74.00% <100.00%> (+1.65%) ⬆️
superset/charts/api.py 82.30% <100.00%> (+0.37%) ⬆️
superset/utils/core.py 89.01% <100.00%> (+0.13%) ⬆️
superset/views/base.py 76.44% <100.00%> (+0.25%) ⬆️
...end/src/SqlLab/components/RunQueryActionButton.tsx 64.28% <0.00%> (ø)
...d/src/filters/components/GroupBy/transformProps.ts 33.33% <0.00%> (ø)
...src/filters/components/TimeGrain/transformProps.ts 33.33% <0.00%> (ø)
...rc/filters/components/TimeColumn/transformProps.ts 33.33% <0.00%> (ø)
... and 12 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 c55418d...48a91f3. Read the comment docs.

@michael-s-molina
Copy link
Member

@exemplary-citizen Thanks for this PR. Can you resolve the conflicts?

@villebro Can you enable CI?

@exemplary-citizen
Copy link
Contributor Author

@villebro Can you enable CI when you get a chance?

@exemplary-citizen
Copy link
Contributor Author

I don't think the cypress failure is related to the changes in this PR. The bubble test that is failing does seem to have a history of instability #11630. Other than that, the rest of CI should come back green.

@villebro
Copy link
Member

Is there some specific use case for adding XML support? Since the data that's exported is mostly tabular, it feels like CSV and JSON should be adequate for most use cases. The reason I'm hesitant to add it is adding clutter to the already crowded chart header..

@exemplary-citizen
Copy link
Contributor Author

Yeah XML is definitely a very popular format especially for data exchange and interoperability. There are also a bunch of tools that provide functionality for migrating XML documents to tables or datasets including:

  • R's popular XML for simple, shallow structures
  • SAS maintains the libname xml engine to read and write XML documents to/from SAS datasets
  • Excel maintains the Workbooks.OpenXML method
  • MySQL maintains the LOAD XML to parse XML data to/from database tables
  • MS Access' object library (not database engine) maintains the Application.ImportXML and Application.ExportXML to import XML documents to/from database tables.

To alleviate your concern of adding clutter we could move all three buttons to an export dropdown like so:
image

@villebro
Copy link
Member

Going through old PRs and noticed this one. To continue this discussion, I like the idea of moving the export buttons to a more general export dropdown. However, to reiterate my original concern, I still feel adding XML export not really adding any new functionality, as it's really just exporting tabular data but in XML format (=same as the JSON export but in a nowadays less popular format). In addition, it will add maintenance burden to Superset developers, and is IMO a distraction from the core mission of Superset. If someone really wants to convert the data to XML, there's probably great tools for doing that from CSV/JSON formats.

But if others feel otherwise I'm happy to resume this discussion if this is a highly requested feature.

@exemplary-citizen
Copy link
Contributor Author

@villebro totally understand your concerns with regards to maintenance burden and am happy to proceed in whichever direction you think is best. I do think you should address the folks in #14415 that issue regularly gets bumped.

Let me know if you think we should close this PR.

@villebro
Copy link
Member

villebro commented Apr 6, 2022

@exemplary-citizen with the recent redesign of chart actions in #19446, this feature wouldn't add any more clutter to the UI, which is a good thing. And since this is appears to be something very many people are eagerly awaiting, I don't want to be the person who blocks this feature. Also, given that many will be using it, I assume there will be a shared interest to maintain it, too. So feel free to rebase this, and I'll be happy to review and get this merged 🙂 👍

@rusackas
Copy link
Member

rusackas commented Mar 9, 2023

Hi @exemplary-citizen - echoing @villebro sentiment here that there would be a lot of interest here if this could be rebased and brought back up to speed. Let us know if you have any intent to do so (or not). Thanks!

@justinpark
Copy link
Member

@exemplary-citizen could you rebase the branch and re-push the commit if you still work on this?

@justinpark justinpark closed this Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants