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(dbt): Instruct users of missing dependency when creating a Snowflake virtual dataset #290

Merged
merged 4 commits into from
May 2, 2024

Conversation

Vitor-Avila
Copy link
Contributor

dbt is compatible with warehouses that support creating multiple DBs in the same instance (such as Snowflake). Since the database is specified in the DB connection string in Superset, when a model is created in a different database, the CLI creates it as a virtual dataset (using select * from db_name.schema.table).

SQLAlchemy's create_engine() method might require the DB-specific driver to work. This PR adds proper instructions to users when facing this issue. It currently only covers snowflake, but covering more engines should be straightforward.

Comment on lines +214 to +221
if dialect == "snowflake":
raise CLIError(
(
"Missing required package. "
'Please run ``pip install "preset-cli[snowflake]"`` to install it.'
),
1,
) from exc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future this could be something like:

if dialect in {"snowflake", "redshift", "bigquery"}:
    raise CLIError(
        (
            "Missing required package. "
            f'Please run ``pip install "preset-cli[{dialect}]"`` to install it.'
        ),
        1,
    ) from exc

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.

Looks great! Left a few comments.

@@ -198,6 +201,31 @@ def build_snowflake_sqlalchemy_params(target: Dict[str, Any]) -> Dict[str, Any]:
return parameters


def create_sqlalchemy_engine(url: URL):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def create_sqlalchemy_engine(url: URL):
def create_sqlalchemy_engine(url: URL) -> Engine:

With:

from sqlalchemy.engine import Engine

Also, maybe we should rename this to create_engine_with_check or something more explicit? To make it clear how it's different from create_engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch! and great idea!

@Vitor-Avila Vitor-Avila merged commit 9785acb into main May 2, 2024
5 checks passed
@Vitor-Avila Vitor-Avila deleted the snwoflake-fix branch May 2, 2024 20:04
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.

None yet

2 participants