8000 Proposal: Casting between timestamp and timestamptz should be disallowed in favor of "at time zone" · Issue #16839 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Proposal: Casting between timestamp and timestamptz should be disallowed in favor of "at time zone" #16839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
2 tasks done
jakkes opened this issue Mar 26, 2025 · 10 comments · Fixed by #16899
Closed
2 tasks done

Comments

@jakkes
Copy link
jakkes commented Mar 26, 2025

What happens?

Should the cast operation below really be allowed?

select cast('2020-01-01T00:00:00'::timestamp as timestamptz)

I'm aware that the DuckDB way of working with timezones is definitely not like doing so with pandas/polars etc. I always run

duckdb.sql("set timezone='UTC'")

to avoid issues between systems. However, this cast being allowed seems very frightening. It allows for the following bug:

import duckdb

duckdb.sql("set timezone='UTC'")
data = duckdb.sql("select '2020-01-01T00:00:00+0000'::timestamptz as t")
# convert t to local time zone to extract date or other operations that require local time zone
data2 = data.project("t at time zone 'America/New_York' as t")
# do work with data2
# ...
# for some reason i need timestamptz dtype and i naively convert it back to timestamptz
data3 = data2.project("cast(t as timestamptz) as t")
data3

data3 here will now appear as 2019-12-31 19:00:00+00. Is it possible to disable this cast, and force the use of at time zone ?

To Reproduce

...

OS:

Linux

DuckDB Version:

1.2.2-dev96

DuckDB Client:

Python

Hardware:

No response

Full Name:

Jakob Stigenberg

Affiliation:

Qubos Systematic

What is the latest build you tested with? If possible, we recommend testing with the latest nightly build.

I have tested with a nightly build

Did you include all relevant data sets for reproducing the issue?

Not applicable - the reproduction does not require a data set

Did you include all code required to reproduce the issue?

  • Yes, I have

Did you include all relevant configuration (e.g., CPU architecture, Python version, Linux distribution) to reproduce the issue?

  • Yes, I have
@jakkes jakkes changed the title Scary behavior casting between timestamp and timestamptz Casting between timesta 8000 mp and timestamptz should be banned in favor of "at time zone" Mar 26, 2025
@szarnyasg szarnyasg changed the title Casting between timestamp and timestamptz should be banned in favor of "at time zone" Proposal: Casting between timestamp and timestamptz should be disallowed in favor of "at time zone" Mar 26, 2025
@soerenwolfers
Copy link
Contributor

+1. I previously requested at least an opt out setting at #11570. Using system timezones by default is a giant footgun, especially in times of AWS. Also #12040 for another function that is currently "dangerous" and there isn't even an explicit alternative to specify the timezone.

@hawkfish
Copy link
Contributor

The request in #12040 is interesting and probably pretty easy to do. The discussion in #11570 goes over why this request is difficult. Our current functionality matches Postgres:

hawkfish=# select '2020-01-01T00:00:00+0000'::timestamptz as t;
           t            
------------------------
 2019-12-31 16:00:00-08
(1 row)

@hawkfish
Copy link
Contributor

And PG matches the standard. But I can see how it is not friendly(!)

< 8000 /div>
@hawkfish
Copy link
Contributor

This is not hard for ICU. Do you want it for when ICU is not loaded (it is just a straight copy in that case as the TZ is implicitly UTC

@soerenwolfers
Copy link
Contributor

personally, I don't need it without ICU

@jakkes
Copy link
Author
jakkes commented Mar 27, 2025

What is "ICU" short for?

I have never looked into the DuckDB source code, so I don't know any details here. But I do remember, in previous versions of DuckDB, seeing error messages along the lines of "Cast from XXX to YYY is not implemented", can't you simply remove this cast?

I suppose the problem here is that disabling it at runtime is difficult, vs compile time, and that you want the default configuration to match PG?

@Tishj
Copy link
Contributor
Tishj commented Mar 27, 2025

ICU is a third-party library, I believe this is the link https://icu.unicode.org/, relevant here is https://icu.unicode.org/tznotice

@hawkfish
Copy link
Contributor

ICU is a third-party library, I believe this is the link https://icu.unicode.org/, relevant here is https://icu.unicode.org/tznotice

Yes (International Components for Unicode). We use it for time zone and collation support. It is built in and usually loaded in most environments (but not e.g., WASM).

I was asking because the ICU extension overrides the cast, but when it is not loaded, the default is to just copy the instant, which is like being in UTC. Since the case is not a problem with UTC I was going to leave it alone.

The change is going into the binder (because we can't just remove it and extensions get loaded before you can change the setting) but I could also check whether the current tz is UTC?

hawkfish added a commit to hawkfish/duckdb that referenced this issue Mar 31, 2025
hawkfish added a commit to hawkfish/duckdb that referenced this issue Apr 2, 2025
* PR Feedback: Better message.

fixes: duckdb#16839
hawkfish added a commit to hawkfish/duckdb that referenced this issue Apr 2, 2025
* PR Feedback: Better message test...

fixes: duckdb#16839
hawkfish added a commit to hawkfish/duckdb that referenced this issue Apr 17, 2025
Mytherin added a commit that referenced this issue Apr 22, 2025
* Add a setting to disable timestamp => timestamptz casts in ICU (for
safety)

fixes: #16839
hmeriann pushed a commit to hmeriann/duckdb that referenced this issue Apr 22, 2025
* Add a setting to disable timestamp => timestamptz casts in ICU (for safety)

fixes: duckdb#16839
hmeriann pushed a commit to hmeriann/duckdb that referenced this issue Apr 22, 2025
hmeriann pushed a commit to hmeriann/duckdb that referenced this issue Apr 22, 2025
* PR Feedback: Better message.

fixes: duckdb#16839
hmeriann pushed a commit to hmeriann/duckdb that referenced this issue Apr 22, 2025
* PR Feedback: Better message test...

fixes: duckdb#16839
hmeriann pushed a commit to hmeriann/duckdb that referenced this issue Apr 22, 2025
hmeriann added a commit to hmeriann/duckdb that referenced this issue Apr 22, 2025
* Add a setting to disable timestamp => timestamptz casts in ICU (for safety)

fixes: duckdb#16839
@jakkes
Copy link
Author
jakkes commented Apr 22, 2025

I see this was implemented. Thank you, looking forward to using it!

@jakkes
Copy link
Author
jakkes commented Apr 24, 2025

Ran into this myself now, in an even more frightening way:

>>> duckdb.sql("set timezone='UTC'")
>>> duckdb.sql("select least('2020-01-01T12:00:00+0000'::timestamptz, '2020-01-01T09:00:00')")
┌────────────────────────────────────────────────────────────────────────────────────────────┐
│ least(CAST('2020-01-01T12:00:00+0000' AS TIMESTAMP WITH TIME ZONE), '2020-01-01T09:00:00') │
│                                  timestamp with time zone                                  │
├────────────────────────────────────────────────────────────────────────────────────────────┤
│ 2020-01-01 09:00:00+00                                                                     │
└────────────────────────────────────────────────────────────────────────────────────────────┘

I assume this also disables auto casting.

krlmlr added a commit to duckdb/duckdb-r that referenced this issue May 18, 2025
krlmlr added a commit to duckdb/duckdb-r that referenced this issue May 18, 2025
krlmlr added a commit to duckdb/duckdb-r that referenced this issue May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
0