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

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

Merged
merged 1 commit into from
Oct 30, 2019
+433 −79
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions superset/connectors/sqla/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,19 +220,27 @@ def lookup_obj(lookup_column):

def dttm_sql_literal(self, dttm: DateTime) -> str:
"""Convert datetime object to a SQL expression string"""
sql = (
self.table.database.db_engine_spec.convert_dttm(self.type, dttm)
if self.type
else None
)

if sql:
return sql

tf = self.python_date_format

if tf:
seconds_since_epoch = int(dttm.timestamp())
if tf == "epoch_s":
return str(seconds_since_epoch)
elif tf == "epoch_ms":
if tf in ["epoch_ms", "epoch_s"]:
seconds_since_epoch = int(dttm.timestamp())
if tf == "epoch_s":
return str(seconds_since_epoch)
return str(seconds_since_epoch * 1000)
return "'{}'".format(dttm.strftime(tf))
else:
s = self.table.database.db_engine_spec.convert_dttm(self.type or "", dttm)
return f"'{dttm.strftime(tf)}'"

# TODO(john-bodley): SIP-15 will explicitly require a type conversion.
return s or "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S.%f"))
# TODO(john-bodley): SIP-15 will explicitly require a type conversion.
return f"""'{dttm.strftime("%Y-%m-%d %H:%M:%S.%f")}'"""


class SqlMetric(Model, BaseMetric):
Expand Down
9 changes: 5 additions & 4 deletions superset/db_engine_specs/athena.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from datetime import datetime
from typing import Optional

from superset.db_engine_specs.base import BaseEngineSpec

Expand All @@ -39,13 +40,13 @@ class AthenaEngineSpec(BaseEngineSpec):
}

@classmethod
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 "from_iso8601_date('{}')".format(dttm.isoformat()[:10])
return f"from_iso8601_date('{dttm.date().isoformat()}')"
if tt == "TIMESTAMP":
return "from_iso8601_timestamp('{}')".format(dttm.isoformat())
return "CAST ('{}' AS TIMESTAMP)".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

@classmethod
def epoch_to_dttm(cls) -> str:
Expand Down
12 changes: 6 additions & 6 deletions superset/db_engine_specs/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,15 +439,15 @@ def _allowed_file(filename: str) -> bool:
db.session.commit()

@classmethod
def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
"""
Convert DateTime object to sql expression
Convert Python datetime object to a SQL expression
:param target_type: Target type of expression
:param dttm: DateTime object
:return: SQL expression
:param target_type: The target type of expression
:param dttm: The datetime object
:return: The SQL expression
"""
return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
Copy link
Member Author

Choose a reason for hiding this comment

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

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

return None

@classmethod
def get_all_datasource_names(
Expand Down
12 changes: 8 additions & 4 deletions superset/db_engine_specs/bigquery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import hashlib
import re
from datetime import datetime
from typing import Any, Dict, List, Tuple
from typing import Any, Dict, List, Optional, Tuple

import pandas as pd
from sqlalchemy import literal_column
Expand Down Expand Up @@ -72,11 +72,15 @@ class BigQueryEngineSpec(BaseEngineSpec):
}

@classmethod
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 "'{}'".format(dttm.strftime("%Y-%m-%d"))
return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
return f"CAST('{dttm.date().isoformat()}' AS DATE)"
if tt == "DATETIME":
return f"""CAST('{dttm.isoformat(timespec="microseconds")}' AS DATETIME)"""
if tt == "TIMESTAMP":
Copy link
Member Author

Choose a reason for hiding this comment

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

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

return f"""CAST('{dttm.isoformat(timespec="microseconds")}' AS TIMESTAMP)"""
return None

@classmethod
def fetch_data(cls, cursor, limit: int) -> List[Tuple]:
Expand Down
9 changes: 5 additions & 4 deletions superset/db_engine_specs/clickhouse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from datetime import datetime
from typing import Optional

from superset.db_engine_specs.base import BaseEngineSpec

Expand Down Expand Up @@ -43,10 +44,10 @@ class ClickHouseEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
}

@classmethod
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 "toDate('{}')".format(dttm.strftime("%Y-%m-%d"))
return f"toDate('{dttm.date().isoformat()}')"
if tt == "DATETIME":
return "toDateTime('{}')".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
return f"""toDateTime('{dttm.isoformat(sep=" ", timespec="seconds")}')"""
return None
6 changes: 0 additions & 6 deletions superset/db_engine_specs/db2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from datetime import datetime

