Infinite Interval

Hi Joseph,
I stumbled upon this requirement a few times. So I started working on
this support in my spare time as a hobby project to understand
horology code in PostgreSQL. This was sitting in my repositories for
more than an year. Now that I have someone else showing an interest,
it's time for it to face the world. Rebased it, fixed conflicts.

PFA patch implementing infinite interval. It's still WIP, there are
TODOs in the code and also the commit message lists things that are
known to be incomplete. You might want to assess expected output
carefully

On Sun, Dec 11, 2022 at 12:51 AM Joseph Koshakow <koshy44@gmail.com> wrote:>

The proposed design from the most recent thread was to reserve
INT32_MAX months for infinity and INT32_MIN months for negative
infinity. As pointed out in the thread, these are currently valid
non-infinite intervals, but they are out of the documented range.

The patch uses both months and days together to avoid this problem.

Please feel free to complete the patch, work on review comments etc. I
will help as and when I find time.

--
Best Wishes,
Ashutosh Bapat

On Mon, Dec 12, 2022 at 8:05 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

Hi Joseph,
I stumbled upon this requirement a few times. So I started working on
this support in my spare time as a hobby project to understand
horology code in PostgreSQL. This was sitting in my repositories for
more than an year. Now that I have someone else showing an interest,
it's time for it to face the world. Rebased it, fixed conflicts.

PFA patch implementing infinite interval. It's still WIP, there are
TODOs in the code and also the commit message lists things that are
known to be incomplete. You might want to assess expected output
carefully

That's great! I was also planning to just work on it as a hobby
project, so I'll try and review and add updates as I find free
time as well.

The proposed design from the most recent thread was to reserve
INT32_MAX months for infinity and INT32_MIN months for negative
infinity. As pointed out in the thread, these are currently valid
non-infinite intervals, but they are out of the documented range.

The patch uses both months and days together to avoid this problem.

Can you expand on this part? I believe the full range of representable
intervals are considered valid as of v15.

- Joe Koshakow

Hi Ashutosh,

I've added tests for all the operators and functions involving
intervals and what I think the expected behaviors to be. The
formatting might be slightly off and I've left the contents of the
error messages as TODOs. Hopefully it's a good reference for the
implementation.

Adding infinite interval to an infinite timestamp with opposite
direction is not going to yield 0 but some infinity. Since we are adding
interval to the timestamp the resultant timestamp is an infinity
preserving the direction.

I think I disagree with this. Tom Lane in one of the previous threads
said:

tl;dr: we should model it after the behavior of IEEE float infinities,
except we'll want to throw errors where those produce NaNs.

and I agree with this opinion. I believe that means that adding an
infinite interval to an infinite timestamp with opposite directions
should yield an error instead of some infinity. Since with floats this
would yield a NaN.

Dividing infinite interval by finite number keeps it infinite.
TODO: Do we change the sign of infinity if factor is negative?

Again if we model this after the IEEE float behavior, then the answer
is yes, we do change the sign of infinity.

- Joe Koshakow

On Sat, Dec 17, 2022 at 2:34 PM Joseph Koshakow <koshy44@gmail.com> wrote:

Hi Ashutosh,

I've added tests for all the operators and functions involving
intervals and what I think the expected behaviors to be. The
formatting might be slightly off and I've left the contents of the
error messages as TODOs. Hopefully it's a good reference for the
implementation.

Adding infinite interval to an infinite timestamp with opposite
direction is not going to yield 0 but some infinity. Since we are adding
interval to the timestamp the resultant timestamp is an infinity
preserving the direction.

I think I disagree with this. Tom Lane in one of the previous threads
said:

tl;dr: we should model it after the behavior of IEEE float infinities,
except we'll want to throw errors where those produce NaNs.

and I agree with this opinion. I believe that means that adding an
infinite interval to an infinite timestamp with opposite directions
should yield an error instead of some infinity. Since with floats this
would yield a NaN.

Dividing infinite interval by finite number keeps it infinite.
TODO: Do we change the sign of infinity if factor is negative?

Again if we model this after the IEEE float behavior, then the answer
is yes, we do change the sign of infinity.

- Joe Koshakow

I ended up doing some more work in the attached patch. Here are some
updates:

