You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/23 13:52:31 UTC
[GitHub] [superset] villebro commented on a change in pull request #13294: feat(explore): Postgres datatype conversion
villebro commented on a change in pull request #13294:
URL: https://github.com/apache/superset/pull/13294#discussion_r581050786
##########
File path: superset/db_engine_specs/base.py
##########
@@ -1097,3 +1098,56 @@ def get_extra_params(database: "Database") -> Dict[str, Any]:
def is_readonly_query(cls, parsed_query: ParsedQuery) -> bool:
"""Pessimistic readonly, 100% sure statement won't mutate anything"""
return parsed_query.is_select() or parsed_query.is_explain()
+
+ @classmethod
+ def get_column_type(
+ cls,
+ source: utils.ColumnTypeSource,
+ db_type_map: Dict[utils.GenericDataType, List[str]],
+ native_type: Union[utils.GenericDataType, str],
+ ) -> Tuple[Union[utils.GenericDataType, str], bool]:
+ for generic_type in db_type_map:
+ is_dttm = generic_type == utils.GenericDataType.TEMPORAL
+ for db_type in db_type_map[generic_type]:
+ if db_type == native_type:
+ if source == utils.ColumnTypeSource.CURSOR_DESCRIPION:
+ return db_type, is_dttm
+ if source == utils.ColumnTypeSource.GET_TABLE:
+ return generic_type, is_dttm
+ return "", False
+
+ @classmethod
+ def get_column_spec(
+ cls,
+ source: utils.ColumnTypeSource,
+ column_name: str,
+ native_type: Union[utils.GenericDataType, str],
+ ) -> utils.ColumnSpec:
+ postgres_types_map: Dict[utils.GenericDataType, List[str]] = {
+ utils.GenericDataType.NUMERIC: [
+ "smallint",
+ "integer",
+ "bigint",
+ "decimal",
+ "numeric",
+ "real",
+ "double precision",
+ "smallserial",
+ "serial",
+ "bigserial",
+ ],
+ utils.GenericDataType.STRING: ["varchar", "char", "text",],
+ utils.GenericDataType.TEMPORAL: [
+ "DATE",
+ "TIME",
+ "TIMESTAMP",
+ "TIMESTAMPTZ",
+ "INTERVAL",
+ ],
+ utils.GenericDataType.BOOLEAN: ["boolean",],
+ }
Review comment:
I would break this out so that the base engine only provides a default mapping (can return `None` now), and then each engine would implement the method as they want. An example of similar logic: `convert_dttm`: `BaseEngineSpec` returns `None` (https://github.com/apache/superset/blob/99a0c8a8a129502d6253e000c14db31ab8b0bb19/superset/db_engine_specs/base.py#L533-L542), but e.g. `BigQueryEngineSpec` implements it (https://github.com/apache/superset/blob/99a0c8a8a129502d6253e000c14db31ab8b0bb19/superset/db_engine_specs/bigquery.py#L89-L100).
For matching the native type to specific types I would probably use a sequence of regexp to to find the first match. Something similar has been done on Presto t map the database type to SQLAlchemy types (this new functionality would replace the old logic): https://github.com/apache/superset/blob/99a0c8a8a129502d6253e000c14db31ab8b0bb19/superset/db_engine_specs/presto.py#L358-L384 . Some engines like Druid will also have fixed types for certain column names (e.g. `__time` is actually `TIMESTAMP` despite being returned as `STRING` on the cursor description), so these should probably be caught before doing the matching from the database type.
----------------------------------------------------------------
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.
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