Immutable casting of string > timestamp

[nandanrao]

I have two needs for my computed columns that worked in v20.1.4 but no longer work in v20.2.4 due to the better checking of function volatility in computed columns (#51886):

1. timestamptz + interval
2. string > timestamptz

Now, the first I can solve by changing timestamptz to timestamp (not an issue, all my clients run in UTC). Given that both timestamp and timestamptz are stored in UTC under the hood and only displayed to the client in local time, I'm not sure what context the operation is dependent on, but I can work around it.

The second I cannot work around because string > timestamp is also not Immutable:

cockroach/pkg/sql/sem/tree/casts.go

Line 249 in 9f97177

{from: types.StringFamily, to: types.TimestampFamily, volatility: VolatilityStable},

I'm sure there are issues that I'm not aware of, but it seems to me that it aught to be possible to express a timestamp in a string format in a way that corresponds to a UTC time in a non-context-dependent way, after all, the whole purpose of ISO formats and the like is to encode timestamps into strings such that they do indeed represent a single point in time.

Also, thanks for all the hard work :)

[blathers-crl]

Hello, I am Blathers. I am here to help you get the issue triaged.

It looks like you have not filled out the issue in the format of any of our templates. To best assist you, we advise you to use one of these templates.

I have CC'd a few people who may be able to assist you:

• @RaduBerinde (author of sql: check volatility of all operators #51886)
• @mgartner (commented on sql: check volatility of all operators #51886)

If we have not gotten back to your issue within a few business days, you can try the following:

• Join our community slack channel and ask on #cockroachdb.
• Try find someone from here if you know they worked closely on the area and CC them.

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

[RaduBerinde]

CC @otan

Sorry for the apparent regression, unfortunately these changes were necessary to ensure correctness in all cases.

I am pretty sure we had an example for 1 where the result was different (it was surprising to me too). I can try to dig it out.

With 2 the problem is that unfortunately things like 'now', 'tomorrow' can be cast to timestamps.. When the string is a constant literal, we make a best effort to determine if we're in one of these cases. But we can't do anything for variable inputs. I see two options: stop supporting 'now' etc when they are not constants (though this is fragile, as what is constant sometimes depends on how smart the optimizer is; and might break other uses). Or, add a special function to convert strings to timestamp which only supports absolute values.

[RaduBerinde]

Also, I don't know if it's possible in your usecase, but consider using non-computed columns with default values instead.

[RaduBerinde]

Here's an example for 1, with postgres:

 radu=> set time zone 'Europe/Berlin';
 SET
 radu=> select extract(epoch FROM TIMESTAMP WITH TIME ZONE '2010-11-06 23:59:00-05:00' + INTERVAL '1 day');
  date_part  
 ------------
  1289192340
 (1 row)
 
 radu=> set time zone 'America/Chicago';
 SET
 radu=> select extract(epoch FROM TIMESTAMP WITH TIME ZONE '2010-11-06 23:59:00-05:00' + INTERVAL '1 day');
  date_part  
 ------------
  1289195940
 (1 row)

[nandanrao]

Woah!!!

postgres=# set time zone 'America/Chicago';
SET
postgres=# select '2010-11-06 12:59:00-05:00'::TIMESTAMPTZ + INTERVAL '24 hours';
        ?column?
------------------------
 2010-11-07 11:59:00-06
(1 row)

postgres=# select '2010-11-06 12:59:00-05:00'::TIMESTAMPTZ + INTERVAL '1 day';
        ?column?
------------------------
 2010-11-07 12:59:00-06
(1 row)

postgres=# set time zone 'Europe/Berlin';
SET
postgres=# select '2010-11-06 12:59:00-05:00'::TIMESTAMPTZ + INTERVAL '24 hours';
        ?column?
------------------------
 2010-11-07 18:59:00+01
(1 row)

postgres=# select '2010-11-06 12:59:00-05:00'::TIMESTAMPTZ + INTERVAL '1 day';
        ?column?
------------------------
 2010-11-07 18:59:00+01
(1 row)

I see now that, indeed, "a day" is a localized interval! In some moments in time, in some places, a day is less than 24 hours. Fair enough!

As for 2 -

Hrm, yes if "now" can be cast into a timestamp, then indeed, I see that string > timestamp is not viable as an immutable construct. And while this is a crazy design choice, in keeping with PSQL compatibility, I see that it's necessary.

A pure function that casts strings into timestamps sounds like a totally reasonable thing to exist, to be honest! It would logically be very simple and maintainable and would be a matter of just extracting the default clause (https://github.com/cockroachdb/cockroach/blob/master/pkg/util/timeutil/pgdate/field_extract.go#L164) of the casting logic and exposing that for use by a wrapper function, no?

[nandanrao]

(Ah, I'm afraid non-computed columns don't work for my use-case, as it's pulling from a quite flexible JSON object, but to be honest, my case my not be too representative. I've been pushing a lot of logic onto Cockroach via computed columns, which simplifies my life a lot and has so far been a quite elegant and relatively scalable, just write times slow down but that's easy to deal with, but it's not exactly traditional by any means)

[RaduBerinde]

Indeed. I can add that, unless you want to work on it - you seem to be familiar with the code :)
I know that we have a facility for parsing timestamps which returns a flag indicating if the context was used; we could simply error out in those cases.

[RaduBerinde]

By the way, the next major release will likely add virtual computed columns (sounds like it might be useful to you).

[nandanrao]

Ha, it would be fun for me to play around with the Cockroach code but I suspect you and your team will get to it before me if you think it's useful, I dunno when I would be able to prioritize this!

Virtual? As in, computed at query time rather than write time?

[RaduBerinde]

Yeah, exactly.

[RaduBerinde]

@nandanrao I am adding a parse_timestamp builtin which parses an absolute timestamp string into a TIMESTAMP. It should be available in the next 20.2.x release.

[nandanrao]

Ha, wow, amazing!!!

Thanks a lot, @RaduBerinde, that's a big headache removed for me, personally. I really appreciate that.

And it's a super clean implementation for something that seems useful!