-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[schema] Deprecating the table_columns.database_expression column #7653
[schema] Deprecating the table_columns.database_expression column #7653
Conversation
802406b
to
eb87690
Compare
UPDATING.md
Outdated
@@ -23,6 +23,11 @@ assists people when migrating to a new version. | |||
|
|||
## Next Version | |||
|
|||
* [7653](https://github.com/apache/incubator-superset/pull/7653): a change | |||
whcih deprecates the table_columns.database_expression column. Expressions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: whcih -> which
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small typo and needs rebase, otherwise LGTM. Kudos for achieving diff with more - than + 👍
@betodealmeida @mistercrunch are you onboard with this change? |
6aaf4cd
to
9d826ae
Compare
+1 from me |
f5fec06
to
8f72373
Compare
8f72373
to
970e4c4
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
CATEGORY
Choose one
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):python_date_format
: Which is used explicitly for string like or numeric types (represent Unix timestamps) and converts a Pythondatetime
object into either a string or numeric type.database_expression
: Which is used for date, date-time like types and converts a Pythondatatime
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 fordatabase_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 aTIMESTAMP
rather thanVARCHAR
and had the type been correctly defined, thePrestoEngineSpec.convert_dttm
method would have correctly handled the conversion here. Note if thetimestamp
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
REVIEWERS
to: @betodealmeida @michellethomas @mistercrunch @villebro