sql: implement datetime builtins

[annrpom]

Previously, make_date, make_timestamp, and make_timestamptz
built-ins were not implemented. In addition, a new signature for
date_trunc was not implemented. This was inadequate because it caused
an incompatibility issue for tools that needed these datetime functions.
To address this, this patch adds said datetime built-ins. Note that behaviors
do not align with postgres when negative years are inpits, as we adhere
to ISO 8601 standard.

Epic: none
Fixes: #108448

Release note (sql change): Datetime built-ins (make_date,
make_timestamp, and make_timestamptz) are implemented - allowing for
the creation of timestamps, timestamps with time zones, and dates. In
addition, date_trunc now allows for a timestamp to be truncated in a
provided timezone (to a provided precision).

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[annrpom]

Kindly ignore this draft PR (edit: for now) - I ideally would like to:

1. change the test output formatting so it matches what the user sees (this comment was left before i realized that the fix was to just make_date(foo)::string)
2. I need to fix the wrong timezone displaying

[annrpom]

is using timeutil.TimeZoneStringToLocationPOSIXStandard alongside timeutil.TimeZoneStringToLocation not the move here? i would think that the result here should be 2013-07-15 07:15:23.5 -0400 EST

[chrisseto]

Just a couple nits and questions. I don't yet consider my LGTM to be authoritative but LGTM. I'll wait for someone else to come through and fully approve it!

[annrpom]

[note to self]
Currently, the code for formatting dates/timestamps when given a negative year results in a date like -2013-01-12 to be printed as 2014-01-12 BC. For the make_* built-ins, this does not match with the behavior of Postgres. Ex.:

postgres=# SELECT make_date(-2013, 1, 12);
   make_date   
---------------
 2013-01-12 BC
(1 row)

As to what changing this formatting would affect, I have this draft PR up with CI looking like this.

[rafiss]

looking good! just have some nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @chrisseto)

---

pkg/sql/sem/builtins/builtins.go line 2522 at r4 (raw file):

	"make_timestamp": makeBuiltin(
		tree.FunctionProperties{Category: builtinconstants.CategoryDateAndTime},
		makeTimestampStatementBuiltinOverload(false, false),

nit: it's good practice to document the parameter names if we pass in constant literals like this:

makeTimestampStatementBuiltinOverload(false /* withTZ */, false /* withInputTZ */),

even better is to avoid having functions with boolean parameters - but that's more important for heavily used functions used in many places. since this one is just a helper used in these two places, i think the boolean parameters are fine.

---

pkg/sql/sem/builtins/builtins.go line 11349 at r4 (raw file):

}

func makeTimestampStatementBuiltinOverload(withTZ bool, withInputTZ bool) tree.Overload {

nit: i think withTZ could be named withOutputTZ to make this more clear

---

pkg/sql/sem/builtins/builtins.go line 11352 at r4 (raw file):

	// If we are not creating a timestamp with a timezone, we shouldn't expect an input timezone
	if !withTZ {
		withInputTZ = false

since this function is only called during initialization time, i think we actually should panic here - if it is called in the wrong way it is a programming error that someone should fix.

---

pkg/sql/sem/builtins/builtins.go line 11354 at r4 (raw file):

		withInputTZ = false
	}
	info := "Create timestamp"

nit: there should be a space after "timestamp".

---

pkg/sql/logictest/testdata/logic_test/datetime line 2088 at r1 (raw file):

Previously, annrpom (annie pompa) wrote…

is using timeutil.TimeZoneStringToLocationPOSIXStandard alongside timeutil.TimeZoneStringToLocation not the move here? i would think that the result here should be 2013-07-15 07:15:23.5 -0400 EST

the result here seems consistent with the postgres result (i.e., -5 for the timezone)

---

pkg/sql/logictest/testdata/logic_test/datetime line 2032 at r4 (raw file):

-2013-07-15 00:00:00 +0000 +0000

statement error pq: make_date\(\): date field value out of range: 0-11-11

nit: use statement error pgcode XXXXX so we can test what the error code is too

---

pkg/sql/logictest/testdata/logic_test/datetime line 2095 at r4 (raw file):

statement error pq: make_timestamptz\(\): date field value out of range: 0-07-15
SELECT make_timestamptz(0, 7, 15, 8, 15, 23.5, 'America/New_York');

nit: could you add tests for make_timestamptz and date_trunc that pass in an invalid timezone name?

[annrpom]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @rafiss)

---

pkg/sql/sem/builtins/builtins.go line 2509 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

to see which code PG uses, you can add this to your ~/.psqlrc file:

-- Show error codes and location.
\set VERBOSITY verbose

Then if you connect to postgres using psql, any error it shows will also show you the pgcode

ack - ty

---

pkg/sql/sem/builtins/builtins.go line 11397 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

you can use a query like this in PG to see what the volatility should be:

select pg_get_function_arg_default(p.oid, 1), proname, pronamespace, n.nspname, proargtypes, prorettype, provolatile, proleakproof from pg_proc p join pg_namespace n on p.pronamespace = n.oid where proname ilike 'make_%';

seems like only make_timestamptz is stable, and the other make_* functions are immutable.

date_trunc(.., timestamptz) is stable, while the other versions are immutable.

(Check out the comment on the Volatility type if you want to see what this means)

ah! thank you - i was going based off of my best guess from the description of the Volatility types T_T

---

pkg/sql/sem/builtins/builtins.go line 11352 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

since this function is only called during initialization time, i think we actually should panic here - if it is called in the wrong way it is a programming error that someone should fix.

Done

---

pkg/sql/logictest/testdata/logic_test/datetime line 2088 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the result here seems consistent with the postgres result (i.e., -5 for the timezone)

which version of postgres are you on (i am on 15.3 & tried this same thing for 15.4):

psql (15.3 (Homebrew))

Type "help" for help.

  

postgres=# SELECT make_timestamptz(2013, 7, 15, 8, 15, 23.5, 'America/New_York');

     make_timestamptz     

--------------------------

 2013-07-15 08:15:23.5-04

(1 row)

  

---

pkg/sql/logictest/testdata/logic_test/datetime line 2095 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: could you add tests for make_timestamptz and date_trunc that pass in an invalid timezone name?

done

[rafiss]

great work!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @chrisseto)

---

pkg/sql/logictest/testdata/logic_test/datetime line 2088 at r1 (raw file):

Previously, annrpom (annie pompa) wrote…

which version of postgres are you on (i am on 15.3 & tried this same thing for 15.4):

psql (15.3 (Homebrew))

Type "help" for help.

  

postgres=# SELECT make_timestamptz(2013, 7, 15, 8, 15, 23.5, 'America/New_York');

     make_timestamptz     

--------------------------

 2013-07-15 08:15:23.5-04

(1 row)

  

i just remembered why this could happen - the way the timestamptz result is displayed depends on the timezone you use in postgres:

postgres=# set time zone 'utc';
SET
postgres=# SELECT make_timestamptz(2013, 7, 15, 8, 15, 23.5, 'America/New_York');;
     make_timestamptz
--------------------------
 2013-07-15 12:15:23.5+00
(1 row)

postgres=# set time zone 'America/New_York';
SET
postgres=# SELECT make_timestamptz(2013, 7, 15, 8, 15, 23.5, 'America/New_York');;
     make_timestamptz
--------------------------
 2013-07-15 08:15:23.5-04
(1 row)

postgres=# set time zone '-5';
SET
postgres=# SELECT make_timestamptz(2013, 7, 15, 8, 15, 23.5, 'America/New_York');;
     make_timestamptz
--------------------------
 2013-07-15 07:15:23.5-05
(1 row)

[rafiss]

i think you'd mentioned that you were considering adding a separate commit to this PR to fix some incompatibilities - were you still planning to?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @chrisseto)

---

