rusackas commented on code in PR #43167:
URL: https://github.com/apache/superset/pull/43167#discussion_r3943101734
##########
superset/db_engine_specs/postgres.py:
##########
@@ -267,17 +267,32 @@ def get_timestamp_expr(
time_grain: str | None,
) -> TimestampExpression:
"""
- Construct a timestamp expression while preserving pure ``DATE``
semantics.
+ Construct a timestamp expression for Postgres temporal columns.
Applying ``DATE_TRUNC`` to a ``DATE`` column implicitly casts the
value to
``TIMESTAMP``, which can trigger unwanted timezone conversion on the
client
and shift the displayed date by a day. To avoid this, the truncated
value is
cast back to ``DATE`` when the source column is a pure ``DATE`` type.
+ String columns explicitly marked as temporal are cast to ``TIMESTAMP``
before
+ applying a time grain because Postgres does not implicitly cast
strings for
+ ``DATE_TRUNC`` or ``EXTRACT``.
+
See https://github.com/apache/superset/issues/42254.
+ See https://github.com/apache/superset/issues/42386.
"""
expr = super().get_timestamp_expr(col, pdf, time_grain)
col_type = getattr(col, "type", None)
+ if (
+ time_grain
+ and isinstance(col_type, String)
+ and pdf not in ("epoch_s", "epoch_ms")
+ ):
Review Comment:
I think this one's pre-existing rather than something this PR introduces.
The base spec already emits `event_timestamp * interval '1 second'` for a
string epoch column, so the exclusion here just keeps it from getting wrapped
in a second bad cast. Happy to see it handled in a follow-up, but I wouldn't
block on it.
##########
tests/unit_tests/db_engine_specs/test_postgres.py:
##########
@@ -418,6 +418,40 @@ def test_get_timestamp_expr_datetime_column_not_cast() ->
None:
assert _compile(expr) == "DATE_TRUNC('day', event_ts)"
+def test_get_timestamp_expr_string_column_casts_to_timestamp() -> None:
+ """DB Eng Specs (postgres): temporal string columns are cast before
truncation."""
+ col = column("event_timestamp", type_=types.String())
+ expr = spec.get_timestamp_expr(col, None, "P1D")
+ assert _compile(expr) == "DATE_TRUNC('day', CAST(event_timestamp AS
TIMESTAMP))"
+
+
+def test_get_timestamp_expr_string_column_without_grain_not_cast() -> None:
+ """DB Eng Specs (postgres): strings without a time grain remain
unchanged."""
+ col = column("event_timestamp", type_=types.String())
+ expr = spec.get_timestamp_expr(col, None, None)
+ assert _compile(expr) == "event_timestamp"
+
+
+def test_get_timestamp_expr_epoch_string_column_not_cast() -> None:
+ """DB Eng Specs (postgres): epoch conversion handles strings before
truncation."""
+ col = column("event_timestamp", type_=types.String())
Review Comment:
Agreed, the docstring oversells this a bit. The test is really just pinning
that the new CAST wrapper stays out of the epoch path, so maybe say that
instead.
--
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]