- I modified the arithmetic operators to more closely match IEEE
floats. Error messages are still all TODO, and they may have the wrong
error code.
- I implemented some more operators and functions.
- I moved the helper functions you created into macros in timestamp.h
to more closely match the implementation of infinite timestamps and
dates. Also so dates.c could access them.
- There seems to be an existing overflow error with interval
subtraction. Many of the arithmetic operators of the form
`X - Interval` are converted to `X + (-Interval)`. This will overflow
in the case that some interval field is INT32_MIN or INT64_MIN.
Additionally, negating a positive infinity interval won't result in a
negative infinity interval and vice versa. We'll have to come up with
an efficient solution for this.

- Joe Koshakow

Hi Ashutosh,

I ended up doing some more work on this today. All of the major
features should be implemented now. Below are what I think are the
outstanding TODOs:
- Clean up error messages and error codes
- Figure out how to correctly implement interval_part for infinite
intervals. For now I pretty much copied the implementation of
timestamp_part, but I'm not convinced that's correct.
- Fix horology tests.
- Test consolidation. After looking through the interval tests, I
realized that I may have duplicated some test cases. It would probably
be best to remove those duplicate tests.
- General cleanup, remove TODOs.

Attached is my most recent patch.

- Joe Koshakow

I have another update, I cleaned up some of the error messages, fixed
the horology tests, and ran pgindent.

- Joe

On Fri, Dec 30, 2022 at 10:47 PM Joseph Koshakow <koshy44@gmail.com> wrote:

I have another update, I cleaned up some of the error messages, fixed
the horology tests, and ran pgindent.

- Joe

Hi, there.

Since in float8 you can use '+inf', '+infinity', So should we also make
interval '+infinity' valid?
Also in timestamp. '+infinity'::timestamp is invalid, should we make it
valid.

In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch
select interval 'infinity' / float8 'infinity'; returns infinity.
I am not sure it's right. I found this related post (
https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity
).

I recommend David Deutsch's <<The Beginning of Infinity>>

Jian

On 12/31/22 06:09, jian he wrote:

Since in float8 you can use '+inf', '+infinity', So should we also make
interval '+infinity' valid?

Yes.

Also in timestamp. '+infinity'::timestamp is invalid, should we make it
valid.

Yes, we should. I wrote a trivial patch for this a while ago but it
appears I never posted it. I will post that in a new thread so as not
to confuse the bots.
--
Vik Fearing

On Sat, Dec 31, 2022 at 12:09 AM jian he <jian.universality@gmail.com> wrote:

In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch select interval 'infinity' / float8 'infinity'; returns infinity.
I am not sure it's right. I found this related post (https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity).

Good point, I agree this should return an error. We also need to
properly handle multiplication and division of infinite intervals by
float8 'nan'. My patch is returning an infinite interval, but it should
be returning an error. I'll upload a new patch shortly.

- Joe

On Mon, Jan 2, 2023 at 1:21 PM Joseph Koshakow <koshy44@gmail.com> wrote:

On Sat, Dec 31, 2022 at 12:09 AM jian he <jian.universality@gmail.com> wrote:

In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch select interval 'infinity' / float8 'infinity'; returns infinity.
I am not sure it's right. I found this related post (https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity).

Good point, I agree this should return an error. We also need to
properly handle multiplication and division of infinite intervals by
float8 'nan'. My patch is returning an infinite interval, but it should
be returning an error. I'll upload a new patch shortly.

- Joe

Attached is the patch to handle these scenarios. Apparently dividing by
NaN is currently broken:
postgres=# SELECT INTERVAL '1 day' / float8 'nan';
?column?
---------------------------------------------------
-178956970 years -8 mons -2562047788:00:54.775808
(1 row)

This patch will fix the issue, but we may want a separate patch that
handles this specific, existing issue. Any thoughts?

- Joe

I have another patch, this one adds validations to operations that
return intervals and updated error messages. I tried to give all of the
error messages meaningful text, but I'm starting to think that almost all
of them should just say "interval out of range". The current approach
may reveal some implementation details and lead to confusion. For
example, some subtractions are converted to additions which would lead
to an error message about addition.

SELECT date 'infinity' - interval 'infinity';
ERROR: cannot add infinite values with opposite signs

I've also updated the commit message to include the remaining TODOs,
which I've copied below

1. Various TODOs in code.
2. Correctly implement interval_part for infinite intervals.
3. Test consolidation.
4. Should we just use the months field to test for infinity?

On Tue, Jan 3, 2023 at 6:14 AM Joseph Koshakow <koshy44@gmail.com> wrote:

I have another patch, this one adds validations to operations that
return intervals and updated error messages. I tried to give all of the
error messages meaningful text, but I'm starting to think that almost all
of them should just say "interval out of range". The current approach
may reveal some implementation details and lead to confusion. For
example, some subtractions are converted to additions which would lead
to an error message about addition.

SELECT date 'infinity' - interval 'infinity';
ERROR: cannot add infinite values with opposite signs

I've also updated the commit message to include the remaining TODOs,
which I've copied below

1. Various TODOs in code.
2. Correctly implement interval_part for infinite intervals.
3. Test consolidation.
4. Should we just use the months field to test for infinity?

3. Test consolidation.
I used the DO command, reduced a lot of test sql code.
I don't know how to generate an interval.out file.
I hope the format is ok. I use https://sqlformat.darold.net/ format the sql
code.
Then I saw on the internet that one line should be no more than 80 chars.
so I slightly changed the format.

--
I recommend David Deutsch's <<The Beginning of Infinity>>

Jian

On Wed, Jan 4, 2023 at 10:13 PM jian he <jian.universality@gmail.com> wrote:

On Tue, Jan 3, 2023 at 6:14 AM Joseph Koshakow <koshy44@gmail.com> wrote:

I have another patch, this one adds validations to operations that
return intervals and updated error messages. I tried to give all of the
error messages meaningful text, but I'm starting to think that almost all
of them should just say "interval out of range". The current approach
may reveal some implementation details and lead to confusion. For
example, some subtractions are converted to additions which would lead
to an error message about addition.

SELECT date 'infinity' - interval 'infinity';
ERROR: cannot add infinite values with opposite signs

I've also updated the commit message to include the remaining TODOs,
which I've copied below

1. Various TODOs in code.
2. Correctly implement interval_part for infinite intervals.
3. Test consolidation.
4. Should we just use the months field to test for infinity?

3. Test consolidation.
I used the DO command, reduced a lot of test sql code.
I don't know how to generate an interval.out file.
I hope the format is ok. I use https://sqlformat.darold.net/ format the
sql code.
Then I saw on the internet that one line should be no more than 80 chars.
so I slightly changed the format.

--
I recommend David Deutsch's <<The Beginning of Infinity>>

Jian

1. Various TODOs in code.
logic combine and clean up for functions in backend/utils/adt/timestamp.c
(timestamp_pl_interval,timestamptz_pl_interval, interval_pl, interval_mi).
3. Test consolidation in /regress/sql/interval.sql

For 1. I don't know how to format the code. I have a problem installing
pg_indent. If the format is wrong, please reformat.
3. As the previous email thread.

On Thu, Jan 5, 2023 at 5:20 AM jian he <jian.universality@gmail.com> wrote:

On Wed, Jan 4, 2023 at 10:13 PM jian he <jian.universality@gmail.com> wrote:

I don't know how to generate an interval.out file.

Personally I just write the .out files manually. I think it especially
helps as a way to double-check that the results are what you expected.
After running make check a regressions.diff file will be generated with
all the differences between your .out file and the results of the test.

logic combine and clean up for functions in backend/utils/adt/timestamp.c (timestamp_pl_interval,timestamptz_pl_interval, interval_pl, interval_mi).

One thing I was hoping to achieve was to avoid redundant checks if
possible. For example, in the following code:

+    if ((INTERVAL_IS_NOBEGIN(span1) && INTERVAL_IS_NOEND(span2))
+      ||(INTERVAL_IS_NOBEGIN(span1) && !INTERVAL_NOT_FINITE(span2))
+      ||(!INTERVAL_NOT_FINITE(span1) && INTERVAL_IS_NOEND(span2)))
+           INTERVAL_NOBEGIN(result);

If `(INTERVAL_IS_NOBEGIN(span1) && INTERVAL_IS_NOEND(span2))` is false,
then we end up checking `INTERVAL_IS_NOBEGIN(span1)` twice

For 1. I don't know how to format the code. I have a problem installing pg_indent. If the format is wrong, please reformat.

I'll run pg_indent and send an updated patch if anything changes.

Thanks for your help on this patch!

- Joe Koshakow

Jian,

I incorporated your changes and updated interval.out and ran
pgindent. Looks like some of the error messages have changed and we
have some issues with parsing "+infinity" after rebasing.

- Joe

On Fri, Jan 6, 2023 at 6:54 AM Joseph Koshakow <koshy44@gmail.com> wrote:

Jian,

I incorporated your changes and updated interval.out and ran
pgindent. Looks like some of the error messages have changed and we
have some issues with parsing "+infinity" after rebasing.

- Joe

Looks like some of the error messages have changed and we

have some issues with parsing "+infinity" after rebasing.

There is a commit 2ceea5adb02603ef52579b568ca2c5aebed87358
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ceea5adb02603ef52579b568ca2c5aebed87358
if you pull this commit then you can do select interval '+infinity', even
though I don't know why.

On Thu, Jan 5, 2023 at 11:30 PM jian he <jian.universality@gmail.com> wrote:

On Fri, Jan 6, 2023 at 6:54 AM Joseph Koshakow <koshy44@gmail.com> wrote:

Looks like some of the error messages have changed and we
have some issues with parsing "+infinity" after rebasing.

There is a commit 2ceea5adb02603ef52579b568ca2c5aebed87358
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ceea5adb02603ef52579b568ca2c5aebed87358
if you pull this commit then you can do select interval '+infinity', even though I don't know why.

It turns out that I was just misreading the error. The test was
expecting us to fail on "+infinity" but we succeeded. I just removed
that test case.

pgindent. Looks like some of the error messages have changed

The conditions for checking valid addition/subtraction between infinite
values were missing some cases which explains the change in error
messages. I've updated the logic and removed duplicate checks.

I removed the extract/date_part tests since they were duplicated in a
test above. I also converted the DO command tests to using SQL with
joins so it more closely matches the existing tests.

I've updated the extract/date_part logic for infinite intervals. Fields
that are monotonically increasing should return +/-infinity and all
others should return NULL. For Intervals, the fields are the same as
timestamps plus the hour and day fields since those don't overflow into
the next highest field.

I think this patch is just about ready for review, except for the
following two questions:
1. Should finite checks on intervals only look at months or all three
fields?
2. Should we make the error messages for adding/subtracting infinite
values more generic or leave them as is?

My opinions are
1. We should only look at months.
2. We should make the errors more generic.

Anyone else have any thoughts?

- Joe

On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow <koshy44@gmail.com> wrote:

On Thu, Jan 5, 2023 at 11:30 PM jian he <jian.universality@gmail.com> wrote:

On Fri, Jan 6, 2023 at 6:54 AM Joseph Koshakow <koshy44@gmail.com> wrote:

Looks like some of the error messages have changed and we
have some issues with parsing "+infinity" after rebasing.

There is a commit 2ceea5adb02603ef52579b568ca2c5aebed87358
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ceea5adb02603ef52579b568ca2c5aebed87358
if you pull this commit then you can do select interval '+infinity', even though I don't know why.

It turns out that I was just misreading the error. The test was
expecting us to fail on "+infinity" but we succeeded. I just removed
that test case.

pgindent. Looks like some of the error messages have changed

The conditions for checking valid addition/subtraction between infinite
values were missing some cases which explains the change in error
messages. I've updated the logic and removed duplicate checks.

I removed the extract/date_part tests since they were duplicated in a
test above. I also converted the DO command tests to using SQL with
joins so it more closely matches the existing tests.

I've updated the extract/date_part logic for infinite intervals. Fields
that are monotonically increasing should return +/-infinity and all
others should return NULL. For Intervals, the fields are the same as
timestamps plus the hour and day fields since those don't overflow into
the next highest field.

I think this patch is just about ready for review, except for the
following two questions:
1. Should finite checks on intervals only look at months or all three
fields?
2. Should we make the error messages for adding/subtracting infinite
values more generic or leave them as is?

My opinions are
1. We should only look at months.
2. We should make the errors more generic.

Anyone else have any thoughts?

- Joe

Oops I forgot the actual patch. Please see attached.

On Sat, Jan 7, 2023 at 3:05 PM Joseph Koshakow <koshy44@gmail.com> wrote:

On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow <koshy44@gmail.com> wrote:

I think this patch is just about ready for review, except for the
following two questions:
1. Should finite checks on intervals only look at months or all three
fields?
2. Should we make the error messages for adding/subtracting infinite
values more generic or leave them as is?

My opinions are
1. We should only look at months.
2. We should make the errors more generic.

Anyone else have any thoughts?

Here's a patch with the more generic error messages.

- Joe