builtins: implement to_timestamp for Unix epoch #82523
Conversation
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
This change is |
|
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
|
||
| ## Test for invalid inputs | ||
| statement error to_timestamp\(\): invalid input for type text | ||
| SELECT to_timestamp('invalid') |
There was a problem hiding this comment.
can you add test cases for NaN too
There was a problem hiding this comment.
Added test of to_timestamp() for testing NaN.
pkg/sql/sem/builtins/builtins.go
Outdated
| if ts == math.Inf(-1) { | ||
| return tree.MakeDTimestampTZ(pgdate.TimeNegativeInfinity, time.Microsecond) | ||
| } | ||
| strts := strings.Split(strconv.FormatFloat(ts, 'f', 6, 64), ".") |
There was a problem hiding this comment.
we should not be casting to string to do this.
return tree.MakeDTimestampTZ(timeutil.Unix(0, int64(ts * float64(time.Second)), time.Microsecond)` should do the trick!
There was a problem hiding this comment.
Using tree.MakeDTimestampTZ(timeutil.Unix(0, int64(ts * float64(time.Second)), time.Microsecond) instead of custing to string.
pkg/sql/sem/builtins/builtins.go
Outdated
| if math.IsNaN(ts) { | ||
| return nil, pgerror.New(pgcode.DatetimeFieldOverflow, "timestamp cannot be NaN") | ||
| } | ||
| if ts == math.Inf(0) { |
There was a problem hiding this comment.
this should be Inf(1) - please add a test for pos/neg inf.
There was a problem hiding this comment.
Inf(0) and Inf(1) are the same thing, but just fixed
fixes #77591 Previously, CockroachDB did not support to_timestamp(double precision) In PostgreSQL, the function also handles INT, DECIMAL and text by casting, so the commit also implements those for compatibility. Release note (sql change): Add new function to_timestamp which converts Unix epoch of FLOAT, INT, DECIMAL and text to Timestamp with time zone.
|
Build succeeded: |
|
Will this be released in the next 22.1 version or do we have to wait till 22.2? |
|
22.2 unless there's compelling reason. is there a use case you need it for? |
|
Yes, there is a reason: apache/superset#15825 |
| return floatToTimestampTZ(ts) | ||
| }, | ||
| Info: "Convert Unix epoch (seconds since 1970-01-01 00:00:00+00) to timestamp with time zone.", | ||
| Volatility: volatility.Immutable, |
There was a problem hiding this comment.
I believe this should be stable, not immutable. In Postgres:
marcus=# select proargtypes, prorettype::regtype, provolatile, proleakproof from pg_proc where proname = 'to_timestamp';
proargtypes | prorettype | provolatile | proleakproof
-------------+--------------------------+-------------+--------------
701 | timestamp with time zone | i | f
25 25 | timestamp with time zone | s | f
(2 rows)
marcus=# select 701::oid::regtype, 25::oid::regtype;
regtype | regtype
------------------+---------
double precision | text
(1 row)
There was a problem hiding this comment.
The one arg version is immutable - so this is right right?
| // postgres=# select to_timestamp('1646906263.123456'); | ||
| // to_timestamp | ||
| // ------------------------------- | ||
| // 2022-03-10 09:57:43.123456+00 |
There was a problem hiding this comment.
This PR adds overloads that do not exist in Postgres. The only two overloads in PG are to_timestamp(double_precision) and to_timestamp(text, text).
All of these other examples work because the argument type can be implicitly cast to a FLOAT8, or they are parsed as numeric types, as in to_timestamp('1646906263.123456').
There was a problem hiding this comment.
It is better if we can implicitly cast (or parse) text to numeric on function call. However at least on v22, CockroachDB
does not perform implicit casting, so I added the overload, to_timestamp(text) for compatibility.
Is it much suitable to fix overload.go (or any other files) to parse text as numeric?
| "to_timestamp": makeBuiltin( | ||
| tree.FunctionProperties{Category: categoryDateAndTime}, | ||
| tree.Overload{ | ||
| Types: tree.ArgTypes{{"timestamp", types.String}}, |
There was a problem hiding this comment.
The string overload should take two string arguments. https://www.postgresql.org/docs/current/functions-formatting.html
There was a problem hiding this comment.
would this be the same as experimental_strptime?
There was a problem hiding this comment.
It's similar, but experimental_strptime doesn't support the same formatting strings as to_timestamp. Here's an example from the Postgres docs:
defaultdb> select experimental_strptime('05 Dec 2000', 'DD Mon YYYY');
experimental_strptime
--------------------------
0001-01-01 00:00:00+00
The result should be 2000-12-05 00:00:00-05 (assuming the timezone is set to -05). It looks like experimental_strptime returns a default value when parsing fails, rather than erring.
fixes #77591
Previously, CockroachDB did not support to_timestamp(double precision)
In PostgreSQL, the function also handles INT, DECIMAL and text by casting,
so the commit also implements those for compatibility.
Release note (sql change): Add new function to_timestamp which
converts Unix epoch of FLOAT, INT, DECIMAL and text to Timestamp with time zone.