fix(sql_lab): Add custom timestamp type for literal casting for presto timestamps #13082

kekwan · 2021-02-11T19:28:34Z

SUMMARY

Described in detail in: #13009 but described here again just for clarity.

When previewing a table in SQL Lab, a SELECT * statement is generated based on table data and metadata. This particular bug occur whens previewing tables with TIMESTAMP partition for Presto/Trino data sources. Unlike some other databases, Presto doesn't automatically convert between varchars and other types. In particular, TIMESTAMP needs to be explicitly casted. The simplest way is to use the type constructor TIMESTAMP before the value.

Therefore, where_latest_partition in presto.py needs to cast the TIMESTAMP when it generates the WHERE block in the preview table statement. Accomplish this by creating new SQLAlchemy Type which subclasses TypeDecorator adding additional functionality to existing TIMESTAMP type. For each partition column, I check if the type is TIMESTAMP, and if it is I instantiate the custom TimeStamp class in the Column object which will call process_bind_params when the query compiles.

Invalid syntax (generated by /<int:pk>/table/<table_name>/<schema_name>/ -> get_table_metadata -> select_star -> where_latest_partition)

presto:events> SELECT "event_type" AS "event_type",
            ->        "functional_domain" AS "functional_domain",
            ->        "falcon_instance" AS "falcon_instance",
            ->        "trace_id" AS "trace_id",
            ->        "tenant" AS "tenant",
            ->        "core_tenant" AS "core_tenant",
            ->        "event_timestamp" AS "event_timestamp",
            ->        "table_id" AS "table_id",
            ->        "dtr_workspace_id" AS "dtr_workspace_id",
            ->        "connectors" AS "connectors",
            ->        "table_type" AS "table_type",
            ->        "row_count" AS "row_count",
            ->        "field_count" AS "field_count",
            ->        "partition_count" AS "partition_count",
            ->        "data_size_bytes" AS "data_size_bytes",
            ->        "datetimepartition" AS "datetimepartition",
            ->        "etl_ts" AS "etl_ts",
            ->        "insert_try_number" AS "insert_try_number"
            -> FROM "events"."data_tables"
            -> WHERE "datetimepartition" = '2021-02-11 18:00:00.000'
            ->   AND "etl_ts" = '2021-02-11 18:50:00.000'
            ->   AND "insert_try_number" = 1
            -> LIMIT 100;
Query 20210211_191554_05354_4nnfj failed: line 20:27: Cannot apply operator: timestamp(3) = varchar(23)

Proper syntax

presto:events> SELECT "event_type" AS "event_type",
            ->        "functional_domain" AS "functional_domain",
            ->        "falcon_instance" AS "falcon_instance",
            ->        "trace_id" AS "trace_id",
            ->        "tenant" AS "tenant",
            ->        "core_tenant" AS "core_tenant",
            ->        "event_timestamp" AS "event_timestamp",
            ->        "table_id" AS "table_id",
            ->        "dtr_workspace_id" AS "dtr_workspace_id",
            ->        "connectors" AS "connectors",
            ->        "table_type" AS "table_type",
            ->        "row_count" AS "row_count",
            ->        "field_count" AS "field_count",
            ->        "partition_count" AS "partition_count",
            ->        "data_size_bytes" AS "data_size_bytes",
            ->        "datetimepartition" AS "datetimepartition",
            ->        "etl_ts" AS "etl_ts",
            ->        "insert_try_number" AS "insert_try_number"
            -> FROM "events"."data_tables"
            -> WHERE "datetimepartition" = TIMESTAMP '2021-02-11 18:00:00.000'
            ->   AND "etl_ts" = TIMESTAMP '2021-02-11 18:50:00.000'
            ->   AND "insert_try_number" = 1
            -> LIMIT 100

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

Has associated issue: Incorrect SELECT * statement previewing tables with timestamp partitions #13009
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

codecov-io · 2021-02-11T19:49:03Z

Codecov Report

Merging #13082 (a06038b) into master (2ce7982) will increase coverage by 15.74%.
The diff coverage is 42.85%.

