Re: [HACKERS] Interval aggregate regression failure

Michael Glaesemann wrote:
> On Aug 30, 2006, at 7:12 , Bruce Momjian wrote:
>
> > Here are the results using my newest patch:
> >
> > test=> select interval '41 mon 12 days 360:00' / 10 as quotient_a
> > , interval '41 mon -12 days -360:00' / 10 as quotient_b
> > , interval '-41 mon 12 days 360:00' / 10 as quotient_c
> > , interval '-41 mon -12 days -360:00' / 10 as quotient_d;
> > quotient_a | quotient_b |
> > quotient_c | quotient_d
> > ------------------------+-------------------------
> > +---------------------------+---------------------------
> > 4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2
> > days +40:48:00 | -4 mons -4 days -40:48:00
> > (1 row)
> >
> > test=> select interval '41 mon 12 days 360:00' * 0.3 as product_a
> > , interval '41 mon -12 days -360:00' * 0.3 as product_b
> > , interval '-41 mon 12 days 360:00' * 0.3 as product_c
> > , interval '-41 mon -12 days -360:00' * 0.3 as product_d;
> > product_a | product_b |
> > product_c | product_d
> > --------------------------+--------------------------
> > +-----------------------------+------------------------------
> > 1 year 12 days 122:24:00 | 1 year 6 days -122:24:00 | -1 years -6
> > days +122:24:00 | -1 years -12 days -122:24:00
> > (1 row)
> >
> > I see no "23:60" entries.
>
> Using Bruce's newest patch, I still get the "23:60" entries on my
> machine (no integer-datetimes)

Strange, I do not see that here. Is there something wrong with our
hour/minute display? Someone posted a patch a few days ago for that.

Here is a test program. What does it show for you?

#include <stdio.h>


int
main(int argc, char *argv[])
{
double x;

x = 41;
x = x / 10.0;
printf("%f\n", x);
x = x - (int)x;
x = x * 30;
printf("%15.15f\n", x);
x = 0.1 * 30;
printf("%15.15f\n", x);
return 0;
}

The output for me is:

4.100000000000000
2.999999999999989
3.000000000000000

>
> select version();
>
> version
> ------------------------------------------------------------------------
> ------------------------------------------------------------------------
> -
> PostgreSQL 8.2devel on powerpc-apple-darwin8.7.0, compiled by GCC
> powerpc-apple-darwin8-gcc-4.0.1 (GCC) 4.0.1 (Apple Computer, Inc.
> build 5341)
> (1 row)

Powerpc. Hmmm. I am on Intel.

> select interval '41 mon 12 days 360:00' / 10 as quotient_a
> , interval '41 mon -12 days -360:00' / 10 as quotient_b
> , interval '-41 mon 12 days 360:00' / 10 as quotient_c
> , interval '-41 mon -12 days -360:00' / 10 as quotient_d;
> quotient_a | quotient_b |
> quotient_c | quotient_d
> ------------------------+-------------------------
> +---------------------------+---------------------------
> 4 mons 4 days 40:48:00 | 4 mons 2 days -40:48:00 | -4 mons -2 days
> +40:48:00 | -4 mons -4 days -40:48:00
> (1 row)
>
> select interval '41 mon 12 days 360:00' * 0.3 as product_a
> , interval '41 mon -12 days -360:00' * 0.3 as product_b
> , interval '-41 mon 12 days 360:00' * 0.3 as product_c
> , interval '-41 mon -12 days -360:00' * 0.3 as product_d;
> product_a | product_b |
> product_c | product_d
> --------------------------+-----------------------------
> +-----------------------------+---------------------------------
> 1 year 12 days 122:24:00 | 1 year 6 days -122:23:60.00 | -1 years -6
> days +122:24:00 | -1 years -12 days -122:23:60.00
> (1 row)
>

Yea, I see that -122:23:60.00.

> > The code assume if it is within 0.000001 of a whole number, it
> > should be
> > rounded to a whole number. Patch attached with comments added.
>
> > /* fractional months full days into days */
> > month_remainder_days = month_remainder * DAYS_PER_MONTH;
> > + /*
> > + * The remainders suffer from float rounding, so if they are
> > + * within 0.000001 of an integer, we round them to integers.
> > + */
> > + if (month_remainder_days != (int32)month_remainder_days &&
> > + TSROUND(month_remainder_days) == rint(month_remainder_days))
> > + month_remainder_days = rint(month_remainder_days);
> > result->day += (int32) month_remainder_days;
> >
>
> Don't we want to be checking for rounding at the usec level rather
> than 0.000001 of a day? I think this should be
>
> if (month_remainder_days != (int32)month_remainder_days &&
> TSROUND(month_remainder_days * SECS_PER_DAY) ==
> rint(month_remainder_days * SECS_PER_DAY))
> month_remainder_days = rint(month_remainder_days);
>

Good idea. I updated the patch to do that.

> Another question I have concerns the month_remainder_days != (int32)
> month_remainder_days comparison. If I understand it correctly, if the
> TSROUND == rint portion is true, the first part is true. Or is this
> just a quick, fast check to see if it's necessary to do a more
> computationally intensive check?

Yea, just an optimization, but I was worried that the computations might
throw problems for certain numbers, so I figured I would only trigger it
when necessary.

> TSROUND isn't defined for HAVE_INT64_TIMESTAMP. My first attempt at
> performing a corresponding comparison doesn't work:
>
> + if (month_remainder_days != (int32) month_remainder_days &&
> + #ifdef HAVE_INT64_TIMESTAMP
> + rint(month_remainder_days * USECS_PER_DAY) ==
> + (month_remainder_days * USECS_PER_DAY))
> + #else
> + TSROUND(month_remainder_days * SECS_PER_DAY) ==
> + rint(month_remainder_days * SECS_PER_DAY))
> + #endif
> + month_remainder_days = rint(month_remainder_days);
>
> Anyway, I'll pound on this some more tonight.

You don't want to do any conditional tests. SECS_PER_DAY is all you
want. Those two macros are useful only in representing the internal
storage format, not for float compuations of rounding.

Patch attached. It also fixes a regression test output too.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +