seunggabi commented on code in PR #44500:
URL: https://github.com/apache/superset/pull/44500#discussion_r4067892530
##########
superset/utils/core.py:
##########
@@ -2046,7 +2047,7 @@ def _process_datetime_column(
col: DateColumn,
) -> None:
"""Process a single datetime column with format detection."""
- if col.timestamp_format in ("epoch_s", "epoch_ms"):
+ if col.timestamp_format in EPOCH_FORMATS:
Review Comment:
The pandas unit is already derived from the format on the lines right below
this check: `unit = col.timestamp_format.replace("epoch_", "")`, so `epoch_us`
resolves to `unit="us"`. That line was on `master` before this PR; only the
membership test changed. `test_normalize_dttm_col_epoch_microseconds` in
`tests/unit_tests/utils/test_core.py` asserts `1577836800000000` → `2020-01-01`.
##########
superset/connectors/sqla/models.py:
##########
@@ -1295,7 +1296,7 @@ def get_timestamp_expression( # noqa: C901
label = label or utils.DTTM_ALIAS
pdf = self.python_date_format
- is_epoch = pdf in ("epoch_s", "epoch_ms")
+ is_epoch = pdf in EPOCH_FORMATS
Review Comment:
This is the existing contract for `epoch_s` / `epoch_ms`: a column tagged
with an epoch `python_date_format` is routed through epoch arithmetic
regardless of its SQL type. `epoch_us` follows the same rule, so a text column
mislabelled as `epoch_us` fails the same way a text column mislabelled as
`epoch_ms` already does. That is a dataset configuration error rather than
something introduced here.
##########
superset/db_engine_specs/base.py:
##########
@@ -1529,6 +1532,16 @@ def epoch_ms_to_dttm(cls) -> str:
"""
return cls.epoch_to_dttm().replace("{col}", "({col}/1000)")
+ @classmethod
+ def epoch_us_to_dttm(cls) -> str:
+ """
+ SQL expression that converts epoch (microseconds) to datetime that can
be used
+ in a query.
+
+ :return: SQL Expression
+ """
+ return cls.epoch_to_dttm().replace("{col}", "({col}/1000000)")
Review Comment:
BigQuery does not inherit the default: `BigQueryEngineSpec.epoch_us_to_dttm`
returns `TIMESTAMP_MICROS({col})` (see `superset/db_engine_specs/bigquery.py`),
and `test_epoch_us_to_dttm` in `tests/unit_tests/db_engine_specs/test_base.py`
asserts exactly that.
##########
superset/utils/core.py:
##########
@@ -2046,7 +2047,7 @@ def _process_datetime_column(
col: DateColumn,
) -> None:
"""Process a single datetime column with format detection."""
- if col.timestamp_format in ("epoch_s", "epoch_ms"):
+ if col.timestamp_format in EPOCH_FORMATS:
Review Comment:
Pre-existing behaviour shared by all epoch formats: the numeric branch is
keyed on the pandas dtype, and non-numeric series go through `pd.Timestamp` for
`epoch_s` and `epoch_ms` today as well. This PR keeps the three formats
consistent; changing how string-typed epoch columns are handled would be a
separate change.
##########
superset/models/helpers.py:
##########
@@ -4194,9 +4199,7 @@ def dttm_sql_literal(self, dttm: datetime, col:
"TableColumn") -> str:
dttm_tz_aware = dttm_tz_aware.replace(tzinfo=timezone.utc)
seconds_since_epoch = int(dttm_tz_aware.timestamp())
- if tf == "epoch_s":
- return str(seconds_since_epoch)
- return str(seconds_since_epoch * 1000)
+ return str(seconds_since_epoch * EPOCH_FORMATS[tf])
Review Comment:
Intentional. `dttm_sql_literal` emits the filter literal in the same unit
the column is stored in, so the time-range comparison is like-for-like: an
`epoch_us` column holds microseconds, hence `seconds * 1_000_000`; `epoch_ms`
still gets `* 1_000` and `epoch_s` `* 1`. Covered by `test_dttm_sql_literal` in
`tests/unit_tests/models/core_test.py`.
##########
superset/db_engine_specs/base.py:
##########
@@ -1529,6 +1536,21 @@ def epoch_ms_to_dttm(cls) -> str:
"""
return cls.epoch_to_dttm().replace("{col}", "({col}/1000)")
+ @classmethod
+ def epoch_us_to_dttm(cls) -> str:
+ """
+ SQL expression that converts epoch (microseconds) to datetime that can
be used
+ in a query.
+
+ The default routes through ``epoch_ms_to_dttm`` so engines that already
+ override the millisecond conversion keep their validated SQL; the
result
+ has millisecond resolution. Engines with a native microsecond function
Review Comment:
Good catch, and thanks for verifying on SQLite. Reworded the docstring with
your phrasing in 5dbc016.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
