[SIP-15] Adding database level python-date-format #8474

john-bodley · 2019-10-30T07:33:56Z

SUMMARY

This PR addresses the SIP-15 issue where for string column types the start endpoint may behave like (start rather than [start due to lexicographical ordering if the RHS is not encoded to mimic the formatting of the LHS.

For example at Airbnb we have over 20,000 tables in Superset which have a ds column (datestamp) which adheres to the YYYY-MM-DD ISO 8601 format however no python-date-format is specified for these columns and thus the filter endpoints behave like,

> SELECT
    '2018-01-01' >= '2018-01-01 00:00:00',
    '2018-01-01' <= '2018-01-02 00:00:00'
FALSE TRUE

i.e., (start rather than [start. This problem could be remedied by setting the python-date-format for every table but that seems overkill and doesn't ensure newly added tables will define the formatting. This PR creates a default column/python-date-format mapping on a per engine basis where one can specify a fallback (default) format if not specified.

Note #8464 switched the order of evaluation for obtaining the datetime SQL literal ensuring known per engine data types conversions, i.e., DATE, TIMESTAMP, etc. are applied prior to any python-date-format. This helps to ensure that the mapping isn't heavy handed, i.e., applying the format in an unnecessary or problematic manner.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

Has associated issue: [SIP-15] Fixing datetime conversion and SQL literal #8464
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 @etr2460 @michellethomas @mistercrunch @villebro @willbarrett

willbarrett

A couple of grammar nits and a non-blocking question about non-inclusive start times. Thank you for adding the documentation!

willbarrett · 2019-10-30T16:42:15Z

docs/installation.rst

@@ -1306,7 +1306,17 @@ SIP-15

 `SIP-15 <https://github.com/apache/incubator-superset/issues/6360>`_ aims to ensure that time intervals are handled in a consistent and transparent manner for both the Druid and SQLAlchemy connectors.

-Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave like exclusive depending on the time column (refer to the SIP for details) and thus the endpoint behavior could be unknown. To aid with transparency the current endpoint behavior is explicitly called out in the chart time range (post SIP-15 this will be [start, end) for all connectors and databases). One can override the defaults on a per database level via the ``extra``
+Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave like exclusive for string columns (due to lexicographical ordering) if no formatting was defined and the column formatting did not conform to an ISO 8601 date-time (refer to the SIP for details).


Nit: it would be a convenience for the reader to add a link to the SIP issue on Github.

@willbarrett there's a link to the GitHub issue on line #1307.

Whup, I missed the context. Thanks!

willbarrett · 2019-10-30T16:45:10Z

superset/connectors/sqla/views.py

@@ -112,6 +112,9 @@ class TableColumnInlineView(CompactCRUDMixin, SupersetModelView):
                "define an expression and type for transforming the string into a "
                "date or timestamp. Note currently time zones are not supported. "
                "If time is stored in epoch format, put `epoch_s` or `epoch_ms`."
+                "If no pattern is specified we fallback to using the optional defaults "


Nit: fallback is a noun, fall back is the verb form.

willbarrett · 2019-10-30T16:45:31Z

superset/assets/src/datasource/DatasourceEditor.jsx

@@ -113,7 +113,10 @@ function ColumnCollectionTable({
                      you will need to define an expression and type for
                      transforming the string into a date or timestamp. Note
                      currently time zones are not supported. If time is stored
-                      in epoch format, put \`epoch_s\` or \`epoch_ms\`.`)}
+                      in epoch format, put \`epoch_s\` or \`epoch_ms\`. If no pattern
+                      is specified we fallback to using the optional defaults on a per


Nit: fallback is a noun, fall back is the verb form.

willbarrett · 2019-10-30T16:48:16Z

superset/connectors/sqla/models.py

@@ -169,15 +169,19 @@ def get_time_filter(
        col = self.get_sqla_col(label="__time")
        l = []
        if start_dttm:
-            l.append(col >= text(self.dttm_sql_literal(start_dttm)))
+            l.append(


If there is now configuration for inclusive/exclusive start points, should we have another if condition in this block supporting col > <start time literal statement>?

@willbarrett per SIP-15 the end state is [start, end) i.e., this should not be configurable. The reason for the interim configuration is to allow users to preview their charts using the [start, end) time range endpoints before saving and thus persisting this information in the chart form data.

The time_range_endpoints form-data is merely there as a mechanism to provide both the current and future states simultaneously for a period which enabling an institution to manually fix charts as necessary.

OK, thanks for clarifying!

john-bodley · 2019-10-30T17:50:21Z

@willbarrett thanks for the review. I addressed your fallback vs. fall back comments.

codecov-io · 2019-10-30T18:23:52Z

Codecov Report

Merging #8474 into master will increase coverage by 4.93%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #8474      +/-   ##
==========================================
+ Coverage   66.52%   71.46%   +4.93%     
==========================================
  Files         449      117     -332     
  Lines       22595    12334   -10261     
  Branches     2367        0    -2367     
==========================================
- Hits        15032     8814    -6218     
+ Misses       7425     3520    -3905     
+ Partials      138        0     -138

Impacted Files	Coverage Δ
superset/config.py	`89.34% <ø> (ø)`	⬆️
superset/connectors/sqla/views.py	`67.52% <ø> (ø)`	⬆️
superset/connectors/sqla/models.py	`84.81% <66.66%> (-0.12%)`	⬇️
...sets/src/explore/components/ExploreChartHeader.jsx
superset/assets/src/components/Checkbox.jsx
...perset/assets/src/dashboard/util/getEmptyLayout.js
...ts/src/dashboard/util/getComponentWidthFromDrop.js
.../assets/src/dashboard/reducers/dashboardFilters.js
...rc/explore/components/controls/AnnotationLayer.jsx
superset/assets/src/welcome/DashboardTable.jsx
... and 324 more

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 18c6d17...04b55d2. Read the comment docs.

(cherry picked from commit 554a6d8)

pull-request-size bot added the size/M label Oct 30, 2019

john-bodley force-pushed the john-bodley--sip-15--python-date-format-fallback branch from 52131dd to 892164f Compare October 30, 2019 07:34

willbarrett reviewed Oct 30, 2019

View reviewed changes

john-bodley force-pushed the john-bodley--sip-15--python-date-format-fallback branch 3 times, most recently from 2550f04 to 5fb124e Compare October 30, 2019 17:49

[sip-15] Adding database level python-date-format

04b55d2

john-bodley force-pushed the john-bodley--sip-15--python-date-format-fallback branch from 5fb124e to 04b55d2 Compare October 30, 2019 17:53

willbarrett approved these changes Oct 30, 2019

View reviewed changes

john-bodley merged commit 554a6d8 into apache:master Oct 31, 2019

john-bodley added a commit to john-bodley/superset that referenced this pull request Oct 31, 2019

[sip-15] Adding database level python-date-format (apache#8474)

05f0c8d

(cherry picked from commit 554a6d8)

graceguo-supercat pushed a commit that referenced this pull request Nov 13, 2019

[sip-15] Adding database level python-date-format (#8474)

9c9ddc6

dpgaspar added v0.35 and removed v0.35 labels Dec 20, 2019

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIP-15] Adding database level python-date-format #8474

[SIP-15] Adding database level python-date-format #8474

john-bodley commented Oct 30, 2019

willbarrett left a comment

willbarrett Oct 30, 2019

john-bodley Oct 30, 2019

willbarrett Oct 30, 2019

willbarrett Oct 30, 2019

willbarrett Oct 30, 2019

willbarrett Oct 30, 2019

john-bodley Oct 30, 2019

willbarrett Oct 30, 2019

john-bodley commented Oct 30, 2019

codecov-io commented Oct 30, 2019 •

edited

Loading

[SIP-15] Adding database level python-date-format #8474

[SIP-15] Adding database level python-date-format #8474

Conversation

john-bodley commented Oct 30, 2019

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

willbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Oct 30, 2019

codecov-io commented Oct 30, 2019 • edited Loading

Codecov Report

codecov-io commented Oct 30, 2019 •

edited

Loading