[schema] Deprecating the table_columns.database_expression column

[john-bodley]

CATEGORY

Choose one

• Bug Fix
• 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):

1. python_date_format: Which is used explicitly for string like or numeric types (represent Unix 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.

This PR modifies the dttm_sql_literal method to simply deprecate the need for database_expression as the appropriate conversion will occur here.

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. Note if the timestamp column was expressed as milliseconds since epoch the column could have been defined as:

• type: BIGINT
• python_date_format: epoch_ms

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

CI.

ADDITIONAL INFORMATION

• 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 @villebro

[john-bodley]

#7656

[villebro]

typo: whcih -> which

[villebro]

One small typo and needs rebase, otherwise LGTM. Kudos for achieving diff with more - than + 👍

[john-bodley]

@betodealmeida @mistercrunch are you onboard with this change?

[betodealmeida]

+1 from me

[codecov-io]

Codecov Report

Merging #7653 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7653      +/-   ##
==========================================
- Coverage   65.89%   65.89%   -0.01%     
==========================================
  Files         459      459              
  Lines       22016    22013       -3     
  Branches     2417     2417              
==========================================
- Hits        14507    14505       -2     
+ Misses       7388     7387       -1     
  Partials      121      121

Impacted Files	Coverage Δ
superset/connectors/base/models.py	89.89% <ø> (ø)	⬆️
...uperset/assets/src/datasource/DatasourceEditor.jsx	61.84% <ø> (ø)	⬆️
superset/connectors/sqla/views.py	65.28% <ø> (ø)	⬆️
superset/connectors/sqla/models.py	82.48% <100%> (+0.08%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0125ba4...970e4c4. Read the comment docs.