[SIP-15] Fixing datetime conversion and SQL literal #8464

john-bodley · 2019-10-29T00:10:31Z

SUMMARY

This PR updates a number of aspects related to the datetime SQL literal logic which is used for the RHS of the time filter comparisons. Specifically:

Make DbEngineSpec.convert_dttm optionally return None if a type conversion does not exist.
Updates the logic in dttm_sql_literal to first try to convert a datetime object to a SQL expression (1) prior to converting either a string on numerical type based on the python-date-format. The reason for this change is we should handle known types before falling back to using the python-date-format, as in theory these could be defined incorrectly for non-string/float types.
Updates the convert_dttm logic for a number of database engines. There were a handful of examples where this was simply returning either i) a string representation of the datetime object rather than the necessary SQL expression for casting from a datetime object to the native temporal type, or ii) incorrect datetime formatting or casting logic. Note I'm not familiar with a number of these database engines and thus the updated conversions were the result of Googling. This logic should probably be validated.
Adds unit tests for the database engine specific convert_dttm function.
Uses f-strings rather than str.format(...).
Replaced dttm.isoformat()[:10] with dttm.date().isoformat() in improve readability.

Regarding timestamp precision some databases only seem to support seconds whereas others support microseconds. Where possible the datetime object is formatted to the highest level precision available.

Note for context I sense this PoC is probably the end state for correctly handling the necessary casting of types (#7682).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

CI and additional unit tests.

ADDITIONAL INFORMATION

Has associated issue: [SIP-15] Transparent and Consistent Time Intervals #6360
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 @dpgaspar @etr2460 @michellethomas @mistercrunch @villebro

john-bodley · 2019-10-29T00:11:29Z

superset/db_engine_specs/bigquery.py

-            return "'{}'".format(dttm.strftime("%Y-%m-%d"))
-        return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
+            return f"CAST('{dttm.isoformat()[:10]}' AS DATE)"
+        if tt == "TIMESTAMP":


@betodealmeida do you know why the TIMESTAMP type wasn't defined for BigQuery?

john-bodley · 2019-10-29T00:12:30Z

superset/db_engine_specs/elasticsearch.py

-        return f"'{dttm.strftime('%Y-%m-%d %H:%M:%S')}'"
+    def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
+        if target_type.upper() in ("DATE", "DATETIME"):
+            return f"CAST('{dttm.isoformat()}' AS TIMESTAMP)"


@dpgaspar I know you recently added this and I was hoping you could validate the logic.

john-bodley · 2019-10-29T00:20:19Z

superset/db_engine_specs/athena.py

        if tt == "TIMESTAMP":
-            return "from_iso8601_timestamp('{}')".format(dttm.isoformat())
-        return "CAST ('{}' AS TIMESTAMP)".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))


We shouldn't be casting other types to TIMESTAMP. The reason for this function is to ensure that the LHS and RHS for the time filter comparison are equivalent types.

john-bodley · 2019-10-29T00:21:08Z

