[GitHub] [incubator-superset] john-bodley opened a new pull request #7653: [schema] Deprecating the table_columns.database_expression column

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

john-bodley opened a new pull request #7653: [schema] Deprecating the table_columns.database_expression column
URL: https://github.com/apache/incubator-superset/pull/7653
 
 
   ### CATEGORY
   
   Choose one
   
   - [ ] Bug Fix
   - [x] Enhancement (new features, refinement)
   - [ ] Refactor
   - [ ] Add tests
   - [ ] Build / Development Environment
   - [ ] Documentation
   
   ### SUMMARY
   
   As far as I can tell I believe that the `database_expression` field for SQLAlchemy datasources is obsolete. Why? There are currently two (independent) mechanisms for for converting a Python date-time into the appropriate data type for temporal filters (which are only used [here](https://github.com/apache/incubator-superset/blob/45b988051614bda145add7934af2d310f3a94754/superset/connectors/sqla/models.py#L135)): 
   
   1. `python_date_format`: Which is used explicitly for string like or numeric types (represent epoch timestamps) and converts a Python `datetime` object into either a string or numeric type.
   2. `database_expression`: Which is used for date, date-time like types and converts a Python `datatime` object into the native database type using a SQL expression. 
   
   The reason the later is not required is each engine has already defined the date-time conversion (via the `convert_dttm` function for the various temporal types the engine supports in [db_engine_specs](https://github.com/apache/incubator-superset/blob/master/superset/db_engine_specs.py). 
   
   This PR modifies the [`dttm_sql_literal `](https://github.com/apache/incubator-superset/blob/45b988051614bda145add7934af2d310f3a94754/superset/connectors/sqla/models.py#L176) method to simply deprecate the need for `database_expression` as the appropriate conversion will occur [here](https://github.com/apache/incubator-superset/blob/45b988051614bda145add7934af2d310f3a94754/superset/connectors/sqla/models.py#L200).
   
   For reference I examined our deployment where the `database_expression` field was defined, of the 15 records only four contained valid SQL and in all cases they were being used either i)incorrectly, i.e., the filter comparison resulted in mixed types, or ii) in an unnecessary manner. For example there was a record (associated with a Presto datasource) which contained the following fields: 
   
       `type`: `VARCHAR`
       `expression`: `FROM_UNIXTIME(CAST(timestamp as BIGINT) / 1000)`
       `database_expression`: DATE_PARSE({}', '%%Y-%%m-%%d %%H:%%i:%%s'`
   
   The mistake here is that `FROM_UNIXTIME` returns a `TIMESTAMP` rather than `VARCHAR` and had the type been correctly defined, the `PrestoEngineSpec.convert_dttm` method would have correctly handled the conversion [here](https://github.com/apache/incubator-superset/blob/45b988051614bda145add7934af2d310f3a94754/superset/db_engine_specs.py#L1199).
   
   ### TEST PLAN
   
   CI. 
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   ### REVIEWERS
   
   to: @betodealmeida @michellethomas @mistercrunch 

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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org