Re: [HACKERS] Interval aggregate regression failure - Mailing list pgsql-patches

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@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: src/backend/utils/adt/timestamp.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.165
diff -c -c -r1.165 timestamp.c
*** src/backend/utils/adt/timestamp.c    13 Jul 2006 16:49:16 -0000    1.165
--- src/backend/utils/adt/timestamp.c    30 Aug 2006 03:45:41 -0000
***************
*** 2518,2523 ****
--- 2518,2532 ----

      /* 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 1us of an integer, we round them to integers.
+      *    It seems doing two multiplies is causing the imprecision.
+      */
+     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);
      result->day += (int32) month_remainder_days;
      /* fractional months partial days into time */
      day_remainder += month_remainder_days - (int32) month_remainder_days;
***************
*** 2571,2576 ****
--- 2580,2595 ----

      /* 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 1us of an integer, we round them to integers.
+      *    It seems doing a division and multiplies is causing the
+      *    imprecision.
+      */
+     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);
      result->day += (int32) month_remainder_days;
      /* fractional months partial days into time */
      day_remainder += month_remainder_days - (int32) month_remainder_days;
Index: src/include/utils/timestamp.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/timestamp.h,v
retrieving revision 1.62
diff -c -c -r1.62 timestamp.h
*** src/include/utils/timestamp.h    13 Jul 2006 16:49:20 -0000    1.62
--- src/include/utils/timestamp.h    30 Aug 2006 03:45:42 -0000
***************
*** 161,171 ****

  typedef double fsec_t;

! /* round off to MAX_TIMESTAMP_PRECISION decimal places */
! /* note: this is also used for rounding off intervals */
  #define TS_PREC_INV 1000000.0
  #define TSROUND(j) (rint(((double) (j)) * TS_PREC_INV) / TS_PREC_INV)
- #endif

  #define TIMESTAMP_MASK(b) (1 << (b))
  #define INTERVAL_MASK(b) (1 << (b))
--- 161,175 ----

  typedef double fsec_t;

! #endif
!
! /*
!  *    Round off to MAX_TIMESTAMP_PRECISION decimal places.
!  *    This is also used for rounding off intervals, and
!  *    for rounding interval multiplication/division calculations.
!  */
  #define TS_PREC_INV 1000000.0
  #define TSROUND(j) (rint(((double) (j)) * TS_PREC_INV) / TS_PREC_INV)

  #define TIMESTAMP_MASK(b) (1 << (b))
  #define INTERVAL_MASK(b) (1 << (b))
Index: src/test/regress/expected/interval.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/interval.out,v
retrieving revision 1.15
diff -c -c -r1.15 interval.out
*** src/test/regress/expected/interval.out    6 Mar 2006 22:49:17 -0000    1.15
--- src/test/regress/expected/interval.out    30 Aug 2006 03:45:43 -0000
***************
*** 218,224 ****
  select avg(f1) from interval_tbl;
                         avg
  -------------------------------------------------
!  @ 4 years 1 mon 9 days 28 hours 18 mins 23 secs
  (1 row)

  -- test long interval input
--- 218,224 ----
  select avg(f1) from interval_tbl;
                         avg
  -------------------------------------------------
!  @ 4 years 1 mon 10 days 4 hours 18 mins 23 secs
  (1 row)

  -- test long interval input