john-bodley commented on a change in pull request #16950:
URL: https://github.com/apache/superset/pull/16950#discussion_r732263165
##########
File path: superset/connectors/sqla/models.py
##########
@@ -334,8 +334,7 @@ def dttm_sql_literal(
],
) -> str:
"""Convert datetime object to a SQL expression string"""
- dttm_type = self.type or ("DATETIME" if self.is_dttm else None)
- sql = self.db_engine_spec.convert_dttm(dttm_type, dttm) if dttm_type
else None
+ sql = self.db_engine_spec.convert_dttm(self.type, dttm) if self.type
else None
Review comment:
@villebro the issue is any column which is flagged as being `is_dttm`
without a type is treated as a `DATETIME` type which could result in the wrong
conversion. It's not clear that to me that `DATETIME` is the best default, for
example at Airbnb the vast majority of temporal columns are actually
`VARCHAR`/`STRING` due to Hive partitions (which I _believe_ need to be
strings).
Again it seems like the original change was unnecessary and causes issues.
The actual solution/paved path workflow—which worked previously—is merely to
define the type of the calculated column. We likely should make this a required
field and perform form validation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