from superset.db_engine_specs.base import BaseEngineSpec, LimitMethod


Expand Down Expand Up @@ -51,7 +49,3 @@ class Db2EngineSpec(BaseEngineSpec):
@classmethod
def epoch_to_dttm(cls) -> str:
return "(TIMESTAMP('1970-01-01', '00:00:00') + {col} SECONDS)"

@classmethod
def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
return "'{}'".format(dttm.strftime("%Y-%m-%d-%H.%M.%S"))
9 changes: 5 additions & 4 deletions superset/db_engine_specs/drill.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from datetime import datetime
from typing import Optional
from urllib import parse

from superset.db_engine_specs.base import BaseEngineSpec
Expand Down Expand Up @@ -49,13 +50,13 @@ def epoch_ms_to_dttm(cls) -> str:
return "TO_DATE({col})"

@classmethod
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
Comment on lines -52 to +59
Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do


@classmethod
def adjust_database_uri(cls, uri, selected_schema):
Expand Down
10 changes: 5 additions & 5 deletions superset/db_engine_specs/elasticsearch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# under the License.
# pylint: disable=C,R,W
from datetime import datetime
from typing import Dict
from typing import Dict, Optional

from superset.db_engine_specs.base import BaseEngineSpec

Expand All @@ -41,7 +41,7 @@ class ElasticSearchEngineSpec(BaseEngineSpec):
type_code_map: Dict[int, str] = {} # loaded from get_datatype only if needed

@classmethod
def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
if target_type.upper() in ("DATETIME", "DATE"):
return f"'{dttm.isoformat()}'"
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() == "DATETIME":
return f"""CAST('{dttm.isoformat(timespec="seconds")}' AS DATETIME)"""
return None
Copy link
Member

Choose a reason for hiding this comment

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

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

8 changes: 4 additions & 4 deletions superset/db_engine_specs/hive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,13 @@ def convert_to_hive_type(col_type):
engine.execute(sql)

@classmethod
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"CAST('{dttm.date().isoformat()}' AS DATE)"
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="microseconds")}' AS TIMESTAMP)""" # pylint: disable=line-too-long
return None

@classmethod
def adjust_database_uri(cls, uri, selected_schema=None):
Expand Down
10 changes: 6 additions & 4 deletions superset/db_engine_specs/impala.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from datetime import datetime
from typing import List
from typing import List, Optional

from sqlalchemy.engine.reflection import Inspector

Expand Down Expand Up @@ -43,11 +43,13 @@ def epoch_to_dttm(cls) -> str:
return "from_unixtime({col})"

@classmethod
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 "'{}'".format(dttm.strftime("%Y-%m-%d"))
return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
return f"CAST('{dttm.date().isoformat()}' AS DATE)"
elif tt == "TIMESTAMP":
return f"""CAST('{dttm.isoformat(timespec="microseconds")}' AS TIMESTAMP)"""
return None

@classmethod
def get_schema_names(cls, inspector: Inspector) -> List[str]:
Expand Down
9 changes: 5 additions & 4 deletions superset/db_engine_specs/kylin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from datetime import datetime
from typing import Optional

from superset.db_engine_specs.base import BaseEngineSpec

Expand All @@ -39,10 +40,10 @@ class KylinEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
}

@classmethod
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"CAST('{dttm.date().isoformat()}' AS DATE)"
if 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
11 changes: 9 additions & 2 deletions superset/db_engine_specs/mssql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,15 @@ 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())
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
tt = target_type.upper()
if tt == "DATE":
return f"CONVERT(DATE, '{dttm.date().isoformat()}', 23)"
if tt == "DATETIME":
return f"""CONVERT(DATETIME, '{dttm.isoformat(timespec="milliseconds")}', 126)""" # pylint: disable=line-too-long
if tt == "SMALLDATETIME":
return f"""CONVERT(SMALLDATETIME, '{dttm.isoformat(sep=" ", timespec="seconds")}', 20)""" # pylint: disable=line-too-long
return None

