[GitHub] [superset] john-bodley commented on pull request #16795: feat: handle temporal columns in group bys

[GitBox <gi...@apache.org>]

john-bodley commented on pull request #16795:
URL: https://github.com/apache/superset/pull/16795#issuecomment-955122235


   @betodealmeida and @villebro I don't believe this logic works for all cases. You could have a temporal field which is encoded as a string per the Python date format which shouldn't be converted to a `TIMESTAMP`. 
   
   Isn't the correct logic to use the type of the column rather than assuming it's a `TIMESTAMP`? Furthermore `self.db_engine_spec.convert_dttm("TIMESTAMP", dttm)` could return `None` (even for the `TIMESTAMP` target type). I was thinking the logic should be of the form,
   
   ```python
   if column_map[dimension].is_temporal and and isinstance(value, str):
       if rv := self.db_engine_spec.convert_dttm(
           column_map[dimension].type, 
           dateutil.parser.parse(value),
       ):
           value = text(rv)
   ```
   


-- 
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