@@             Coverage Diff             @@
##           master   #13082       +/-   ##
===========================================
+ Coverage   53.06%   68.81%   +15.74%     
===========================================
  Files         489      697      +208     
  Lines       17314    38411    +21097     
  Branches     4482        0     -4482     
===========================================
+ Hits         9187    26431    +17244     
- Misses       8127    11980     +3853

Flag	Coverage Δ
cypress	`?`
hive	`44.03% <26.42%> (?)`
mysql	`68.42% <42.85%> (?)`
postgres	`68.45% <42.85%> (?)`
presto	`43.91% <26.42%> (?)`
python	`68.81% <42.85%> (?)`
sqlite	`68.26% <42.85%> (?)`
unit	`39.75% <25.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/examples/birth_names.py	`71.29% <ø> (ø)`
...s/260bf0649a77_migrate_x_dateunit_in_time_range.py	`0.00% <0.00%> (ø)`
...ons/versions/41ce8799acc3_rename_pie_label_type.py	`0.00% <0.00%> (ø)`
superset/utils/core.py	`89.83% <52.00%> (ø)`
superset/views/core.py	`77.02% <60.00%> (ø)`
superset/utils/date_parser.py	`96.63% <75.00%> (ø)`
superset/db_engine_specs/presto.py	`88.70% <80.00%> (ø)`
superset/models/sql_types/presto_sql_types.py	`80.39% <84.61%> (ø)`
superset/charts/commands/exceptions.py	`91.48% <87.50%> (ø)`
superset/db_engine_specs/elasticsearch.py	`92.85% <94.11%> (ø)`
... and 1187 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 ad380fd...a06038b. Read the comment docs.

ktmud · 2021-02-11T20:19:49Z

superset/models/sql_types/presto_sql_types.py

+        Used for in-line rendering of TIMESTAMP data type
+        as Presto does not support automatic casting.
+        """
+        return "TIMESTAMP '%s'" % value


nit: missing EOF (CI will fail)

etr2460 · 2021-02-11T21:53:11Z

@john-bodley this looks like something you'd be very interested in

john-bodley · 2021-02-12T00:11:51Z

superset/db_engine_specs/presto.py

@@ -911,10 +912,14 @@ def where_latest_partition(  # pylint: disable=too-many-arguments
        if values is None:
            return None

-        column_names = {column.get("name") for column in columns or []}
+        column_names = {column.get("name") : column.get('type') for column in columns or []}


Maybe this should be column_type_by_name and thus line 918 will be easier to grok. In that case you could roll lines 918 and 919 into one.

john-bodley · 2021-02-12T00:14:37Z

superset/db_engine_specs/presto.py

        for col_name, value in zip(col_names, values):
            if col_name in column_names:
-                query = query.where(Column(col_name) == value)
+                col_type = column_names.get(col_name)
+                if col_type == 'TIMESTAMP':


I agree the existing logic is wrong but I wonder if SQLAlchemy has a mechanism for auto-casting between the type of value and the column type for the comparison.

Yeah, that would be ideal solution or getting Presto/Trino to support auto-casting.

I've looked into it a bit and tried a few things but couldn't get any auto-casting. I've tried to pass in the default SQLAlchemy data types e.g. types.TIMESTAMP to the Column object as well as using a literal clause but the default data types doesn't seem to know how to auto-cast the type of value. I'm guessing because each DB dialect casts types differently so its up to the user to cast it yourself using one of these approaches to apply SQL-level binding

I have only done this casting for 2 Trino data types TIMESTAMP and DATE in this PR since these are the most popular partition column data types for our use case. But all of these data types besides the basic string, integer, boolean types require explicit casting so this is still an issue for other data types.

john-bodley · 2021-02-12T19:14:18Z

superset/db_engine_specs/presto.py

-            if col_name in column_names:
-                query = query.where(Column(col_name) == value)
+            if col_name in column_type_by_name:
+                col_type = column_type_by_name.get(col_name)


Suggested change

col_type = column_type_by_name.get(col_name)

john-bodley · 2021-02-12T19:14:29Z

superset/db_engine_specs/presto.py

-                query = query.where(Column(col_name) == value)
+            if col_name in column_type_by_name:
+                col_type = column_type_by_name.get(col_name)
+                if col_type == 'TIMESTAMP':


Suggested change

if col_type == 'TIMESTAMP':

if column_type_by_name.get(col_name) == 'TIMESTAMP':

john-bodley · 2021-02-17T01:30:48Z

@kekwan you'll need to fix a couple of linting issues.

kekwan · 2021-02-17T03:18:21Z

thanks @john-bodley fixed the linting issues now 😅

kekwan · 2021-02-24T19:58:59Z

@john-bodley can i get another look at this when u get a chance? thanks!

superset/models/sql_types/presto_sql_types.py

Co-authored-by: John Bodley <[email protected]>

villebro · 2021-02-25T08:25:34Z

superset/db_engine_specs/presto.py

-            if col_name in column_names:
-                query = query.where(Column(col_name) == value)
+            if col_name in column_type_by_name:
+                if column_type_by_name.get(col_name) == "TIMESTAMP":
+                    query = query.where(Column(col_name, TimeStamp()) == value)
+                elif column_type_by_name.get(col_name) == "DATE":
+                    query = query.where(Column(col_name, Date()) == value)
+                else:
+                    query = query.where(Column(col_name) == value)


I'm wondering if we couldn't just use the existing convert_dttm here, as we should already have type casting logic in place for these types of conversions?

superset/superset/db_engine_specs/presto.py

Lines 597 to 604 in 8ab45c9

@classmethod

def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:

tt = target_type.upper()

if tt == utils.TemporalType.DATE:

return f"""from_iso8601_date('{dttm.date().isoformat()}')"""

if tt == utils.TemporalType.TIMESTAMP:

return f"""from_iso8601_timestamp('{dttm.isoformat(timespec="microseconds")}')""" # pylint: disable=line-too-long

return None

Something like query = query.where(Column(col_name) == cls.convert_dttm(column_type_by_name.get(col_name), value) (not tested! thinking out loud)

Certainly possible but the drawbacks I see to re-using this existing method is that:

We have to explicitly cast the string value to a python datetime to call the method only to get it converted back to a string with .isoformat.

from_iso8601_x doesn't seem to be a recommended function anymore in the latest versions of Presto/Trino https://trino.io/docs/current/language/types.html#date-and-time

This method only works if your date/timestamps are in ISO 8601 format.

In the future, if more Trino types are to be supported in previewing data with partitions, we will have to create new types like this anyways. Also, I think this is more the sqlalchemy friendly way.

andreas-eberle · 2021-04-06T08:28:26Z

What is the status of this PR? This makes it impossible to work with timeseries data on Trino. :(

stale · 2021-06-11T01:48:25Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

junlincc · 2021-10-19T20:46:33Z

@villebro what do we need to do to push this PR forward?

stale · 2022-04-16T14:56:18Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

…o timestamps (apache#13082) * Add custom timestamp type for literal casting for presto timestamps * Remove typo in comment * Use process_bind_params as in sqla docs * Uncommit local superset config * Add DATE literal casting * Fix lint errors and change var name * Get rid of col_type and whitespace * Fix linting * Fix arg type * Fix isort lint error * ran black and isort locally.. * accidentally removed EOF * Dont need eof * Use newer string formatting style from comments Co-authored-by: John Bodley <[email protected]> * Trigger notification * Trigger notification Co-authored-by: Kenny Kwan <[email protected]> Co-authored-by: John Bodley <[email protected]>

Kenny Kwan added 3 commits February 11, 2021 10:58

Add custom timestamp type for literal casting for presto timestamps

d0d9424

Remove typo in comment

30dc769

Use process_bind_params as in sqla docs

72237b3

pull-request-size bot added the size/M label Feb 11, 2021

Uncommit local superset config

8b41dba

pull-request-size bot added size/S and removed size/M labels Feb 11, 2021

kekwan mentioned this pull request Feb 11, 2021

Incorrect SELECT * statement previewing tables with timestamp partitions #13009

Closed

3 tasks

ktmud reviewed Feb 11, 2021

View reviewed changes

Add DATE literal casting

593b821

pull-request-size bot added size/M and removed size/S labels Feb 12, 2021

john-bodley reviewed Feb 12, 2021

View reviewed changes

junlincc added the new:contributor The author is a new contributor label Feb 12, 2021

Fix lint errors and change var name

91fc846

john-bodley reviewed Feb 12, 2021

View reviewed changes

Get rid of col_type and whitespace

626a723

kekwan requested a review from john-bodley February 16, 2021 17:13

Kenny Kwan added 4 commits February 16, 2021 17:48

Fix linting

a7f0940

Fix arg type

c206354

Fix isort lint error

2feb219

ran black and isort locally..

862478e

Kenny Kwan added 2 commits February 16, 2021 19:19

accidentally removed EOF

7d1e104

Dont need eof

6c69f95

john-bodley reviewed Feb 25, 2021

View reviewed changes

superset/models/sql_types/presto_sql_types.py Outdated Show resolved Hide resolved

superset/models/sql_types/presto_sql_types.py Outdated Show resolved Hide resolved

Use newer string formatting style from comments

1a0614f

Co-authored-by: John Bodley <[email protected]>

john-bodley approved these changes Feb 25, 2021

View reviewed changes

villebro reviewed Feb 25, 2021

View reviewed changes

stale bot added the inactive Inactive for >= 30 days label Jun 11, 2021

stale bot removed the inactive Inactive for >= 30 days label Oct 19, 2021

stale bot added the inactive Inactive for >= 30 days label Apr 16, 2022

villebro approved these changes Apr 20, 2022

View reviewed changes

stale bot removed the inactive Inactive for >= 30 days label Apr 20, 2022

john-bodley added 2 commits April 21, 2022 05:39

Trigger notification

5dee4c7

Trigger notification

a06038b

john-bodley merged commit 1b55778 into apache:master Apr 20, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql_lab): Add custom timestamp type for literal casting for presto timestamps #13082

fix(sql_lab): Add custom timestamp type for literal casting for presto timestamps #13082

kekwan commented Feb 11, 2021 •

edited

Loading

codecov-io commented Feb 11, 2021 •

edited by codecov bot

Loading

ktmud Feb 11, 2021

etr2460 commented Feb 11, 2021

john-bodley Feb 12, 2021

john-bodley Feb 12, 2021

kekwan Feb 12, 2021

john-bodley Feb 12, 2021

john-bodley Feb 12, 2021

john-bodley commented Feb 17, 2021

kekwan commented Feb 17, 2021

kekwan commented Feb 24, 2021

villebro Feb 25, 2021

kekwan Feb 25, 2021

andreas-eberle commented Apr 6, 2021

stale bot commented Jun 11, 2021

junlincc commented Oct 19, 2021

stale bot commented Apr 16, 2022

	if col_type == 'TIMESTAMP':
	if column_type_by_name.get(col_name) == 'TIMESTAMP':

	@classmethod
	def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
	tt = target_type.upper()
	if tt == utils.TemporalType.DATE:
	return f"""from_iso8601_date('{dttm.date().isoformat()}')"""
	if tt == utils.TemporalType.TIMESTAMP:
	return f"""from_iso8601_timestamp('{dttm.isoformat(timespec="microseconds")}')""" # pylint: disable=line-too-long
	return None

fix(sql_lab): Add custom timestamp type for literal casting for presto timestamps #13082

fix(sql_lab): Add custom timestamp type for literal casting for presto timestamps #13082

Conversation

kekwan commented Feb 11, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Feb 11, 2021 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

etr2460 commented Feb 11, 2021

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 Feb 17, 2021

kekwan commented Feb 17, 2021

kekwan commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreas-eberle commented Apr 6, 2021

stale bot commented Jun 11, 2021

junlincc commented Oct 19, 2021

stale bot commented Apr 16, 2022

kekwan commented Feb 11, 2021 •

edited

Loading

codecov-io commented Feb 11, 2021 •

edited by codecov bot

Loading