-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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(trino): Fix Trino timestamp conversion #21737
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -17,6 +17,8 @@ | |||
from __future__ import annotations | ||||
|
||||
import logging | ||||
import re | ||||
from datetime import datetime | ||||
from typing import Any, Dict, Optional, Type, TYPE_CHECKING | ||||
|
||||
import simplejson as json | ||||
|
@@ -47,6 +49,29 @@ class TrinoEngineSpec(PrestoBaseEngineSpec): | |||
engine = "trino" | ||||
engine_name = "Trino" | ||||
|
||||
@classmethod | ||||
def convert_dttm( | ||||
cls, target_type: str, dttm: datetime, db_extra: Optional[Dict[str, Any]] = None | ||||
) -> Optional[str]: | ||||
""" | ||||
Convert a Python `datetime` object to a SQL expression. | ||||
:param target_type: The target type of expression | ||||
:param dttm: The datetime object | ||||
:param db_extra: The database extra object | ||||
:return: The SQL expression | ||||
Superset only defines time zone naive `datetime` objects, though this method | ||||
handles both time zone naive and aware conversions. | ||||
""" | ||||
tt = target_type.upper() | ||||
if tt == utils.TemporalType.DATE: | ||||
return f"DATE '{dttm.date().isoformat()}'" | ||||
if re.sub(r"\(\d\)", "", tt) in ( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a reason this needs to be handled differently than the method already defined in PrestoBaseEngineSpec? superset/superset/db_engine_specs/presto.py Line 206 in 08f2c9b
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trino exposes the precision in the temporal types, this is not covered in the Presto code (this is what's the bug was about) |
||||
utils.TemporalType.TIMESTAMP, | ||||
utils.TemporalType.TIMESTAMP_WITH_TIME_ZONE, | ||||
): | ||||
return f"""TIMESTAMP '{dttm.isoformat(timespec="microseconds", sep=" ")}'""" | ||||
return None | ||||
|
||||
@classmethod | ||||
def extra_table_metadata( | ||||
cls, | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,10 +19,12 @@ | |
from unittest.mock import Mock, patch | ||
|
||
import pytest | ||
from sqlalchemy import types | ||
|
||
import superset.config | ||
from superset.constants import USER_AGENT | ||
from superset.db_engine_specs.trino import TrinoEngineSpec | ||
from superset.utils.core import GenericDataType | ||
from tests.integration_tests.db_engine_specs.base_tests import TestDbEngineSpec | ||
|
||
|
||
|
@@ -166,3 +168,31 @@ def test_auth_custom_auth_denied(self): | |
f"For security reason, custom authentication '{auth_method}' " | ||
f"must be listed in 'ALLOWED_EXTRA_AUTHENTICATIONS' config" | ||
) | ||
|
||
def test_convert_dttm(self): | ||
dttm = self.get_dttm() | ||
|
||
self.assertEqual( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||
TrinoEngineSpec.convert_dttm("TIMESTAMP", dttm), | ||
"TIMESTAMP '2019-01-02 03:04:05.678900'", | ||
) | ||
|
||
self.assertEqual( | ||
TrinoEngineSpec.convert_dttm("TIMESTAMP(3)", dttm), | ||
"TIMESTAMP '2019-01-02 03:04:05.678900'", | ||
) | ||
|
||
self.assertEqual( | ||
TrinoEngineSpec.convert_dttm("TIMESTAMP WITH TIME ZONE", dttm), | ||
"TIMESTAMP '2019-01-02 03:04:05.678900'", | ||
) | ||
|
||
self.assertEqual( | ||
TrinoEngineSpec.convert_dttm("TIMESTAMP(3) WITH TIME ZONE", dttm), | ||
"TIMESTAMP '2019-01-02 03:04:05.678900'", | ||
) | ||
|
||
self.assertEqual( | ||
TrinoEngineSpec.convert_dttm("DATE", dttm), | ||
"DATE '2019-01-02'", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that Presto and Trino have diverged in therms of their
TIMESTAMP
functions I wonder if thePrestoBaseEngineSpec.convert_dttm
method should be moved to thePrestoEngineSpec
class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll take care of that.