fix: convert_timezone must use to_local_time(), not the Postgres pattern - #1
Conversation
The double-AT TIME ZONE + cast-to-naive-timestamp pattern silently loses the shift on DataFusion: the final cast re-renders the UTC instant rather than the target zone's wall clock. to_local_time() yields the local wall clock and is DST-aware. Verified live: 2024-01-01 12:00 UTC -> 04:00 (PST) and 2024-07-01 12:00 UTC -> 05:00 (PDT). Found by review on hotdata-dev/jaffle-shop#7.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| to_local_time( | ||
| (cast({{ column }} as timestamp) at time zone '{{ source_tz }}') | ||
| at time zone '{{ target_tz }}' | ||
| ) |
There was a problem hiding this comment.
source_tz is interpolated but has no effect, so any non-UTC source silently returns the wrong wall clock — the same failure mode this PR is fixing.
The PR description's own finding establishes it: cast(x as timestamp) at time zone src at time zone tgt cast back to naive returns the original value. That round-trip is only lossless if AT TIME ZONE on DataFusion is metadata-only (attach/relabel the display zone, UTC instant unchanged) rather than Postgres-style localization. Given that, ... at time zone '{{ source_tz }}' is a pure no-op and the expression reduces to to_local_time(cast({{ column }} as timestamp) at time zone '{{ target_tz }}') — the input is always treated as UTC.
Concretely, convert_timezone(ts, target_tz='UTC', source_tz='America/New_York') on 2024-01-01 12:00 should yield 17:00; this returns 12:00, with no error. The verification in the description only covers source_tz='UTC', which is why it passed.
Two options, either is fine:
- Reject what isn't supported, matching the
datediffpattern already in this file:
{%- set source_tz = source_tz or 'UTC' -%}
{%- if source_tz | upper != 'UTC' -%}
{% do exceptions.raise_compiler_error("convert_timezone source_tz '" ~ source_tz ~ "' is not supported on Hotdata; only UTC sources can be converted") %}
{%- endif -%}- Actually apply the source offset, if the engine has a
from_local_time-style inverse.
Note the source_tz or 'UTC' default is worth keeping either way: dbt_date can dispatch here with source_tz=None, which currently renders at time zone 'None'.
There was a problem hiding this comment.
Tested the premise before choosing an option — source_tz does work on this engine. AT TIME ZONE on a naive timestamp localizes it as the source zone's wall clock (Postgres-style), not a metadata relabel:
cast('2024-01-01 12:00:00' as timestamp) at time zone 'America/New_York'→2024-01-01T12:00:00-05:00- NY 12:00 → UTC target: 17:00 ✓ (your concrete case)
- NY 12:00 → LA target: 09:00 ✓
The round-trip observation that motivated the inference has a different explanation: the loss was in the cast-to-naive rendering step (it renders the UTC instant), not in AT TIME ZONE — so the round trip is lossless even though the conversion is real.
Applied the part that stands: source_tz or 'UTC' default (a direct dispatch with source_tz=None did render at time zone 'None'), plus a comment documenting the verified source semantics, plus the changelog entry from the nit. Verified end-to-end through the macro via dbt show: NY source → 17:00, None source → UTC default → 04:00 LA.
| config to reach it). NOT the Postgres double-AT TIME ZONE pattern: on | ||
| DataFusion the final cast back to a naive timestamp re-renders the UTC | ||
| instant, silently losing the shift. to_local_time() yields the target | ||
| zone's wall clock (DST-aware). |
There was a problem hiding this comment.
nit: this fixes user-visible behavior shipped in 0.1.0, but CHANGELOG.md has an empty ## [Unreleased] section — worth a ### Fixed entry there, since the changelog already advertises convert_timezone as a supported cross-database macro. (not blocking)
There was a problem hiding this comment.
Review
Blocking Issues
dbt/include/hotdata/macros/utils.sql:23-26—source_tzis interpolated into the SQL but has no effect. The PR's own finding (the doubleAT TIME ZONE+ cast-to-naive round-trip returns the original value) impliesAT TIME ZONEis metadata-only on DataFusion, soat time zone '{{ source_tz }}'is a no-op and the input is always treated as UTC.convert_timezone(ts, 'UTC', 'America/New_York')on2024-01-01 12:00returns12:00instead of17:00, silently — the same failure mode this PR fixes for the target side. The live verification only exercisedsource_tz='UTC'.
Action Required
Handle non-UTC source_tz explicitly: either raise a compiler error for it (mirroring the datediff unsupported-datepart branch already in this file) or apply the source offset for real. Also default a falsy source_tz to 'UTC' — dbt_date can dispatch with source_tz=None, which currently renders at time zone 'None'.
source_tz itself works — verified live that AT TIME ZONE on a naive timestamp localizes it as the source zone's wall clock (12:00 America/New_York -> 17:00 UTC, -> 09:00 LA). The earlier round-trip loss was in the cast-to-naive rendering step, not AT TIME ZONE.
There was a problem hiding this comment.
Prior blocking finding on source_tz is withdrawn: my inference was wrong. DataFusion AT TIME ZONE is asymmetric — on a naive timestamp it localizes (wall clock preserved, instant shifted; see the to_local_time docs example where '2024-04-01T00:00:20Z'::timestamp AT TIME ZONE 'Europe/Brussels' renders as 2024-04-01T00:00:20+02:00), and it is the tz-aware→naive cast(... as timestamp) that re-renders the UTC instant. So the old code failing does not imply the source-side AT TIME ZONE is a no-op; the source offset is applied and non-UTC sources convert correctly. The source_tz or 'UTC' default and the changelog entry cover the rest.
1 participant
The double-
AT TIME ZONE+ cast-to-naive-timestamp pattern silently loses the shift on DataFusion: the final cast re-renders the UTC instant rather than the target zone's wall clock.to_local_time()yields the local wall clock and is DST-aware.Verified live:
2024-01-01 12:00 UTC → 04:00(PST) and2024-07-01 12:00 UTC → 05:00(PDT); full jaffle-shopdbt buildgreen with the fix.Found by review on hotdata-dev/jaffle-shop#7.