Re: abs function for interval

Hi,

On 2019-10-31 23:20:07 -0300, Euler Taveira wrote:
> diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
> index 1dc4c820de..a6b8b8c221 100644
> --- a/src/backend/utils/adt/timestamp.c
> +++ b/src/backend/utils/adt/timestamp.c
> @@ -2435,6 +2435,23 @@ interval_cmp(PG_FUNCTION_ARGS)
> PG_RETURN_INT32(interval_cmp_internal(interval1, interval2));
> }
>
> +Datum
> +interval_abs(PG_FUNCTION_ARGS)
> +{
> + Interval *interval = PG_GETARG_INTERVAL_P(0);
> + Interval *result;
> +
> + result = palloc(sizeof(Interval));
> + *result = *interval;
> +
> + /* convert all struct Interval members to absolute values */
> + result->month = (interval->month < 0) ? (-1 * interval->month) : interval->month;
> + result->day = (interval->day < 0) ? (-1 * interval->day) : interval->day;
> + result->time = (interval->time < 0) ? (-1 * interval->time) : interval->time;
> +
> + PG_RETURN_INTERVAL_P(result);
> +}
> +

Several points:

1) I don't think you can do the < 0 check on an elementwise basis. Your
code would e.g. make a hash out of abs('1 day -1 second'), by
inverting the second, but not the day (whereas nothing should be
done).

It'd probably be easiest to implement this by comparing with a 0
interval using inteval_lt() or interval_cmp_internal().

2) This will not correctly handle overflows, I believe. What happens if you
do SELECT abs('-2147483648 days'::interval)? You probably should
reuse interval_um() for this.

> --- a/src/test/regress/expected/interval.out
> +++ b/src/test/regress/expected/interval.out
> @@ -927,3 +927,11 @@ select make_interval(secs := 7e12);
> @ 1944444444 hours 26 mins 40 secs
> (1 row)
>
> +-- test absolute operator
> +set IntervalStyle to postgres;
> +select @ interval '1 years -2 months 3 days 4 hours -5 minutes 6.789 seconds' as t;
> + t
> +-----------------------------
> + 10 mons 3 days 03:55:06.789
> +(1 row)
> +
> diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
> index bc5537d1b9..8f9a2bda29 100644
> --- a/src/test/regress/sql/interval.sql
> +++ b/src/test/regress/sql/interval.sql

> @@ -308,3 +308,7 @@ select make_interval(months := 'NaN'::float::int);
> select make_interval(secs := 'inf');
> select make_interval(secs := 'NaN');
> select make_interval(secs := 7e12);
> +
> +-- test absolute operator
> +set IntervalStyle to postgres;
> +select @ interval '1 years -2 months 3 days 4 hours -5 minutes 6.789 seconds' as t;
> --
> 2.11.0

This is not even remotely close to enough tests. In your only test abs()
does not change the value, as there's no negative component (the 1 year
-2 month result in a positive 10 months, and the hours, minutes and
seconds get folded together too).

At the very least a few boundary conditions need to be tested (see b)
above), a few more complicated cases with different components being
of different signs, and you need to show the values with and without
applying abs().

Greetings,

Andres Freund