-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
OF-2500: Stop using text to represent dates or timestamps #3034
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
base: main
Are you sure you want to change the base?
Conversation
creationDate CHAR(15) NOT NULL, | ||
modificationDate CHAR(15) NOT NULL, | ||
creationDate TIMESTAMP NOT NULL, | ||
modificationDate TIMESTAMP NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timezone naive timestamps? This is not the life we want to live.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an alternative that works across all database systems that we support?
630473f
to
78884b0
Compare
Another TODO is illustrated by this comment from
The code as-is does what's adviced against here: vie Timestamp values as an instance of |
|
||
ALTER TABLE ofUser | ||
ALTER COLUMN creationDate SET DATA TYPE TIMESTAMP | ||
USING to_timestamp(CAST(creationDate AS BIGINT) / 1e3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this will round trip correctly and be subject to the environment's timezone setting.
alter table ofuser add tempdt timestamp;
update ofuser set tempdt = to_timestamp(CAST(creationDate AS BIGINT) / 1e3) ;
select creationdate, to_timestamp(CAST(creationDate AS BIGINT) / 1e3), tempdt from ofuser limit 1;
creationdate | to_timestamp | tempdt
-----------------+----------------------------+-------------------------
001324996252502 | 2011-12-27 08:30:52.502-06 | 2011-12-27 08:30:52.502
that tempdt
is a local timezone value and not UTC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using AT TIME ZONE 'UTC'
be more correct?
SELECT
creationdate,
to_timestamp(CAST(creationDate AS BIGINT) / 1000.0) AT TIME ZONE 'UTC'
FROM
ofUser
LIMIT 1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For postgresql, the AT TIME ZONE 'UTC'
results in a naive timestamp result (this actually may be the way...), which may be brittle if it gets jammed back into a time zone aware timestamp type. So likely need to address if we can use timestamptz everywhere, or not first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My current theory is that this SQL can work across our supported databases
(timestamp '1970-01-01 00:00:00 UTC' + cast(COL_TO_CONVERT as bigint) / 1000 * INTERVAL '1 second') at time zone 'UTC'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that dividing by 1e3
or 1000.0
instead of 1000
gives you fractional seconds. It would be nice to be able to retain at least the millisecond granularity.
ALTER COLUMN modificationDate SET DATA TYPE TIMESTAMP | ||
USING to_timestamp(CAST(modificationDate AS BIGINT) / 1e3); | ||
ALTER COLUMN creationDate SET DATA TYPE TIMESTAMP WITH TIME ZONE | ||
USING to_timestamp(CAST(creationDate AS BIGINT) / 1000.0) AT TIME ZONE 'UTC', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't work for me as the resulting timestamp is naive.
select (timestamp '1970-01-01 00:00:00 UTC' + cast(creationDate as bigint) / 1e3 * INTERVAL '1 second') at time zone 'UTC',
to_timestamp(CAST(creationDate AS BIGINT) / 1000.0) AT TIME ZONE 'UTC' from ofuser limit 1;
timezone | timezone
----------------------------+-------------------------
2011-12-27 08:30:52.502-06 | 2011-12-27 14:30:52.502
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to work from my local testing
ALTER TABLE ofUser
ALTER COLUMN creationDate SET DATA TYPE TIMESTAMP WITH TIME ZONE
USING (timestamp '1970-01-01 00:00:00' + cast(creationDate as bigint) / 1000.0 * INTERVAL '1 second') at time zone 'UTC',
ALTER COLUMN modificationDate SET DATA TYPE TIMESTAMP WITH TIME ZONE
USING (timestamp '1970-01-01 00:00:00' + cast(modificationDate as bigint) / 1000.0 * INTERVAL '1 second') at time zone 'UTC';
I have pushed a change that replaces usage of older |
3248cfa
to
f9be336
Compare
This commit contains the required code-changes (but not the database migrations) to use temporal types in the database, rather than converting to and from a zero-padded string.
To help prevent environmental configurations from imposing a timezone context to these values, values are to be written to and obtained from the database using the JDBC `setTimestamp(columnId, Timestamp, Calendar)` methods with a third argument that explicitly defines the UTC-calendar.
Not using the older Java SQL data types but `java.time` ones instead makes life a lot less complex when dealing with time zone oddities. This change requires JDBC 4.2 support in the database driver, and expects the corresponding database columns to be of type `TIMESTAMP WITH TIME ZONE`
f9be336
to
bcded77
Compare
In the database, we occasionally use a text-based column type (eg: varchar) to represent a timestamp. There’s code that converts a Java data into an epoch long, which then gets zero-padded and added to the database.
Every database system will support a timestamp-like datatype. We need to update the database tables in which this happens, migrate existing data, and adjust corresponding code.
See individual commits for details.