pkg/sql/sem/builtins/builtins.go line 11385 at r6 (raw file):

				location, err = timeutil.TimeZoneStringToLocation(string(tree.MustBeDString(args[6])), timeutil.TimeZoneStringToLocationPOSIXStandard)
				if err != nil {
					return nil, pgerror.New(pgcode.DatetimeFieldOverflow, "the given timezone could not be parsed properly")

some nits:

trying this in postgres, it seems like we should use pgcode.InvalidParameterValue here

the error message here should include the input timezone, that way it is easier for the user to see what they misspelled. there are two ways to do that.

in this case, the original error already has that information, so we can do: pgerror.WithCandidateCode(err, pgcode.InvalidParameterValue)

if it did not have all the information we needed, we would use pgerror.Wrapf(err, pgcode.InvalidParameterValue, "time zone %q not recognized", timezone)

---

pkg/sql/logictest/testdata/logic_test/datetime line 637 at r6 (raw file):

statement error pgcode 22009 pq: date_trunc\(\): negative year value is not valid
SELECT date_trunc('hours', '2016-02-10 19:46:33.306157519 BC'::timestamp);

could you explain this one a bit more? (apologies if you already mentioned this elsewhere, but it's just easier to discuss in writing on the PR itself)

trying on postgres, i get:

postgres=# SELECT date_trunc('hours', '2016-02-10 19:46:33.306157519 BC'::timestamp);;
       date_trunc
------------------------
 2016-02-10 19:00:00 BC
(1 row)

[annrpom]

not anymore, i believe. the main thing that is blocking this PR is the fact that there was a difference in how dates/timestamps were being printed (via their respective String() methods) in crdb vs postgres when dealing with BC years. in my attempt to tinker with a solution for this, i saw that the standard was to be ISO 8601 compliant (https://www.postgresql.org/docs/current/datatype-datetime.html#:~:text=The%20output%20format%20of%20the,of%20the%20ISO%208601%20format.), which is the reason why we see a year like 0000 being represented as 1 BC (and -0001 == 2 BC & so on). we seem to follow this, but postgres doesn't? what do you think?

otherwise, the solution to this would be to override the String() function for the Date struct (and probably something similar to timestamp)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @rafiss)

---

pkg/sql/sem/builtins/builtins.go line 11385 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

some nits:

trying this in postgres, it seems like we should use pgcode.InvalidParameterValue here

the error message here should include the input timezone, that way it is easier for the user to see what they misspelled. there are two ways to do that.

in this case, the original error already has that information, so we can do: pgerror.WithCandidateCode(err, pgcode.InvalidParameterValue)

if it did not have all the information we needed, we would use pgerror.Wrapf(err, pgcode.InvalidParameterValue, "time zone %q not recognized", timezone)

done - ty for catching this

---

pkg/sql/logictest/testdata/logic_test/datetime line 637 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could you explain this one a bit more? (apologies if you already mentioned this elsewhere, but it's just easier to discuss in writing on the PR itself)

trying on postgres, i get:

postgres=# SELECT date_trunc('hours', '2016-02-10 19:46:33.306157519 BC'::timestamp);;
       date_trunc
------------------------
 2016-02-10 19:00:00 BC
(1 row)

oh - interesting! this the original query i used in postgres to determine this behavior

postgres=# SELECT date_trunc('hours', '-2016-02-10 19:46:33.306157519'::timestamp);

ERROR:  22009: time zone displacement out of range: "-2016-02-10 19:46:33.306157519"

LINE 1: SELECT date_trunc('hours', '-2016-02-10 19:46:33.306157519':...

                                   ^

LOCATION:  DateTimeParseError, datetime.c:4044

i personally think that the behaviors between these two queries should be consistent, but i am happy to change it to consistently succeed instead of consistently fail

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @chrisseto)

---

pkg/sql/logictest/testdata/logic_test/datetime line 637 at r6 (raw file):

Previously, annrpom (annie pompa) wrote…

oh - interesting! this the original query i used in postgres to determine this behavior

postgres=# SELECT date_trunc('hours', '-2016-02-10 19:46:33.306157519'::timestamp);

ERROR:  22009: time zone displacement out of range: "-2016-02-10 19:46:33.306157519"

LINE 1: SELECT date_trunc('hours', '-2016-02-10 19:46:33.306157519':...

                                   ^

LOCATION:  DateTimeParseError, datetime.c:4044

i personally think that the behaviors between these two queries should be consistent, but i am happy to change it to consistently succeed instead of consistently fail

after looking a bit more, it seems that the issue here is not with the builtin itself. it happens just when converting the string to a timestamp

postgres=# select '-2016-02-10 19:46:33.306157519'::timestamp;
ERROR:  22009: time zone displacement out of range: "-2016-02-10 19:46:33.306157519"
LINE 1: select '-2016-02-10 19:46:33.306157519'::timestamp;
               ^
LOCATION:  DateTimeParseError, datetime.c:3796

postgres=# select '2016-02-10 19:46:33.306157519 BC'::timestamp;
           timestamp
-------------------------------
 2016-02-10 19:46:33.306158 BC
(1 row)

i think it's OK to be more permissive than postgres here.

[annrpom]

we had a discussion IRL about this :-) yes, there is a difference between our behavior vs postgres behavior when we format the result of a make_* with a negative year, but our follows the ISO 8601 standard; thus, it is OK to not match postgres here

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @rafiss)

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @chrisseto)

---

-- commits line 17 at r8:
super super nit: this last sentence can be moved to the commit message, outside of the release note

a user reading this release note probably won't know what we mean by "behaviors do not align." this is referring to aligning with postgres behavior right?

---

pkg/sql/logictest/testdata/logic_test/datetime line 637 at r8 (raw file):

statement error pgcode 22009 pq: date_trunc\(\): negative year value is not valid
SELECT date_trunc('hours', '2016-02-10 19:46:33.306157519 BC'::timestamp);

i thought where we landed is that we were going to be more permissive than postgres. that is, we would allow both of the following:

SELECT date_trunc('hours', '2016-02-10 19:46:33.306157519 BC'::timestamp);

SELECT date_trunc('hours', '-2016-02-10 19:46:33.306157519'::timestamp);

[annrpom]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @rafiss)

---

-- commits line 17 at r8:

Previously, rafiss (Rafi Shamim) wrote…

super super nit: this last sentence can be moved to the commit message, outside of the release note

a user reading this release note probably won't know what we mean by "behaviors do not align." this is referring to aligning with postgres behavior right?

ah yes makes sense - done

---

pkg/sql/logictest/testdata/logic_test/datetime line 637 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

after looking a bit more, it seems that the issue here is not with the builtin itself. it happens just when converting the string to a timestamp

postgres=# select '-2016-02-10 19:46:33.306157519'::timestamp;
ERROR:  22009: time zone displacement out of range: "-2016-02-10 19:46:33.306157519"
LINE 1: select '-2016-02-10 19:46:33.306157519'::timestamp;
               ^
LOCATION:  DateTimeParseError, datetime.c:3796

postgres=# select '2016-02-10 19:46:33.306157519 BC'::timestamp;
           timestamp
-------------------------------
 2016-02-10 19:46:33.306158 BC
(1 row)

i think it's OK to be more permissive than postgres here.

done!

[rafiss]

lgtm! nice work

i'll add a backport label since that was added for compatibility with a tool

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @chrisseto)

[annrpom]

TFTR! ('-')7
bors r=rafiss, chrisseto

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from ffe1fd7 to blathers/backport-release-23.1-108824: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}