-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
feat(SIP-95): new endpoint for table metadata #28122
Conversation
@@ -403,7 +413,7 @@ def get_sqla_engine( | |||
# if ssh_tunnel is available build engine with information | |||
engine_context = ssh_manager_factory.instance.create_tunnel( | |||
ssh_tunnel=ssh_tunnel, | |||
sqlalchemy_database_uri=self.sqlalchemy_uri_decrypted, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See line 396 above.
# The `get_sqla_engine_with_context` was renamed to `get_sqla_engine`, but we kept a | ||
# reference to the old method to prevent breaking third-party applications. | ||
# TODO (betodealmeida): Remove in 5.0 | ||
get_sqla_engine_with_context = get_sqla_engine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this because the signature for get_sqla_engine
changed to accept a catalog.
# The ``adjust_database_uri`` method was renamed to ``adjust_engine_params`` and | ||
# had its signature changed in order to support more DB engine specs. Since DB | ||
# engine specs can be released as 3rd party modules we want to make sure the old | ||
# method is still supported so we don't introduce a breaking change. | ||
if hasattr(self.db_engine_spec, "adjust_database_uri"): | ||
sqlalchemy_url = self.db_engine_spec.adjust_database_uri( | ||
sqlalchemy_url, | ||
schema, | ||
) | ||
logger.warning( | ||
"DB engine spec %s implements the method `adjust_database_uri`, which is " | ||
"deprecated and will be removed in version 3.0. Please update it to " | ||
"implement `adjust_engine_params` instead.", | ||
self.db_engine_spec, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're on 4.0 now.
587f4e7
to
636ea42
Compare
636ea42
to
02dcd9d
Compare
02dcd9d
to
78caf97
Compare
092fd9d
to
cc6088d
Compare
@property | ||
def sqla_metadata(self) -> None: | ||
# pylint: disable=no-member | ||
with self.get_sqla_engine() as engine: | ||
meta = MetaData(bind=engine) | ||
meta.reflect() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used anywhere, AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/Users/beto/Projects/superset/superset (new-table-metadata-api)$ git grep '\.sqla_metadata'
/Users/beto/Projects/superset/superset (new-table-metadata-api)$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, after I removed this property there are no occurrences of sqla_metadata
in the codebase:
/Users/beto/Projects/superset (new-table-metadata-api)$ git grep sqla_metadata
/Users/beto/Projects/superset (new-table-metadata-api)$
171569f
to
1f73f2f
Compare
def get_table_metadata( | ||
database: Any, table_name: str, schema_name: Optional[str] | ||
) -> dict[str, Any]: | ||
def get_table_metadata(database: Any, table: Table) -> TableMetadataResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the database type be a Database model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good catch!
1f73f2f
to
a41acfb
Compare
a41acfb
to
9cf8c0c
Compare
9cf8c0c
to
3cf1ac6
Compare
SUMMARY
This PR adds a new endpoint to get metadata from a table, optionally passing a schema and a catalog.
In order to support the new endpoint, this PR has two refactors. Despite touching many files, they are simple:
table_name
and aschema
were updated to received aTable
instance instead, since it carries the catalog:superset/superset/sql_parse.py
Lines 242 to 250 in 69a7bfc
schema
now also take acatalog
.Note that currently the catalog being passed around is always
None
. It gets passed all the way to theadjust_engine_params
method in the DB engine spec, and DB engine specs are responsible for modifying the SQLAlchemy URI to connect to the catalog.In the next PR I'll start implementing support for catalogs in Postgres, and will modify the UI to allow choosing a catalog when running SQL or creating a dataset.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
I added unit tests for the new endpoint, and updated existing tests.
ADDITIONAL INFORMATION