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

chore: avoid virtual datasets whenever possible #295

Merged
merged 2 commits into from
May 29, 2024

Conversation

betodealmeida
Copy link
Member

With SIP-95 implemented we don't need virtual datasets anymore, yay!

Note: ideally this still requires testing with a real project, and also for the SIP-95 branch to be deployed at Preset.

@betodealmeida betodealmeida requested a review from Vitor-Avila May 10, 2024 14:21
@betodealmeida betodealmeida force-pushed the bye-virtual-datasets branch 3 times, most recently from e7b70c5 to 6d8d25b Compare May 16, 2024 13:24
@betodealmeida betodealmeida changed the title chore: remove virtual datasets chore: avoid virtual datasets whenever possible May 16, 2024
@betodealmeida betodealmeida marked this pull request as ready for review May 16, 2024 13:24
@betodealmeida betodealmeida force-pushed the bye-virtual-datasets branch from 6d8d25b to c75ab5e Compare May 16, 2024 13:25
except SupersetError as ex:
if not no_catalog_support(ex):
raise ex

Copy link
Contributor

@Vitor-Avila Vitor-Avila May 28, 2024

Choose a reason for hiding this comment

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

You could just delete the catalog key here (del kwargs["catalog"])

except SupersetError as ex:
if not no_catalog_support(ex):
raise ex

url = make_url(database["sqlalchemy_uri"])
if model_in_database(model, url):
Copy link
Contributor

@Vitor-Avila Vitor-Avila May 28, 2024

Choose a reason for hiding this comment

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

then avoid this if/else and have a single if not model_in_database(model, url) with kwargs["sql"] = f"SELECT * FROM {source}" (and the remaining logic that's specific for virtual datasets)

Copy link
Contributor

@Vitor-Avila Vitor-Avila left a comment

Choose a reason for hiding this comment

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

left a non-blocking suggestion

@betodealmeida betodealmeida merged commit ed4b067 into main May 29, 2024
5 checks passed
@betodealmeida betodealmeida deleted the bye-virtual-datasets branch May 29, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants