gabotorresruiz commented on code in PR #44500:
URL: https://github.com/apache/superset/pull/44500#discussion_r4066561439
##########
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:
Just a small NIT on the docstring: "the result has millisecond resolution"
holds for the engines that override `epoch_ms_to_dttm` (Druid, Crate,
Couchbase, Ocient, Oracle, Drill, Parseable), but every other engine inherits
the default `epoch_ms_to_dttm`, so the chain ends at `epoch_to_dttm()` fed
`(({col}/1000)/1000)` and the resolution is seconds.
I confirmed it on this branch with a real SQLite dataset on `epoch_us`: the
column renders as `datetime(((ts_us/1000)/1000), 'unixepoch')` and a stored
`1672534800123456` comes back as `2023-01-01 01:00:00`, with the `123456` gone.
`epoch_ms` truncates the same way there today, so nothing regresses, it is only
the docstring that would set the wrong expectation for the next engine author.
Maybe something like "the result inherits whatever resolution that engine's
`epoch_ms_to_dttm` has, which is seconds when the default is inherited".
##########
tests/unit_tests/db_engine_specs/test_base.py:
##########
@@ -1652,3 +1652,79 @@ def
test_base_spec_extended_aggregation_func_defaults_to_unsupported(
def test_base_spec_extended_aggregation_func_unknown_name_is_unsupported() ->
None:
"""An aggregate name outside the known extended set is also just None."""
assert
BaseEngineSpec.get_extended_aggregation_func("NOT_A_REAL_AGGREGATE") is None
+
+
+class _EpochSpec(BaseEngineSpec):
+ """Minimal spec implementing only ``epoch_to_dttm``, like most engines."""
+
+ engine = "epoch"
+ engine_name = "epoch"
+ _time_grain_expressions = {
+ None: "{col}",
+ "P1D": "DATE_TRUNC('day', {col})",
+ }
+
+ @classmethod
+ def epoch_to_dttm(cls) -> str:
+ return "from_unixtime({col})"
+
+
[email protected](
+ "pdf,time_grain,expected",
+ [
+ ("epoch_s", None, "from_unixtime(ts)"),
+ ("epoch_ms", None, "from_unixtime((ts/1000))"),
+ ("epoch_us", None, "from_unixtime(((ts/1000)/1000))"),
+ (
+ "epoch_us",
+ "P1D",
+ "DATE_TRUNC('day', from_unixtime(((ts/1000)/1000)))",
+ ),
+ (None, None, "ts"),
+ ],
+)
+def test_get_timestamp_expr_epoch_formats(
+ pdf: str | None, time_grain: str | None, expected: str
+) -> None:
+ """
+ Every ``epoch_*`` python_date_format routes the raw column through the
+ matching ``epoch*_to_dttm`` template before the time grain is applied; the
+ default ``epoch_us_to_dttm`` reuses the millisecond template.
+ """
+ expr = _EpochSpec.get_timestamp_expr(column("ts"), pdf, time_grain)
+ assert str(expr.compile(compile_kwargs={"literal_binds": True})) ==
expected
+
+
[email protected](
+ "spec_path,expected",
+ [
+ # engines relying on the default: their millisecond template is reused
+ ("athena.AthenaEngineSpec", "from_unixtime((({col}/1000)/1000))"),
+ ("crate.CrateEngineSpec", "({col}/1000)"),
+ ("druid.DruidEngineSpec", "MILLIS_TO_TIMESTAMP(({col}/1000))"),
+ ("couchbase.CouchbaseEngineSpec", "MILLIS_TO_STR(({col}/1000))"),
+ # engines with a native microsecond conversion
+ ("bigquery.BigQueryEngineSpec", "TIMESTAMP_MICROS({col})"),
+ ("snowflake.SnowflakeEngineSpec", "DATEADD(US, {col}, '1970-01-01')"),
+ ("kusto.KustoKqlEngineSpec",
"unixtime_microseconds_todatetime({col})"),
+ (
+ "pinot.PinotEngineSpec",
+ "DATETIMECONVERT({col}, '1:MICROSECONDS:EPOCH', "
+ "'1:MICROSECONDS:EPOCH', '1:MICROSECONDS')",
+ ),
+ ],
+)
+def test_epoch_us_to_dttm(spec_path: str, expected: str) -> None:
+ """
+ ``epoch_us_to_dttm`` yields valid SQL for engines that override
+ ``epoch_ms_to_dttm`` (via the default) and for engines with a native
+ microsecond function (via their own override).
+ """
+ import importlib
Review Comment:
Not a blocker, just a NIT: resolving the specs from dotted strings with
`importlib` inside the test body turns a renamed class or a moved module into a
runtime `AttributeError` / `ModuleNotFoundError` rather than an import failure
at collection, and it is the only dynamic import in this file. Unless there was
an import cost reason, the classes themselves read better:
```python
@pytest.mark.parametrize(
"spec,expected",
[
(AthenaEngineSpec, "from_unixtime((({col}/1000)/1000))"),
(CrateEngineSpec, "({col}/1000)"),
...
(BigQueryEngineSpec, "TIMESTAMP_MICROS({col})"),
],
)
def test_epoch_us_to_dttm(spec: type[BaseEngineSpec], expected: str) -> None:
assert spec.epoch_us_to_dttm() == expected
```
--
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]