superset/db_engine_specs/base.py

        """
-        return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))


This is now handled in dttm_sql_literal as we need to determine if the python-date-format logic needs to be invoked.

john-bodley · 2019-10-29T00:22:25Z

superset/db_engine_specs/mssql.py

@@ -50,8 +50,10 @@ def epoch_to_dttm(cls):
        return "dateadd(S, {col}, '1970-01-01')"

    @classmethod
-    def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
-        return "CONVERT(DATETIME, '{}', 126)".format(dttm.isoformat())


We shouldn't be casting all types to a DATETIME. The reason for this function is to ensure that the LHS and RHS for the time filter comparison are equivalent types.

john-bodley · 2019-10-29T00:23:02Z

superset/db_engine_specs/oracle.py

-        )
+    def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
+        tt = target_type.upper()
+        if tt == "DATE":


Note there was no previous logic for handling the DATE type.

villebro

A very quick first pass, will look in closer detail later.

superset/db_engine_specs/mssql.py

villebro · 2019-10-29T05:16:14Z

tests/db_engine_specs/mssql_tests.py

+        for target_type in ("DATE", "DATETIME", "SMALLDATETIME", "TIMESTAMP"):
+            self.assertEqual(
+                MssqlEngineSpec.convert_dttm(target_type, dttm),
+                "CONVERT(DATETIME, '2019-01-02T03:04:05.678900', 126)",
+            )


..and the test here.

codecov-io · 2019-10-29T18:40:36Z

Codecov Report

Merging #8464 into master will decrease coverage by 0.04%.
The diff coverage is 42.55%.

@@            Coverage Diff             @@
##           master    #8464      +/-   ##
==========================================
- Coverage   66.57%   66.52%   -0.05%     
==========================================
  Files         449      449              
  Lines       22567    22595      +28     
  Branches     2367     2367              
==========================================
+ Hits        15023    15032       +9     
- Misses       7406     7425      +19     
  Partials      138      138

Impacted Files	Coverage Δ
superset/db_engine_specs/db2.py	`90% <ø> (+4.28%)`	⬆️
superset/db_engine_specs/mssql.py	`48.83% <11.11%> (-9.5%)`	⬇️
superset/db_engine_specs/oracle.py	`66.66% <25%> (-25%)`	⬇️
superset/db_engine_specs/bigquery.py	`42.66% <25%> (-2.41%)`	⬇️
superset/db_engine_specs/presto.py	`22.43% <25%> (ø)`	⬆️
superset/db_engine_specs/hive.py	`27.85% <25%> (ø)`	⬆️
superset/db_engine_specs/impala.py	`59.09% <33.33%> (-5.91%)`	⬇️
superset/db_engine_specs/kylin.py	`57.14% <40%> (+3.29%)`	⬆️
superset/db_engine_specs/clickhouse.py	`62.5% <40%> (+2.5%)`	⬆️
superset/db_engine_specs/elasticsearch.py	`81.25% <40%> (ø)`	⬆️
... and 7 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 5fb0bcb...52ae511. Read the comment docs.

villebro

Second pass

superset/db_engine_specs/oracle.py

superset/db_engine_specs/bigquery.py

villebro · 2019-10-29T19:17:17Z

superset/db_engine_specs/drill.py

-    def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
+    def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
        tt = target_type.upper()
        if tt == "DATE":
-            return "CAST('{}' AS DATE)".format(dttm.isoformat()[:10])
+            return f"TO_DATE('{dttm.date().isoformat()}', 'yyyy-MM-dd')"
        elif tt == "TIMESTAMP":
-            return "CAST('{}' AS TIMESTAMP)".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
-        return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
+            return f"""TO_TIMESTAMP('{dttm.isoformat(sep=" ", timespec="seconds")}', 'yyyy-MM-dd HH:mm:ss')"""  # pylint: disable=line-too-long
+        return None


@cgivre can you check if these are valid and if there's anything missing? A quick googling didn't turn up any DATETIME type for Drill, just DATE, TIME and TIMESTAMP.

villebro · 2019-10-29T19:20:11Z

superset/db_engine_specs/hive.py

        elif tt == "TIMESTAMP":
-            return "CAST('{}' AS TIMESTAMP)".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
-        return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
+            return f"""CAST('{dttm.isoformat(sep=" ", timespec="seconds")}' AS TIMESTAMP)"""  # pylint: disable=line-too-long
+        return None


It seems Hive supports nanosecond precision:
https://cwiki.apache.org/confluence/display/Hive/LanguageManual+Types#LanguageManualTypes-TimestampstimestampTimestamps

superset/db_engine_specs/impala.py

willbarrett · 2019-10-29T21:08:29Z

superset/db_engine_specs/sqlite.py

+    def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
+        if target_type.upper() == "TEXT":
+            return f"""'{dttm.isoformat(sep=" ", timespec="microseconds")}'"""
+        return None


SQLite also supports time stored in REAL and INTEGER columns according to the docs:

TEXT as ISO8601 strings ("YYYY-MM-DD HH:MM:SS.SSS"). REAL as Julian day numbers, the number of days since noon in Greenwich on November 24, 4714 B.C. according to the proleptic Gregorian calendar. INTEGER as Unix Time, the number of seconds since 1970-01-01 00:00:00 UTC.

Should we support the REAL and INTEGER conversions here?

@willbarrett epoch is handled by specifying epoch_s in the python-date-format.

Also a REAL or INTEGER column could encode temporal information in a different form, i.e., one could be using REAL to store a UNIX timestamp with floating point precision though and thus we can’t blindly handle these types.

Note per the referenced PoC PR the future end goal is to provide an engine specific graph which maps between various SQLAlchemy and datetime types. Much of the time grain bucketing is potentially wrong for string columns.

john-bodley · 2019-10-30T00:32:50Z

@villebro @willbarrett thanks for the feedback. I've hopefully addressed all your comments (either via code changes or replies to your comments).

villebro

LGTM, and big thanks for the big effort with the unit tests. I expect something will break as a consequence of this big change, but these are easy to fix later on, so I don't see any reason not to merge this.

john-bodley · 2019-10-30T06:08:59Z

Thanks for the review @villebro. I agree there could be a regression, but i) there were clearly some ill-defined mappings for certain dialects, and ii) with the addition of unit tests after any fixes future regressions should be preventable.

villebro · 2019-10-30T06:20:41Z

Absolutely @john-bodley ; even in the light of possible regressions I expect this to fix more bugs than it introduces. I propose merging this asap.

dpgaspar · 2019-10-30T14:14:30Z

superset/db_engine_specs/elasticsearch.py

+    def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
+        if target_type.upper() == "DATETIME":
+            return f"""CAST('{dttm.isoformat(timespec="seconds")}' AS DATETIME)"""
+        return None


@john-bodley thanks for pointing out to this, the logic looks good and I've retested it and found no problems

pull-request-size bot added the size/L label Oct 29, 2019

john-bodley commented Oct 29, 2019

View reviewed changes

john-bodley force-pushed the john-bodley--fix-convert-dttm branch 2 times, most recently from b65aa7a to 3122919 Compare October 29, 2019 00:19

john-bodley commented Oct 29, 2019

View reviewed changes

john-bodley changed the title ~~[sql] Fixing datetime SQL literal~~ [sql] Fixing datetime conversion and SQL literal Oct 29, 2019

john-bodley force-pushed the john-bodley--fix-convert-dttm branch 2 times, most recently from 827ab4c to 4f1543a Compare October 29, 2019 04:31

john-bodley changed the title ~~[sql] Fixing datetime conversion and SQL literal~~ [SIP-15] Fixing datetime conversion and SQL literal Oct 29, 2019

villebro requested changes Oct 29, 2019

View reviewed changes

john-bodley force-pushed the john-bodley--fix-convert-dttm branch 2 times, most recently from 3ea4968 to fa2d521 Compare October 29, 2019 18:29

pull-request-size bot added size/XL and removed size/L labels Oct 29, 2019

villebro reviewed Oct 29, 2019

View reviewed changes

willbarrett reviewed Oct 29, 2019

View reviewed changes

[sql] Fixing datetime SQL literal

52ae511

john-bodley force-pushed the john-bodley--fix-convert-dttm branch from fa2d521 to 52ae511 Compare October 30, 2019 00:29

villebro approved these changes Oct 30, 2019

View reviewed changes

john-bodley merged commit 0a3b121 into apache:master Oct 30, 2019

john-bodley deleted the john-bodley--fix-convert-dttm branch October 30, 2019 06:26

john-bodley mentioned this pull request Oct 30, 2019

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

Merged

12 tasks

dpgaspar reviewed Oct 30, 2019

View reviewed changes

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

[sql] Fixing datetime SQL literal (#8464)

aff55a3

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] Fixing datetime conversion and SQL literal #8464

[SIP-15] Fixing datetime conversion and SQL literal #8464

john-bodley commented Oct 29, 2019 •

edited

Loading

john-bodley Oct 29, 2019

john-bodley Oct 29, 2019

john-bodley Oct 29, 2019

john-bodley Oct 29, 2019

john-bodley Oct 29, 2019

john-bodley Oct 29, 2019

villebro left a comment

villebro Oct 29, 2019

codecov-io commented Oct 29, 2019 •

edited

Loading

villebro left a comment

villebro Oct 29, 2019

cgivre Oct 30, 2019

villebro Oct 29, 2019

willbarrett Oct 29, 2019

john-bodley Oct 29, 2019 •

edited

Loading

john-bodley commented Oct 30, 2019

villebro left a comment

john-bodley commented Oct 30, 2019

villebro commented Oct 30, 2019

dpgaspar Oct 30, 2019

[SIP-15] Fixing datetime conversion and SQL literal #8464

[SIP-15] Fixing datetime conversion and SQL literal #8464

Conversation

john-bodley commented Oct 29, 2019 • edited Loading

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

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

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 29, 2019 • edited Loading

Codecov Report

villebro 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

john-bodley Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

john-bodley commented Oct 30, 2019

villebro left a comment

Choose a reason for hiding this comment

john-bodley commented Oct 30, 2019

villebro commented Oct 30, 2019

Choose a reason for hiding this comment

john-bodley commented Oct 29, 2019 •

edited

Loading

codecov-io commented Oct 29, 2019 •

edited

Loading

john-bodley Oct 29, 2019 •

edited

Loading