fix: catalog upgrade/downgrade #29780
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dosubot
bot
added
change:backend
betodealmeida
force-pushed
the
catalog-migration
branch
from
6868b9f
to
5437d0e
Compare
michael-s-molina
added
the
review:checkpoint
3 tasks
| # 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 |
There was a problem hiding this comment.
why not ->
class GenericDBException(Exception):
passThere was a problem hiding this comment.
nevermind, I understand now reading the code
There was a problem hiding this comment.
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
passIt'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".
mistercrunch
approved these changes
Jul 30, 2024
|
@supersetbot label 4.1 |
github-actions
bot
added
the
v4.1
sadpandajoe
pushed a commit
to preset-io/superset
that referenced
this pull request
Jul 30, 2024
(cherry picked from commit 525e837)
michael-s-molina
removed
the
review:checkpoint
sadpandajoe
pushed a commit
to preset-io/superset
that referenced
this pull request
Aug 1, 2024
(cherry picked from commit 525e837)
sadpandajoe
pushed a commit
that referenced
this pull request
Aug 13, 2024
(cherry picked from commit 525e837)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Many little improvements to the catalog migration helpers, especially on the downgrade.
The exception to (1) is that on
upgradewe need to rundatabase.get_default_catalog()in order to update models; doing this later would be messy and could lead to security issues. Theget_default_catalog()method usually doesn't need to connect to the analytical database, except for Databricks (where it's always the need) and BigQuery (when the project is not specified in the SQLAlchemy URI).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
Updated and improved tests.
ADDITIONAL INFORMATION