@classmethod
def fetch_data(cls, cursor, limit: int) -> List[Tuple]:
Expand Down
13 changes: 7 additions & 6 deletions superset/db_engine_specs/mysql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,13 @@ class MySQLEngineSpec(BaseEngineSpec):
type_code_map: Dict[int, str] = {} # loaded from get_datatype only if needed

@classmethod
def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
if target_type.upper() in ("DATETIME", "DATE"):
return "STR_TO_DATE('{}', '%Y-%m-%d %H:%i:%s')".format(
dttm.strftime("%Y-%m-%d %H:%M:%S")
)
return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
tt = target_type.upper()
if tt == "DATE":
return f"STR_TO_DATE('{dttm.date().isoformat()}', '%Y-%m-%d')"
if tt == "DATETIME":
return f"""STR_TO_DATE('{dttm.isoformat(sep=" ", timespec="microseconds")}', '%Y-%m-%d %H:%i:%s.%f')""" # pylint: disable=line-too-long
return None

@classmethod
def adjust_database_uri(cls, uri, selected_schema=None):
Expand Down
12 changes: 8 additions & 4 deletions superset/db_engine_specs/oracle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from datetime import datetime
from typing import Optional

from superset.db_engine_specs.base import LimitMethod
from superset.db_engine_specs.postgres import PostgresBaseEngineSpec
Expand All @@ -39,7 +40,10 @@ class OracleEngineSpec(PostgresBaseEngineSpec):
}

@classmethod
def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
return ("""TO_TIMESTAMP('{}', 'YYYY-MM-DD"T"HH24:MI:SS.ff6')""").format(
dttm.isoformat()
)
def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
tt = target_type.upper()
if tt == "DATE":
Copy link
Member Author

Choose a reason for hiding this comment

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

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

return f"TO_DATE('{dttm.date().isoformat()}', 'YYYY-MM-DD')"
if tt == "TIMESTAMP":
return f"""TO_TIMESTAMP('{dttm.isoformat(timespec="microseconds")}', 'YYYY-MM-DD"T"HH24:MI:SS.ff6')""" # pylint: disable=line-too-long
return None
13 changes: 9 additions & 4 deletions superset/db_engine_specs/postgres.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,6 @@ def fetch_data(cls, cursor, limit: int) -> List[Tuple]:
def epoch_to_dttm(cls) -> str:
return "(timestamp 'epoch' + {col} * interval '1 second')"

@classmethod
def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))


class PostgresEngineSpec(PostgresBaseEngineSpec):
engine = "postgresql"
Expand All @@ -73,3 +69,12 @@ def get_table_names(
tables = inspector.get_table_names(schema)
tables.extend(inspector.get_foreign_table_names(schema))
return sorted(tables)

@classmethod
def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
tt = target_type.upper()
if tt == "DATE":
return f"TO_DATE('{dttm.date().isoformat()}', 'YYYY-MM-DD')"
if tt == "TIMESTAMP":
return f"""TO_TIMESTAMP('{dttm.isoformat(sep=" ", timespec="microseconds")}', 'YYYY-MM-DD HH24:MI:SS.US')""" # pylint: disable=line-too-long
return None
8 changes: 4 additions & 4 deletions superset/db_engine_specs/presto.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,13 +520,13 @@ def adjust_database_uri(cls, uri, selected_schema=None):
return uri

@classmethod
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 "from_iso8601_date('{}')".format(dttm.isoformat()[:10])
return f"""from_iso8601_date('{dttm.date().isoformat()}')"""
if tt == "TIMESTAMP":
return "from_iso8601_timestamp('{}')".format(dttm.isoformat())
return "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S"))
return f"""from_iso8601_timestamp('{dttm.isoformat(timespec="microseconds")}')""" # pylint: disable=line-too-long
return None

@classmethod
def epoch_to_dttm(cls) -> str:
Expand Down
9 changes: 4 additions & 5 deletions superset/db_engine_specs/sqlite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,10 @@ def get_all_datasource_names(
raise Exception(f"Unsupported datasource_type: {datasource_type}")

@classmethod
def convert_dttm(cls, target_type: str, dttm: datetime) -> str:
iso = dttm.isoformat().replace("T", " ")
if "." not in iso:
iso += ".000000"
return "'{}'".format(iso)
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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@john-bodley john-bodley Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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.


@classmethod
def get_table_names(
Expand Down
Loading