You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2024/04/16 18:35:36 UTC
[PR] feat(sip-95): new endpoint for extra table metadata [superset]
betodealmeida opened a new pull request, #28063:
URL: https://github.com/apache/superset/pull/28063
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
I started working on [SIP-95](https://github.com/apache/superset/issues/22862), which introduces catalogs to Superset. The introduction of a new hierarchical level, catalog → schema → table, means that some API endpoints need to be updated to support a new parameter.
One of these API endpoints is used to fetch extra metadata about a given table, and the route looks like this:
`/<int:pk>/table_extra/<path:table_name>/<schema_name>/`
There are a few problems with the current route:
- `table_name` is a `path`, because table names can contain slashes.
- `schema_name` is often passed as `null` or `undefined`, so we need special logic to convert it to `None` in the backend.
- `schema_name` should never contain slashes, otherwise the parsing will be wrong.
Because of this, instead of enhancing the existing endpoint to add support for catalogs, I decided to create a new one:
`/<int:pk>/table_metadata/extra/?table=t[&schema=s[&catalog=c]]`
In this new endpoint, `table`, `schema`, and `catalog` are all passed as query parameters. This allows slashes in all of them, and prevents polluting logs with confusing URLs such as `/api/v1/databses/1/table_extra/my_table/undefined/`. Only `table` is required, the other two are optional.
The API calls a new method `get_extra_table_metadata` in the corresponding DB engine spec. The new method takes a `Table` object, which contains a name and optional schema/catalog. The old method `extra_table_metadata` was kept to prevent breaking any 3rd-party DB engine specs, and the default behavior of `get_extra_table_metadata` is to call `extra_table_metadata` without a catalog).
Note that while the old endpoint is not removed in this PR, to prevent breaking changes, the frontend code was updated to use the new endpoint.
In the future, we should remove the old endpoint and also the `extra_table_metadata` method. @michael-s-molina, what do you think of this approach of introducing a new API? Should we mark it as deprecated now and remove it in the next release, or should we mark is as deprecated in 5.0 and remove it in 6.0?
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
I updated existing tests and added new unit tests.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #28063:
URL: https://github.com/apache/superset/pull/28063#issuecomment-2061298960
> Oh, I'm not against nesting resources in general. The problem here is that table names can contain slashes, and I think we should assume that schemas and catalogs can too.
> So something to think about when designing v2.
Yep! That's indeed a requirement we should consider when designing v2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #28063:
URL: https://github.com/apache/superset/pull/28063#discussion_r1569668859
##########
superset/db_engine_specs/presto.py:
##########
@@ -1224,14 +1225,20 @@ def expand_data( # pylint: disable=too-many-locals
return all_columns, data, expanded_columns
@classmethod
- def extra_table_metadata(
- cls, database: Database, table_name: str, schema_name: str | None
+ def get_extra_table_metadata(
+ cls,
+ database: Database,
+ table: Table,
) -> dict[str, Any]:
metadata = {}
- if indexes := database.get_indexes(table_name, schema_name):
+ if indexes := database.get_indexes(table.table, table.schema):
col_names, latest_parts = cls.latest_partition(
- table_name, schema_name, database, show_first=True, indexes=indexes
+ table.table,
Review Comment:
We should! I'm doing that in the next PR! 😊
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #28063:
URL: https://github.com/apache/superset/pull/28063#discussion_r1569677069
##########
superset/databases/schemas.py:
##########
@@ -1195,3 +1195,27 @@ class OAuth2ProviderResponseSchema(Schema):
class Meta: # pylint: disable=too-few-public-methods
# Ignore unknown fields that might be sent by the OAuth2 provider
unknown = EXCLUDE
+
+
+class QualifiedTableSchema(Schema):
+ """
+ Schema for a qualified table reference.
+
+ Catalog and schema can be ommited, to fallback to default values. Table name must be
+ present.
+ """
+
+ table = fields.String(
Review Comment:
Yeah, I think the current terminology is confusing, there's a lot of `table.table` in the codebase. Let me change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #28063:
URL: https://github.com/apache/superset/pull/28063#discussion_r1569669886
##########
superset/databases/api.py:
##########
@@ -812,13 +831,91 @@ def table_extra_metadata(
500:
$ref: '#/components/responses/500'
"""
- self.incr_stats("init", self.table_metadata.__name__)
+ self.incr_stats("init", self.table_extra_metadata_deprecated.__name__)
parsed_schema = parse_js_uri_path_item(schema_name, eval_undefined=True)
table_name = cast(str, parse_js_uri_path_item(table_name))
- payload = database.db_engine_spec.extra_table_metadata(
- database, table_name, parsed_schema
- )
+ table = Table(table_name, parsed_schema)
+ payload = database.db_engine_spec.get_extra_table_metadata(database, table)
+ return self.response(200, **payload)
+
+ @expose("/<int:pk>/table_metadata/extra/", methods=("GET",))
+ @protect()
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".table_extra_metadata",
+ log_to_statsd=False,
+ )
+ def table_extra_metadata(self, pk: int) -> FlaskResponse:
+ """
+ Get extra metadata for a given table.
+
+ Optionally, a schema and a catalog can be passed, if different from the default
+ ones.
+ ---
+ get:
+ summary: Get table extra metadata
+ description: >-
+ Extra metadata associated with the table (partitions, description, etc.)
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The database id
+ - in: query
+ schema:
+ type: string
+ name: table
+ required: true
+ description: Table name
+ - in: query
+ schema:
+ type: string
+ name: schema
+ description: >-
+ Optional table schema, if not passed default schema will be used
+ - in: query
+ schema:
+ type: string
+ name: catalog
+ description: >-
+ Optional table catalog, if not passed default catalog will be used
+ responses:
+ 200:
+ description: Table extra metadata information
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/TableExtraMetadataResponseSchema"
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ self.incr_stats("init", self.table_extra_metadata.__name__)
+
+ database = DatabaseDAO.find_by_id(pk)
+ if database is None:
Review Comment:
Ah, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #28063:
URL: https://github.com/apache/superset/pull/28063#discussion_r1570874925
##########
superset/databases/api.py:
##########
@@ -812,13 +831,92 @@ def table_extra_metadata(
500:
$ref: '#/components/responses/500'
"""
- self.incr_stats("init", self.table_metadata.__name__)
+ self.incr_stats("init", self.table_extra_metadata_deprecated.__name__)
parsed_schema = parse_js_uri_path_item(schema_name, eval_undefined=True)
table_name = cast(str, parse_js_uri_path_item(table_name))
- payload = database.db_engine_spec.extra_table_metadata(
- database, table_name, parsed_schema
- )
+ table = Table(table_name, parsed_schema)
+ payload = database.db_engine_spec.get_extra_table_metadata(database, table)
+ return self.response(200, **payload)
+
+ @expose("/<int:pk>/table_metadata/extra/", methods=("GET",))
+ @protect()
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".table_extra_metadata",
+ log_to_statsd=False,
+ )
+ def table_extra_metadata(self, pk: int) -> FlaskResponse:
+ """
+ Get extra metadata for a given table.
+
+ Optionally, a schema and a catalog can be passed, if different from the default
+ ones.
+ ---
+ get:
+ summary: Get table extra metadata
+ description: >-
+ Extra metadata associated with the table (partitions, description, etc.)
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The database id
+ - in: query
+ schema:
+ type: string
+ name: name
+ required: true
+ description: Table name
+ - in: query
+ schema:
+ type: string
+ name: schema
+ description: >-
+ Optional table schema, if not passed the schema configured in the database
+ will be used
+ - in: query
+ schema:
+ type: string
+ name: catalog
+ description: >-
+ Optional table catalog, if not passed the catalog configured in the
+ database will be used
+ responses:
+ 200:
+ description: Table extra metadata information
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/TableExtraMetadataResponseSchema"
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ self.incr_stats("init", self.table_extra_metadata.__name__)
+
+ if not (database := DatabaseDAO.find_by_id(pk)):
+ raise DatabaseNotFoundException("No such database")
+
+ try:
+ parameters = QualifiedTableSchema().load(request.args)
+ except ValidationError as ex:
+ raise InvalidPayloadSchemaError(ex) from ex
+
+ table = Table(parameters["name"], parameters["schema"], parameters["catalog"])
+ try:
+ security_manager.raise_for_access(database=database, table=table)
+ except SupersetSecurityException as ex:
+ # instead of raising 403, raise 404 to hide table existence
Review Comment:
nice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #28063:
URL: https://github.com/apache/superset/pull/28063
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #28063:
URL: https://github.com/apache/superset/pull/28063#issuecomment-2061052711
> Should we mark it as deprecated now and remove it in the next release, or should we mark is as deprecated in 5.0 and remove it in 6.0?
You can mark it as deprecated now. Please add a proposal [here](https://github.com/orgs/apache/projects/345) to remove the old endpoint in 5.0.
> In this new endpoint, table, schema, and catalog are all passed as query parameters. This allows slashes in all of them, and prevents polluting logs with confusing URLs such as /api/v1/databses/1/table_extra/my_table/undefined/.
Nesting resources or not is a architectural decision, with [trade-offs](https://www.moesif.com/blog/technical/api-design/REST-API-Design-Best-Practices-for-Sub-and-Nested-Resources/), that we need to make consistently in our API. Because we built the API one endpoint at a time, we don't have consistency in nomenclature, response types, params, etc. I'm hoping to fix all these problem with v2 where we would analyze all existing endpoints and propose a consistent API in a SIP. For that reason, any pattern you choose now is acceptable and will be reviewed in v2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #28063:
URL: https://github.com/apache/superset/pull/28063#discussion_r1570879847
##########
superset/db_engine_specs/base.py:
##########
@@ -1034,21 +1035,33 @@ def normalize_indexes(cls, indexes: list[dict[str, Any]]) -> list[dict[str, Any]
return indexes
@classmethod
- def extra_table_metadata( # pylint: disable=unused-argument
+ def get_extra_table_metadata( # pylint: disable=unused-argument
cls,
database: Database,
- table_name: str,
- schema_name: str | None,
+ table: Table,
) -> dict[str, Any]:
"""
Returns engine-specific table metadata
:param database: Database instance
- :param table_name: Table name
- :param schema_name: Schema name
+ :param table: A Table instance
:return: Engine-specific table metadata
"""
- # TODO: Fix circular import caused by importing Database
+ # old method that doesn't work with catalogs
+ if hasattr(cls, "extra_table_metadata"):
Review Comment:
optional:
do:
``` python
if not hasattr(cls, "extra_table_metadata"):
return {}
...
```
to reduce nesting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #28063:
URL: https://github.com/apache/superset/pull/28063#discussion_r1570904529
##########
superset/db_engine_specs/base.py:
##########
@@ -1034,21 +1035,33 @@ def normalize_indexes(cls, indexes: list[dict[str, Any]]) -> list[dict[str, Any]
return indexes
@classmethod
- def extra_table_metadata( # pylint: disable=unused-argument
+ def get_extra_table_metadata( # pylint: disable=unused-argument
cls,
database: Database,
- table_name: str,
- schema_name: str | None,
+ table: Table,
) -> dict[str, Any]:
"""
Returns engine-specific table metadata
:param database: Database instance
- :param table_name: Table name
- :param schema_name: Schema name
+ :param table: A Table instance
:return: Engine-specific table metadata
"""
- # TODO: Fix circular import caused by importing Database
+ # old method that doesn't work with catalogs
+ if hasattr(cls, "extra_table_metadata"):
Review Comment:
Nice, I'll fix it in the next PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #28063:
URL: https://github.com/apache/superset/pull/28063#issuecomment-2061256418
Thanks, @michael-s-molina!
> Nesting resources or not is an architectural decision, with [trade-offs](https://www.moesif.com/blog/technical/api-design/REST-API-Design-Best-Practices-for-Sub-and-Nested-Resources/), that we need to make consistently in our API. Because we built the API one endpoint at a time, we don't have consistency in nomenclature, response types, params, etc. I'm hoping to fix all these problems with v2 where we would analyze all existing endpoints and propose a consistent API in a SIP. For that reason, any pattern you choose now is acceptable and will be reviewed in v2.
Oh, I'm not against nesting resources in general. The problem here is that table names can contain slashes, and I think we should assume that schemas and catalogs can too. Imagine we have this route:
`/some/api/<path:catalog>/<path:schema>/<path:table>/`
And we have a table with slashes in the catalog, schema, and name, which we escape with `%2F` when doing a request:
`/some/api/my%2Fcatalog/my%2Fschema/my%2Ftable/`
What the Flask router sees in the request is:
`/some/api/my/catalog/my/schema/my/table/`
Which gets mapped to:
- catalog: `my`
- schema: `catalog`
- table: `my/schema/my/table`
So we'd have to **double escape** the slashes:
`/some/api/my%252Fcatalog/my%252Fschema/my%252Ftable/`
And the route would have unescape them back. For cases like this I think it's easier to pass the parameters as query arguments:
`/some/api/?table=my/table&schema=my/schema&catalog=my/catalog`
While longer, I think it's easier to read. And when some of those parameters are missing it gets better:
`/some/api/?table=my/table`
vs.
`/some/api/undefined/undefined/my%252Ftable/`
So something to think about when designing v2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(sip-95): new endpoint for extra table metadata [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #28063:
URL: https://github.com/apache/superset/pull/28063#discussion_r1569607807
##########
superset/databases/schemas.py:
##########
@@ -1195,3 +1195,27 @@ class OAuth2ProviderResponseSchema(Schema):
class Meta: # pylint: disable=too-few-public-methods
# Ignore unknown fields that might be sent by the OAuth2 provider
unknown = EXCLUDE
+
+
+class QualifiedTableSchema(Schema):
+ """
+ Schema for a qualified table reference.
+
+ Catalog and schema can be ommited, to fallback to default values. Table name must be
+ present.
+ """
+
+ table = fields.String(
Review Comment:
I know I used the term `table` in the `Table` class, but I wonder if this should be name given it's referring to the name of the table. This is also consistent with your description on line 1210.
##########
superset/db_engine_specs/base.py:
##########
@@ -1034,21 +1035,33 @@ def normalize_indexes(cls, indexes: list[dict[str, Any]]) -> list[dict[str, Any]
return indexes
@classmethod
- def extra_table_metadata( # pylint: disable=unused-argument
+ def get_extra_table_metadata( # pylint: disable=unused-argument
cls,
database: Database,
- table_name: str,
- schema_name: str | None,
+ table: Table,
Review Comment:
Nice!
##########
superset/databases/api.py:
##########
@@ -812,13 +831,91 @@ def table_extra_metadata(
500:
$ref: '#/components/responses/500'
"""
- self.incr_stats("init", self.table_metadata.__name__)
+ self.incr_stats("init", self.table_extra_metadata_deprecated.__name__)
parsed_schema = parse_js_uri_path_item(schema_name, eval_undefined=True)
table_name = cast(str, parse_js_uri_path_item(table_name))
- payload = database.db_engine_spec.extra_table_metadata(
- database, table_name, parsed_schema
- )
+ table = Table(table_name, parsed_schema)
+ payload = database.db_engine_spec.get_extra_table_metadata(database, table)
+ return self.response(200, **payload)
+
+ @expose("/<int:pk>/table_metadata/extra/", methods=("GET",))
+ @protect()
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".table_extra_metadata",
+ log_to_statsd=False,
+ )
+ def table_extra_metadata(self, pk: int) -> FlaskResponse:
+ """
+ Get extra metadata for a given table.
+
+ Optionally, a schema and a catalog can be passed, if different from the default
+ ones.
+ ---
+ get:
+ summary: Get table extra metadata
+ description: >-
+ Extra metadata associated with the table (partitions, description, etc.)
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The database id
+ - in: query
+ schema:
+ type: string
+ name: table
+ required: true
+ description: Table name
+ - in: query
+ schema:
+ type: string
+ name: schema
+ description: >-
+ Optional table schema, if not passed default schema will be used
+ - in: query
+ schema:
+ type: string
+ name: catalog
+ description: >-
+ Optional table catalog, if not passed default catalog will be used
+ responses:
+ 200:
+ description: Table extra metadata information
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/TableExtraMetadataResponseSchema"
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ self.incr_stats("init", self.table_extra_metadata.__name__)
+
+ database = DatabaseDAO.find_by_id(pk)
+ if database is None:
Review Comment:
```suggestion
if not (database := DatabaseDAO.find_by_id(pk)):
```
##########
superset/db_engine_specs/presto.py:
##########
@@ -1224,14 +1225,20 @@ def expand_data( # pylint: disable=too-many-locals
return all_columns, data, expanded_columns
@classmethod
- def extra_table_metadata(
- cls, database: Database, table_name: str, schema_name: str | None
+ def get_extra_table_metadata(
+ cls,
+ database: Database,
+ table: Table,
) -> dict[str, Any]:
metadata = {}
- if indexes := database.get_indexes(table_name, schema_name):
+ if indexes := database.get_indexes(table.table, table.schema):
col_names, latest_parts = cls.latest_partition(
- table_name, schema_name, database, show_first=True, indexes=indexes
+ table.table,
Review Comment:
Should we be passing a `Table` instance everywhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org