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.

Sign up for GitHub

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

Jump to bottom

fix: chart import error with virtual dataset #19782

Merged
merged 4 commits into from
Apr 29, 2022
Merged

fix: chart import error with virtual dataset #19782

merged 4 commits into from
Apr 29, 2022

Conversation

codemaster08240328
Copy link
Contributor

SUMMARY

This issue was not related to importing/exporting over api.

This was happened through UI as well, and the reason was that we regards virtual dataset as sql lab view.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
#13237

AFTER:
[DEV] Superset - 19 April 2022 - Watch Video

TESTING INSTRUCTIONS

Check: #13237

ADDITIONAL INFORMATION

@codecov
Copy link

codecov bot commented Apr 19, 2022 •
edited
Loading

Codecov Report

Merging #19782 (02e1ec8) into master (ae38411) will decrease coverage by 12.58%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #19782       +/-   ##
===========================================
- Coverage   66.55%   53.97%   -12.59%     
===========================================
  Files        1692     1692               
  Lines       64802    64832       +30     
  Branches     6657     6657               
===========================================
- Hits        43129    34990     -8139     
- Misses      19973    28142     +8169     
  Partials     1700     1700
Flag Coverage Δ
hive 52.91% <ø> (+0.04%) ⬆️
mysql ?
postgres ?
presto 52.76% <ø> (+0.04%) ⬆️
python 56.77% <ø> (-25.60%) ⬇️
sqlite ?
unit 47.99% <ø> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
superset/charts/commands/importers/v1/__init__.py 44.89% <ø> (-55.11%) ⬇️
superset/commands/importers/v1/examples.py 0.00% <ø> (ø)
superset/utils/dashboard_import_export.py 0.00% <0.00%> (-100.00%) ⬇️
superset/key_value/commands/upsert.py 0.00% <0.00%> (-89.59%) ⬇️
superset/key_value/commands/update.py 0.00% <0.00%> (-89.37%) ⬇️
superset/key_value/commands/delete.py 0.00% <0.00%> (-85.30%) ⬇️
superset/key_value/commands/delete_expired.py 0.00% <0.00%> (-80.77%) ⬇️
superset/dashboards/commands/importers/v0.py 14.79% <0.00%> (-75.15%) ⬇️
superset/datasets/commands/importers/v0.py 24.82% <0.00%> (-68.80%) ⬇️
superset/databases/commands/test_connection.py 31.42% <0.00%> (-68.58%) ⬇️
... and 271 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 ae38411...02e1ec8. Read the comment docs.

@rusackas
Copy link
Member

Not sure why that logic was there in the first place.... @diegomedina248 or @zhaoyongjie do you see any risks here we're not aware of?

zhaoyongjie
zhaoyongjie reviewed Apr 20, 2022
View reviewed changes
superset/charts/commands/importers/v1/__init__.py
"datasource_type": "view"
if dataset.is_sqllab_view
else "table",
"datasource_type": "table",
Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar with this part, but the datasource_type = view seems the correct value here. Here is the schema defination for updating chart.

@betodealmeida could you take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but virtual dataset should have view as a datasource_type?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not familiar with this part, so I created a chart from a virtual dataset and it has datasource_type set to table. Looking at the source code it seems that "view" here means a database VIEW, not a virtual dataset, and is only used by the DB engine spec to retrieve the list of views from a given database:

superset/superset/db_engine_specs/base.py

Lines 873 to 879 in 4a835a4

elif datasource_type == "view":
all_datasources += database.get_all_view_names_in_schema(
schema=schema,
force=True,
cache=database.table_cache_enabled,
cache_timeout=database.table_cache_timeout,
)

So I think this should be table, and we also need to fix this line:

superset/superset/commands/importers/v1/examples.py

Line 141 in 4a835a4

"datasource_type": "view" if dataset.is_sqllab_view else "table",

betodealmeida
betodealmeida approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

This looks consistent with the current logic, even if it doesn't make sense to me. Can you also fix commands/importers/v1/examples.py in this PR? Thanks!

superset/charts/commands/importers/v1/__init__.py
"datasource_type": "view"
if dataset.is_sqllab_view
else "table",
"datasource_type": "table",
Copy link
Member

Choose a reason for hiding this comment

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

I'm also not familiar with this part, so I created a chart from a virtual dataset and it has datasource_type set to table. Looking at the source code it seems that "view" here means a database VIEW, not a virtual dataset, and is only used by the DB engine spec to retrieve the list of views from a given database:

superset/superset/db_engine_specs/base.py

Lines 873 to 879 in 4a835a4

elif datasource_type == "view":
all_datasources += database.get_all_view_names_in_schema(
schema=schema,
force=True,
cache=database.table_cache_enabled,
cache_timeout=database.table_cache_timeout,
)

So I think this should be table, and we also need to fix this line:

superset/superset/commands/importers/v1/examples.py

Line 141 in 4a835a4

"datasource_type": "view" if dataset.is_sqllab_view else "table",

@rusackas rusackas merged commit 36d45d9 into apache:master Apr 29, 2022
hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
@codemaster08240328
fix: chart import error with virtual dataset (apache#19782)
8ecaf69 
* fix: chart import error with virtual dataset

* remove unnecessary comment

* resolve comment
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@codemaster08240328 @philipher29
fix: chart import error with virtual dataset (apache#19782)
fea39aa 
* fix: chart import error with virtual dataset

* remove unnecessary comment

* resolve comment
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diegomedina248 diegomedina248 diegomedina248 approved these changes

@betodealmeida betodealmeida betodealmeida approved these changes

@zhaoyongjie zhaoyongjie Awaiting requested review from zhaoyongjie

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels Preset-Patch size/XS 🚢 2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@codemaster08240328 @rusackas @betodealmeida @zhaoyongjie @diegomedina248 @mistercrunch