Add missing function abs (interval)

On Mon, Mar 29, 2021 at 3:33 PM Isaac Morland <isaac.morland@gmail.com>
wrote:

On a newly set up system there are 7 types with a unary minus operator

defined, but only 6 of them have an abs function:

...

Would a patch to add a function with this behaviour to the initial

database be welcome?

Looking in the archives, I see this attempt that you can build upon:

/messages/by-id/CAHE3wggpj+k-zXLUdcBDRe3oahkb21pSMPDm-HzPjZxJn4vMMw@mail.gmail.com

If so, should I implement it essentially like the above, or as an

internal function? I've noticed that even when it seems like it might be
reasonable to implement a built-in function as an SQL function they tend to
be internal.

By default it should be internal.

--
John Naylor
EDB: http://www.enterprisedb.com

On Mon, Mar 29, 2021 at 07:15:19PM -0400, John Naylor wrote:

Looking in the archives, I see this attempt that you can build upon:
/messages/by-id/CAHE3wggpj+k-zXLUdcBDRe3oahkb21pSMPDm-HzPjZxJn4vMMw@mail.gmail.com

I see no problem with doing something more here. If you can get a
patch, please feel free to add it to the next commit fest, for
Postgres 15:
https://commitfest.postgresql.org/33/
--
Michael

I've attached a patch for this. Turns out there was a comment in the source
explaining that there is no interval_abs because it's not clear what to
return; but I think it's clear that if i is an interval the larger of i and
-i should be considered to be the absolute value, the same as would be done
for any type; essentially, if the type is orderable and has a meaningful
definition of unary minus, the definition of abs follows from those.

This does have some odd effects, as was observed in the previous discussion
pointed at by John Naylor above (for which thanks!). But those odd effects
are not due to abs(interval) itself but rather due to the odd behaviour of
interval, where values which compare equal to '0'::interval can change a
timestamp when added to it. This in turn comes from what the interval data
type is trying to do combined with the inherently complicated nature of our
timekeeping system.

I have included in the test case some testing of what happens with '1 month
-30 days'::interval, which is "equal" to '0'::interval.

At least one thing concerns me about my code: Given an interval i, I
palloc() space to calculate -i; then either return that or the original
input depending on the result of a comparison. Will I leak space as a
result? Should I free the value if I don't return it?

In addition to adding abs(interval) and related @ operator, I would like to
update interval_smaller and interval_larger to change < and > to <= and >=
respectively. This is to make the min(interval) and max(interval)
aggregates return the first of multiple distinct "equal" intervals,
contrary to the current behaviour:

odyssey=> select max (i) from (values ('1 month -30 days'::interval), ('-1
month 30 days'))t(i);
max
------------------
-1 mons +30 days
(1 row)

odyssey=> select min (i) from (values ('1 month -30 days'::interval), ('-1
month 30 days'))t(i);
min
------------------
-1 mons +30 days
(1 row)

odyssey=>

GREATEST and LEAST already take the first value:

odyssey=> select greatest ('1 month -30 days'::interval, '-1 month 30
days');
greatest
----------------
1 mon -30 days
(1 row)

odyssey=> select least ('1 month -30 days'::interval, '-1 month 30 days');
least
----------------
1 mon -30 days
(1 row)

odyssey=>

On Mon, 29 Mar 2021 at 21:06, Michael Paquier <michael@paquier.xyz> wrote:

Isaac Morland <isaac.morland@gmail.com> writes:

I've attached a patch for this. Turns out there was a comment in the source
explaining that there is no interval_abs because it's not clear what to
return; but I think it's clear that if i is an interval the larger of i and
-i should be considered to be the absolute value, the same as would be done
for any type; essentially, if the type is orderable and has a meaningful
definition of unary minus, the definition of abs follows from those.

The problem with that blithe summary is the hidden assumption that
values that compare "equal" aren't interesting to distinguish. As
the discussion back in 2009 pointed out, this doesn't help you decide
what to do with cases like '1 month -30 days'::interval. Either answer
you might choose seems pretty arbitrary --- and we've got more than
enough arbitrariness in type interval :-(

For similar reasons, I find myself mighty suspicious of your proposal
to change how max(interval) and min(interval) work. That cannot make
things any better overall --- it will only move the undesirable results
from one set of cases to some other set. Moreover your argument for
it seems based on a false assumption, that the input values can be
expected to arrive in a particular order. So I'm inclined to think
that backwards compatibility is sufficient reason to leave that alone.

If we wanted to make some actual progress here, maybe we should
reconsider the definition of equality/ordering for interval, with
an eye to not allowing two intervals to be considered "equal" unless
they really are identical. That is, for two values that are currently
reported as "equal", apply some more-or-less-arbitrary tiebreak rule,
say by sorting on the individual fields left-to-right. This would be
very similar to type text's rule that only bitwise-equal strings are
really equal, even if strcoll() claims otherwise. I am not sure how
feasible this idea is from a compatibility standpoint, but it's
something to think about.

regards, tom lane

I wrote:

Isaac Morland <isaac.morland@gmail.com> writes:

I've attached a patch for this. Turns out there was a comment in the source
explaining that there is no interval_abs because it's not clear what to
return; but I think it's clear that if i is an interval the larger of i and
-i should be considered to be the absolute value, the same as would be done
for any type; essentially, if the type is orderable and has a meaningful
definition of unary minus, the definition of abs follows from those.

The problem with that blithe summary is the hidden assumption that
values that compare "equal" aren't interesting to distinguish.

After thinking about this some more, it seems to me that it's a lot
clearer that the definition of abs(interval) is forced by our comparison
rules if you define it as

CASE WHEN x < '0'::interval THEN -x ELSE x END

In particular, this makes it clear what happens and why for values
that compare equal to zero. The thing that is bothering me about
the formulation GREATEST(x, -x) is exactly that whether you get x
or -x in such a case depends on a probably-unspecified implementation
detail inside GREATEST().

BTW, you could implement this by something along the lines of
(cf generate_series_timestamp()):

MemSet(&interval_zero, 0, sizeof(Interval));
if (interval_cmp_internal(interval, &interval_zero) < 0)
return interval_um(fcinfo);
else
PG_RETURN_INTERVAL_P(interval);

which would avoid the need to refactor interval_um().

regards, tom lane

On Sun, 26 Sept 2021 at 13:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Isaac Morland <isaac.morland@gmail.com> writes:

I've attached a patch for this. Turns out there was a comment in the

source

explaining that there is no interval_abs because it's not clear what to
return; but I think it's clear that if i is an interval the larger of i

and

-i should be considered to be the absolute value, the same as would be

done

for any type; essentially, if the type is orderable and has a meaningful
definition of unary minus, the definition of abs follows from those.

The problem with that blithe summary is the hidden assumption that
values that compare "equal" aren't interesting to distinguish.

After thinking about this some more, it seems to me that it's a lot
clearer that the definition of abs(interval) is forced by our comparison
rules if you define it as

CASE WHEN x < '0'::interval THEN -x ELSE x END

In particular, this makes it clear what happens and why for values
that compare equal to zero. The thing that is bothering me about
the formulation GREATEST(x, -x) is exactly that whether you get x
or -x in such a case depends on a probably-unspecified implementation
detail inside GREATEST().

Thanks very much for continuing to think about this. It really reinforces
my impression that this community takes seriously input and suggestions,
even when it takes some thought to work out how (or whether) to proceed
with a proposed change.

So I think I will prepare a revised patch that uses this formulation; and
if I still have any suggestions that aren't directly related to adding
abs(interval) I will split them off into a separate discussion.

BTW, you could implement this by something along the lines of

On 26 Sep 2021, at 19:58, Isaac Morland <isaac.morland@gmail.com> wrote:

So I think I will prepare a revised patch that uses this formulation; and if I still have any suggestions that aren't directly related to adding abs(interval) I will split them off into a separate discussion.

This CF entry is marked Waiting on Author, have you had the chance to prepare
an updated version of this patch?

--
Daniel Gustafsson https://vmware.com/

On Thu, 4 Nov 2021 at 08:08, Daniel Gustafsson <daniel@yesql.se> wrote:

On 26 Sep 2021, at 19:58, Isaac Morland <isaac.morland@gmail.com> wrote:

So I think I will prepare a revised patch that uses this formulation;

and if I still have any suggestions that aren't directly related to adding
abs(interval) I will split them off into a separate discussion.

This CF entry is marked Waiting on Author, have you had the chance to
prepare
an updated version of this patch?

Not yet, but thanks for the reminder. I will try to get this done on the
weekend.

On Fri, Nov 05, 2021 at 12:06:05AM -0400, Isaac Morland wrote:

Not yet, but thanks for the reminder. I will try to get this done on the
weekend.

Seeing no updates, this has been switched to returned with feedback in
the CF app.
--
Michael