villebro commented on a change in pull request #13595:
URL: https://github.com/apache/superset/pull/13595#discussion_r595874584
##########
File path: tests/sqla_models_tests.py
##########
@@ -54,10 +54,11 @@ def test_is_time_druid_time_col(self):
database = Database(database_name="druid_db",
sqlalchemy_uri="druid://db")
tbl = SqlaTable(table_name="druid_tbl", database=database)
- col = TableColumn(column_name="__time", type="INTEGER", table=tbl)
- self.assertEqual(col.is_dttm, None)
- DruidEngineSpec.alter_new_orm_column(col)
- self.assertEqual(col.is_dttm, True)
+ column_spec = DruidEngineSpec.get_column_spec(column_name="__time")
+ print(">>> {}".format(column_spec))
+ self.assertTrue(column_spec.is_dttm)
Review comment:
The purpose of this test is to make sure that a column with the name
`__time` and `type="INTEGER"` (=the non-temporal type that comes back from the
db) is interpreted as a temporal column and gets the correct sqla type.
Therefore it's important to make sure that the type is defined in the
`TableColumn` instantiation.
Also, some debugging leftover:
```suggestion
column_spec = DruidEngineSpec.get_column_spec(column_name="__time")
self.assertTrue(column_spec.is_dttm)
```
##########
File path: tests/sqla_models_tests.py
##########
@@ -54,10 +54,11 @@ def test_is_time_druid_time_col(self):
database = Database(database_name="druid_db",
sqlalchemy_uri="druid://db")
tbl = SqlaTable(table_name="druid_tbl", database=database)
- col = TableColumn(column_name="__time", type="INTEGER", table=tbl)
- self.assertEqual(col.is_dttm, None)
- DruidEngineSpec.alter_new_orm_column(col)
- self.assertEqual(col.is_dttm, True)
+ column_spec = DruidEngineSpec.get_column_spec(column_name="__time")
+ print(">>> {}".format(column_spec))
+ self.assertTrue(column_spec.is_dttm)
+ col = TableColumn(column_name="__time", type=column_spec.sqla_type,
table=tbl)
+ self.assertTrue(col)
Review comment:
What is the purpose of this assertion? Also, the type of the `type`
field on `BaseColumn` and the child class `TableColumn` is `String(32)`, i.e.
should not be passed a sqla column type.
##########
File path: superset/db_engine_specs/druid.py
##########
@@ -91,3 +99,26 @@ def convert_dttm(cls, target_type: str, dttm: datetime) ->
Optional[str]:
if tt in (utils.TemporalType.DATETIME, utils.TemporalType.TIMESTAMP):
return f"""TIME_PARSE('{dttm.isoformat(timespec="seconds")}')"""
return None
+
+ @classmethod
+ def get_column_spec( # type: ignore
+ # pylint: disable=arguments-differ
+ cls,
+ column_name: Optional[str] = "",
+ column_type_mappings: Tuple[
+ Tuple[
+ Pattern[str],
+ Union[TypeEngine, Callable[[Match[str]], TypeEngine]],
+ GenericDataType,
+ ],
+ ...,
+ ] = (),
+ native_type: Optional[str] = "",
+ source: utils.ColumnTypeSource = utils.ColumnTypeSource.GET_TABLE,
+ ) -> Union[ColumnSpec, None]:
+
+ if column_name == "__time":
+ print(">>> hello!")
Review comment:
nit:
```suggestion
```
----------------------------------------------------------------
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]
