Repository navigation
fix: preserve timezone-ness when creating predicates - #94
Conversation
gabotechs
left a comment
gabotechs
left a comment
There was a problem hiding this comment.
Left some suggestions around comments and tests, but nothing blocking, so +1 from me. Thanks!
| // | ||
| // Note: for types with TZ, the timestamp value itself is always UTC, | ||
| // even if the timezone is not. |
There was a problem hiding this comment.
nit: maybe something like "Arrow stores tz-aware timestamps as UTC epoch values, the tz string only affects display, so they map straight to Iceberg timestamptz"? Reads a bit clearer to me. Might also be worth splitting this from the TimestampSecond/Millisecond note above, they're kind of unrelated now.
| CREATE TABLE default.default.test_timestamp_seconds (id INT NOT NULL, ts TIMESTAMP(0)) | ||
|
|
||
| statement ok | ||
| SET datafusion.execution.time_zone = 'UTC' |
There was a problem hiding this comment.
Could you add a short comment on why this is needed? I'm guessing it's so ::TIMESTAMPTZ ends up as +00:00. Also it sticks around for anything added to this file later, so maybe reset it at the end next to the DROP.
There was a problem hiding this comment.
It's confusing, but without it the CREATE TABLE ends up with a normal timestamp column, even with TIMESTAMP WITH TIME ZONE specified.
| 2 | ||
|
|
||
| query TT | ||
| EXPLAIN SELECT id FROM default.default.test_timestamptz WHERE ts_us > '2023-01-05T00:00:00+00:00'::TIMESTAMPTZ |
There was a problem hiding this comment.
Can we also cover the mixed cases? e.g. ts_us > CAST('2023-01-05' AS TIMESTAMP) (tz column vs naive literal), and a naive TIMESTAMP column vs a ::TIMESTAMPTZ literal. Once the upstream conversion is gone a mismatched datum would fail to bind and I think the query would just error, so it'd be good to lock in that coercion either gives us a matching literal or skips pushdown.
Given a `ScalarValue` timestamp with a timezone attached, we should create `Datum::timestamptz_*`, instead of a naked `Datum::timestamp`. Note that the latter type doesn't actually store the time zone, only the equivalent UTC stamp.
0570418 to
379eb0d
Compare
|
Thanks @colinmarc! |
Given a
ScalarValuetimestamp with a timezone attached, we should createDatum::timestamptz_*, instead of a nakedDatum::timestamp. Note that the latter type doesn't actually store the time zone, only the equivalent UTC stamp.See apache/iceberg-rust#3574 for context. Specifically,
mainhere is depending on a conversion which would be removed in that PR (see this comment).