feat: use strict ISO8601 datetime format in postgres engine spec #7057
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7057 +/- ##
==========================================
- Coverage 64.46% 64.46% -0.01%
==========================================
Files 421 421
Lines 20537 20539 +2
Branches 2247 2247
==========================================
+ Hits 13240 13241 +1
- Misses 7170 7171 +1
Partials 127 127
Continue to review full report at Codecov.
|
kristw
changed the title
kristw
added
the
enhancement:request
| @@ -535,6 +535,10 @@ def get_timestamp_column(expression, column_name): | |||
| return f'"{column_name}"' | |||
| return column_name | |||
|
|
|||
| @classmethod | |||
| def convert_dttm(cls, target_type, dttm): | |||
| return "'{}'".format(dttm.strftime('%Y-%m-%dT%H:%M:%S')) | |||
There was a problem hiding this comment.
wait isn't that identical to the parent's class method?
There was a problem hiding this comment.
No, this one has the time designator, the parent's class method has whitespace instead. Please see a bit more detailed explanation in the PR description.
There was a problem hiding this comment.
Oh sorry I missed that. Any idea as to whether this may be needed at the PostgresBaseEngineSpec level? (would also affect Snowflake, Vertica, Redshift)
There was a problem hiding this comment.
I'd keep it simple in this PR. It really depends on the way they handle timestamps date/time types. Unfortunately, I am not familiar with either of those databases.
|
Closing it |
The
PostgresEngineSpechave the default implementation of theconvert_dttmmethod that follows the%Y-%m-%d %H:%M:%Sdate time format. Despite the fact that the particular format is supported by PostgreSQL, it is not ISO8601 compliant, see section 4.3.2. The standard permits to omit the time designatorT, but does not imply permission to replace it with a whitespace.Therefore, it would be harder to use Superset with databases that support the PostgreSQL Wire Protocol and strictly follow ISO8601. This PR improves Superset integration with such databases.