villebro commented on a change in pull request #13294:
URL: https://github.com/apache/superset/pull/13294#discussion_r584719970
##########
File path: superset/db_engine_specs/base.py
##########
@@ -967,25 +980,27 @@ def make_label_compatible(cls, label: str) -> Union[str,
quoted_name]:
return label_mutated
@classmethod
- def get_sqla_column_type(cls, type_: Optional[str]) ->
Optional[TypeEngine]:
+ def get_sqla_column_type(
+ cls, column_type: Optional[str]
+ ) -> Tuple[Union[TypeEngine, utils.GenericDataType, None]]:
"""
Return a sqlalchemy native column type that corresponds to the column
type
defined in the data source (return None to use default type inferred by
SQLAlchemy). Override `column_type_mappings` for specific needs
(see MSSQL for example of NCHAR/NVARCHAR handling).
- :param type_: Column type returned by inspector
+ :param column_type: Column type returned by inspector
:return: SqlAlchemy column type
"""
- if not type_:
- return None
- for regex, sqla_type in cls.column_type_mappings:
- match = regex.match(type_)
+ if not column_type:
+ return None, None
+ for regex, sqla_type, generic_type in cls.column_type_mappings:
+ match = regex.match(column_type)
if match:
if callable(sqla_type):
- return sqla_type(match)
- return sqla_type
- return None
+ return sqla_type(match), generic_type
+ return sqla_type, generic_type
+ return None, None
Review comment:
For cases where we want to return an empty value (=no match was made),
I'd perhaps prefer returning a pure `None` instead of a `Tuple` with `None`s.
##########
File path: superset/db_engine_specs/base.py
##########
@@ -967,25 +980,27 @@ def make_label_compatible(cls, label: str) -> Union[str,
quoted_name]:
return label_mutated
@classmethod
- def get_sqla_column_type(cls, type_: Optional[str]) ->
Optional[TypeEngine]:
+ def get_sqla_column_type(
+ cls, column_type: Optional[str]
+ ) -> Tuple[Union[TypeEngine, utils.GenericDataType, None]]:
Review comment:
I would consider just leaving the signature of this method unchanged for
now; otherwise I'd just remove this one and start using the new
`get_column_spec` method and implement that wherever `get_sqla_column_type` is
being used, as that's the end state we want to aim for.
##########
File path: superset/utils/core.py
##########
@@ -148,6 +148,10 @@ class GenericDataType(IntEnum):
STRING = 1
TEMPORAL = 2
BOOLEAN = 3
+ ARRAY = 4
+ JSON = 5
+ MAP = 6
+ ROW = 7
Review comment:
We need to make sure this logic is in synced with `superset-ui/core` -
for now it might be a good idea to just map all complex types to `STRING` as
has previously been done, and later introduce more complex generic types once
we add proper support for them.
##########
File path: superset/db_engine_specs/base.py
##########
@@ -179,6 +185,13 @@ class BaseEngineSpec: # pylint:
disable=too-many-public-methods
),
}
+ dttm_types = [
+ types.TIME,
+ types.TIMESTAMP,
+ types.TIMESTAMP(timezone=True),
+ types.Interval,
+ ]
Review comment:
I don't believe this is needed anymore.
##########
File path: superset/db_engine_specs/base.py
##########
@@ -1097,3 +1112,26 @@ 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()
+
+ def get_column_spec(
+ self,
+ column_name: Optional[str],
+ native_type: str,
+ source: utils.ColumnTypeSource = utils.ColumnTypeSource.GET_TABLE,
+ ) -> utils.ColumnSpec:
+
+ column_type, generic_type = self.get_sqla_column_type(native_type)
+ is_dttm = generic_type == utils.GenericDataType.TEMPORAL
+
+ if column_name: # Further logic to be implemented
+ pass
+ if (
+ source == utils.ColumnTypeSource.CURSOR_DESCRIPION
+ ): # Further logic to be implemented
+ pass
Review comment:
For now we can assume `GET_TABLE` and `CURSOR_DESCRIPTION` is handled
with the same matching logic - this can later be refined for engines where this
makes a difference. Also, we might consider keeping the base implementation as
simple as possible, and leaving the more complex logic to the individual db
engine specs.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
