feat(SIP-95): new endpoint for table metadata #28122
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
github-actions
bot
added
risk:db-migration
betodealmeida
commented
Apr 18, 2024
| @@ -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.
See line 396 above.
betodealmeida
commented
Apr 18, 2024
Comment on lines
-427
to
-430
| # 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.
I removed this because the signature for get_sqla_engine changed to accept a catalog.
betodealmeida
commented
Apr 18, 2024
Comment on lines
-450
to
-464
| # 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, | ||
| ) |
betodealmeida
force-pushed
the
new-table-metadata-api
branch
from
587f4e7
to
636ea42
Compare
betodealmeida
force-pushed
the
new-table-metadata-api
branch
from
636ea42
to
02dcd9d
Compare
betodealmeida
requested review from
villebro,
geido,
eschutho,
rusackas,
nytai,
mistercrunch,
craig-rueda,
john-bodley,
kgabryje,
dpgaspar and
jinghua-qa
as code owners
github-actions
bot
added
doc
betodealmeida
changed the base branch from
new-extra-table-metadata-api
to
master
betodealmeida
force-pushed
the
new-table-metadata-api
branch
from
02dcd9d
to
78caf97
Compare
betodealmeida
force-pushed
the
new-table-metadata-api
branch
13 times, most recently
from
092fd9d
to
cc6088d
Compare
betodealmeida
commented
Apr 19, 2024
Comment on lines
-217
to
-223
| @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.
This is not used anywhere, AFAICT.
There was a problem hiding this comment.
/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.
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)$
betodealmeida
force-pushed
the
new-table-metadata-api
branch
4 times, most recently
from
171569f
to
1f73f2f
Compare
eschutho
reviewed
Apr 23, 2024
| 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.
should the database type be a Database model?
betodealmeida
force-pushed
the
new-table-metadata-api
branch
from
1f73f2f
to
a41acfb
Compare
eschutho
approved these changes
Apr 25, 2024
betodealmeida
force-pushed
the
new-table-metadata-api
branch
from
a41acfb
to
9cf8c0c
Compare
betodealmeida
force-pushed
the
new-table-metadata-api
branch
from
9cf8c0c
to
3cf1ac6
Compare
qleroy
pushed a commit
to qleroy/superset
that referenced
this pull request
Apr 28, 2024
jzhao62
pushed a commit
to jzhao62/superset
that referenced
this pull request
May 16, 2024
9 tasks
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
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_nameand aschemawere updated to received aTableinstance instead, since it carries the catalog:superset/superset/sql_parse.py
Lines 242 to 250 in 69a7bfc
schemanow also take acatalog.Note that currently the catalog being passed around is always
None. It gets passed all the way to theadjust_engine_paramsmethod 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