Skip to content
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

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

Merged
merged 16 commits into from
Apr 20, 2022
17 changes: 14 additions & 3 deletions superset/db_engine_specs/presto.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
Map,
Row,
TinyInteger,
TimeStamp,
Date
)
from superset.result_set import destringify
from superset.sql_parse import ParsedQuery
Expand Down Expand Up @@ -911,10 +913,19 @@ 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_type_by_name = {
column.get("name") : column.get('type') for column in columns or []
}

for col_name, value in zip(col_names, values):
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
col_type = column_type_by_name.get(col_name)

if col_type == 'TIMESTAMP':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

query = query.where(Column(col_name, TimeStamp()) == value)
elif col_type == 'DATE':
query = query.where(Column(col_name, Date()) == value)
else:
query = query.where(Column(col_name) == value)
return query

@classmethod
Expand Down
34 changes: 33 additions & 1 deletion superset/models/sql_types/presto_sql_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
# under the License.
from typing import Any, Dict, List, Optional, Type

from sqlalchemy.sql.sqltypes import Integer
from sqlalchemy import types
from sqlalchemy.sql.sqltypes import Integer, TIMESTAMP, DATE
from sqlalchemy.sql.type_api import TypeEngine
from sqlalchemy.sql.visitors import Visitable

Expand Down Expand Up @@ -91,3 +92,34 @@ def python_type(self) -> Optional[Type[Any]]:
@classmethod
def _compiler_dispatch(cls, _visitor: Visitable, **_kw: Any) -> str:
return "ROW"

class TimeStamp(types.TypeDecorator):
"""
A type to extend functionality of timestamp data type.
"""

impl = TIMESTAMP

@classmethod
def process_bind_param(cls, value, dialect):
"""
Used for in-line rendering of TIMESTAMP data type
as Presto does not support automatic casting.
"""
return "TIMESTAMP '%s'" % value
kekwan marked this conversation as resolved.
Show resolved Hide resolved

class Date(types.TypeDecorator):
"""
A type to extend functionality of date data type.
"""

impl = DATE

@classmethod
def process_bind_param(cls, value, dialect):
"""
Used for in-line rendering of DATE data type
as Presto does not support automatic casting.
"""
return "DATE '%s'" % value
kekwan marked this conversation as resolved.
Show resolved Hide resolved