sql: fix the interval/numeric conversions

[knz]

Fixes #25241.

CockroachDB supports converting intervals to/from int. This is a
CockroachDB-specific feature not supported (yet?) in PostgreSQL.

This feature was added in order to simplify analytic queries in a CRL
internal database.

Until now this feature was undocumented and, arguably, non-public.

It was also wrong:

• intervals with a month or day part would be truncated silently. This
  is surprising and arguably incorrect.

• the interval conversion to int would produce the number of seconds,
  whereas the conversion from int to interval would use a number of
  microseconds. This prevented the idempotence of the conversion
  ::int::interval::int or ::interval::int::interval.

This patch fixes this internal feature to make it more correct.
It also extends it to other types (float, decimal) for consistency.

Release note (bug fix): the CockroachDB-specific, undocumented
conversion from interval to/from numeric types was wrong and has
been corrected. It may become documented in the future.

[cockroach-teamcity]

This change is 

[maddyblue]

this comment says microseconds. is that correct?

[knz]

Woops, thanks. Corrected.

[maddyblue]

This happens to guarantee that d.Exponent = 0 because RoundToIntegralValue is implemented under the covers with Quantize(d, x, 0). However I think it is better to call that explicitly in case RoundToIntegralValue ever changes that could allow d.Exponent > 0, which would make the below computation using d.Coeff incorrect since it would have some implied 0s at the end. Since this shouldn't ever round, we can also use ExactCtx. So I think it should be ExactCtx.Quantize(d, &dnanos, 0)

[knz]

I have changed to Quantize but I had to define a new context: ExactCtx has Precision = 0 so ExactCtx.Quantize can never work unless the argument is itself zero.
Can you check I have done this properly?

Also Quantize() has a bunch of check after the call to quantize() which I believe are not needed here. Is there an alternate code path that would allow me to skip these checks? (Does this question even make sense?)

[maddyblue]

Ah yeah I forgot quantize uses Quo under its own covers. Is there a good reason to not use HighPrecisionCtx here? I think it'll work correctly and would then remove the need to add another ctx.

No, there's not another code path to skip those checks unless you implemented Quantize yourself, which we shouldn't. Those checks are fine.

[knz]

ok, thanks. Done.

[knz]

Thank you!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)

[bdarnell]

This is less obviously broken than before, but it's still incorrect to assume that all months have 30 days (or, more subtly, that all days have 24 hours). That's why postgres doesn't have this cast (and why go's time package doesn't have constants larger than time.Hour).

I think it was a mistake for us to have introduced this cast. Whatever query this was meant to facilitate should be converting everything to intervals and timestamps for its calculations, instead of converting intervals to numbers.

[knz]

@bdarnell we already normalize intervals to 30 days a months and 24 hours a day to compare intervals together (otherwise, what could we do?). I didn't invent these factors.

[bdarnell]

Make intervals non-comparable since comparing INTERVAL '30 days' and INTERVAL '1 month' is not well-defined? But I guess we do need some sort of ordering here (and most intervals don't use days or months).

In retrospect (and without the restrictions of postgres/standard compatibility), I think there are two different interval types: one that's just a number of seconds and supports ordering, etc, and one that supports all the different units but must be added to a datetime to get a comparable value.

[knz]

I agree with the sentiment, but what should we do?

Postgres also does this this way:

• the interval data type also has day, month, seconds
• it uses the same conversion factor, for example INTERVAL '3.5 month' is 1 month + 15 days, 1.5 day is 1 day + 12 hours.
• the code for interval-interval comparisons works like this:

static inline TimeOffset
interval_cmp_value(const Interval *interval)
{
        TimeOffset      span;

        span = interval->time;

#ifdef HAVE_INT64_TIMESTAMP
        span += interval->month * INT64CONST(30) * USECS_PER_DAY;
        span += interval->day * INT64CONST(24) * USECS_PER_HOUR;
#else
        span += interval->month * ((double) DAYS_PER_MONTH * SECS_PER_DAY);
        span += interval->day * ((double) HOURS_PER_DAY * SECS_PER_HOUR);
#endif

        return span;
}

What we are doing is arguably equivalent (although perhaps we should change our code to do the same double conversion).

Where can we go from there that would be better?

[bdarnell]

Interesting. I guess we should keep the comparison functions the same then, although I'm still not convinced that we should have the numeric conversions.

[knz]

Yes that's why I am also fine keeping these conversions undocumented.