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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 16 commits into from
Apr 20, 2022
Merged

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

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d0d9424
Add custom timestamp type for literal casting for presto timestamps
Feb 11, 2021
30dc769
Remove typo in comment
Feb 11, 2021
72237b3
Use process_bind_params as in sqla docs
Feb 11, 2021
8b41dba
Uncommit local superset config
Feb 11, 2021
593b821
Add DATE literal casting
Feb 12, 2021
91fc846
Fix lint errors and change var name
Feb 12, 2021
626a723
Get rid of col_type and whitespace
Feb 12, 2021
a7f0940
Fix linting
Feb 17, 2021
c206354
Fix arg type
Feb 17, 2021
2feb219
Fix isort lint error
Feb 17, 2021
862478e
ran black and isort locally..
Feb 17, 2021
7d1e104
accidentally removed EOF
Feb 17, 2021
6c69f95
Dont need eof
Feb 17, 2021
1a0614f
Use newer string formatting style from comments
kekwan Feb 25, 2021
5dee4c7
Trigger notification
john-bodley Apr 20, 2022
a06038b
Trigger notification
john-bodley Apr 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 13 additions & 3 deletions superset/db_engine_specs/presto.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,11 @@
from superset.models.sql_lab import Query
from superset.models.sql_types.presto_sql_types import (
Array,
Date,
Interval,
Map,
Row,
TimeStamp,
TinyInteger,
)
from superset.result_set import destringify
Expand Down Expand Up @@ -911,10 +913,18 @@ 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:
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)
Comment on lines -916 to +927
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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
  3. This method only works if your date/timestamps are in ISO 8601 format.
  4. 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.

return query

@classmethod
Expand Down
38 changes: 37 additions & 1 deletion superset/models/sql_types/presto_sql_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,15 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

# pylint: disable=abstract-method
from typing import Any, Dict, List, Optional, Type

from sqlalchemy.sql.sqltypes import Integer
from sqlalchemy.engine.interfaces import Dialect
from sqlalchemy.sql.sqltypes import DATE, Integer, TIMESTAMP
from sqlalchemy.sql.type_api import TypeEngine
from sqlalchemy.sql.visitors import Visitable
from sqlalchemy.types import TypeDecorator

# _compiler_dispatch is defined to help with type compilation

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


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

impl = TIMESTAMP

@classmethod
def process_bind_param(cls, value: str, dialect: Dialect) -> str:
"""
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(TypeDecorator):
"""
A type to extend functionality of date data type.
"""

impl = DATE

@classmethod
def process_bind_param(cls, value: str, dialect: Dialect) -> str:
"""
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