Re: Infinite Interval

On Tue, Sep 12, 2023 at 2:39 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Thu, 24 Aug 2023 at 14:51, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > The patches still apply. But here's a rebased version with one white
> > space error fixed. Also ran pgindent.
> >
>
> This needs another rebase,

Fixed.

> and it looks like the infinite interval
> input code is broken.
>

The code required to handle 'infinity' as an input value was removed by
d6d1430f404386162831bc32906ad174b2007776. I have added a separate
commit which reverts that commit as 0004, which should be merged into
0003.

> I took a quick look, and had a couple of other review comments:
>
> 1). In interval_mul(), I think "result_sign" would be a more accurate
> name than "result_is_inf" for the local variable.

Fixed as part of 0003.

>
> 2). interval_accum() and interval_accum_inv() don't work correctly
> with infinite intervals. To make them work, they need to count the
> number of infinities seen, to allow them to be subtracted off by the
> inverse function (similar to the code in numeric.c, except for the
> NaN-handling, which will need to be different). Consider, for example:
>
> SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING)
>   FROM (VALUES ('1 day'::interval),
>                ('3 days'::interval),
>                ('infinity'::timestamptz - now()),
>                ('4 days'::interval),
>                ('6 days'::interval)) v(x);
> ERROR:  interval out of range
>
> as compared to:
>
> SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING)
>   FROM (VALUES (1::numeric),
>                (3::numeric),
>                ('infinity'::numeric),
>                (4::numeric),
>                (6::numeric)) v(x);
>
>     x     |        avg
> ----------+--------------------
>         1 | 2.0000000000000000
>         3 |           Infinity
>  Infinity |           Infinity
>         4 | 5.0000000000000000
>         6 | 6.0000000000000000
> (5 rows)

Nice catch. I agree that we need to do something similar to
numeric_accum and numeric_accum_inv. As part of that also add test for
window aggregates on interval data type. We might also need some fix
to sum(). I am planning to work on this next week but in case somebody
else wants to pick this up here are patches with other things fixed.

--
Best Wishes,
Ashutosh Bapat