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: catalog upgrade/downgrade #29780

Merged
merged 1 commit into from
Jul 30, 2024
Merged

fix: catalog upgrade/downgrade #29780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion superset/db_engine_specs/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@

logger = logging.getLogger()

# When connecting to a database it's hard to catch specific exceptions, since we support
# more than 50 different database drivers. Usually the try/except block will catch the
# generic `Exception` class, which requires a pylint disablee comment. To make it clear
# that we know this is a necessary evil we create an alias, and catch it instead.
GenericDBException = Exception
Copy link
Member

Choose a reason for hiding this comment

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

why not ->

class GenericDBException(Exception):
    pass

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, I understand now reading the code

Copy link
Member Author

@betodealmeida betodealmeida Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In that case the exceptions from all the DB API drivers would not be derived from GenericDBException, so we wouldn't be able to catch them.

The only exception that is a parent to all the exceptions from the multitude of DB API drivers we support is Exception itself (I hope!), which is why we have to write:

try:
    # do something with some dynamically loaded DB API driver
except Exception:  # pylint: disable=broad-except
    pass

It's inevitable to have a broad except, because the drivers are loaded dynamically. My alias is just a way of saying "I know this is bad, but what can I do?", but also "I'm expecting an exception here that could be anything".



def convert_inspector_columns(cols: list[SQLAColumnType]) -> list[ResultSetColumnType]:
result_set_columns: list[ResultSetColumnType] = []
Expand Down Expand Up @@ -406,7 +412,8 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
#
# When this is changed to true in a DB engine spec it MUST support the
# `get_default_catalog` and `get_catalog_names` methods. In addition, you MUST write
# a database migration updating any existing schema permissions.
# a database migration updating any existing schema permissions using the helper
# `upgrade_catalog_perms`.
supports_catalog = False

# Can the catalog be changed on a per-query basis?
Expand Down
4 changes: 2 additions & 2 deletions superset/db_engine_specs/databricks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,8 @@ def get_default_catalog(
cls,
database: Database,
) -> str | None:
with database.get_inspector() as inspector:
return inspector.bind.execute("SELECT current_catalog()").scalar()
with database.get_sqla_engine() as engine:
return engine.execute("SELECT current_catalog()").scalar()

@classmethod
def get_prequeries(
Expand Down
Loading
Loading