Re: [PATCHES] Interval aggregate regression failure - Mailing list pgsql-hackers

Here's a patch that appears to work. Gives the same output with and
without --enable-integer-datetimes. Answers look like they're correct.

I'm basically treating the components as three different intervals
(with the other two components zero), rounding them each to usecs,
and adding them together.

While it might be nice to carry a little extra precision around, it
doesn't seem to be needed in these cases. If errors do arise, they
should be at most 3 usec, which is pretty much noise for the floating
point case, I suspect.

Bruce, how's it look on your machine? If it looks good, I'll add the
examples we've been using to the regression tests.

Michael Glaesemann
grzm seespotcode net


Index: src/backend/utils/adt/timestamp.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.165
diff -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    1 Sep 2006 11:26:12 -0000
***************
*** 2494,2511 ****
       float8        factor = PG_GETARG_FLOAT8(1);
       double        month_remainder,
                   day_remainder,
!                 month_remainder_days;
       Interval   *result;

       result = (Interval *) palloc(sizeof(Interval));

!     month_remainder = span->month * factor;
!     day_remainder = span->day * factor;
       result->month = (int32) month_remainder;
       result->day = (int32) day_remainder;
       month_remainder -= result->month;
       day_remainder -= result->day;

       /*
        * The above correctly handles the whole-number part of the month
and day
        * products, but we have to do something with any fractional part
--- 2494,2553 ----
       float8        factor = PG_GETARG_FLOAT8(1);
       double        month_remainder,
                   day_remainder,
!                 month_remainder_days,
!                 month_remainder_time,
!                 day_remainder_time;
       Interval   *result;

       result = (Interval *) palloc(sizeof(Interval));

!
!     month_remainder = span->month / factor;
!     day_remainder = span->day / factor;
       result->month = (int32) month_remainder;
       result->day = (int32) day_remainder;
       month_remainder -= result->month;
       day_remainder -= result->day;

+     month_remainder_days = month_remainder * DAYS_PER_MONTH;
+
+     /*
+         if month_remainder_days is not an integer, check to see if it's an
+         integer when converted to SECS or USECS.
+         If it is, round month_remainder_days to the nearest integer
+
+      */
+
+     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;
+
+ #ifdef HAVE_INT64_TIMESTAMP
+     month_remainder_time = rint((month_remainder_days -
+                             (int32)month_remainder_days) * USECS_PER_DAY);
+
+     day_remainder_time = rint(day_remainder * USECS_PER_DAY);
+
+
+     result->time = rint(rint(span->time * factor) + day_remainder_time +
+                        month_remainder_time);
+ #else
+     month_remainder_time = rint((month_remainder_days -
+                            (int32)month_remainder_days) * SECS_PER_DAY);
+     day_remainder_time = rint(day_remainder * SECS_PER_DAY);
+
+     result->time = span->time * factor + day_remainder_time +
+                     month_remainder_time;
+ #endif
+
+
+     day_remainder = span->day * factor;
+     result->day = (int32) day_remainder;
+     day_remainder -= result->day;
+
       /*
        * The above correctly handles the whole-number part of the month
and day
        * products, but we have to do something with any fractional part
***************
*** 2518,2531 ****

       /* fractional months full days into days */
       month_remainder_days = month_remainder * DAYS_PER_MONTH;
!     result->day += (int32) month_remainder_days;
!     /* fractional months partial days into time */
!     day_remainder += month_remainder_days - (int32) month_remainder_days;

   #ifdef HAVE_INT64_TIMESTAMP
!     result->time = rint(span->time * factor + day_remainder *
USECS_PER_DAY);
   #else
!     result->time = span->time * factor + day_remainder * SECS_PER_DAY;
   #endif

       PG_RETURN_INTERVAL_P(result);
--- 2560,2599 ----

       /* 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.
!      */

   #ifdef HAVE_INT64_TIMESTAMP
!     if (month_remainder_days != (int32)month_remainder_days &&
!         rint(month_remainder_days * USECS_PER_DAY) ==
!           (int64)(month_remainder_days * USECS_PER_DAY))
!         month_remainder_days = rint(month_remainder_days);
!     result->day += (int32) month_remainder_days;
!     month_remainder_time = rint((month_remainder_days -
!                             (int32)month_remainder_days) * USECS_PER_DAY);
!
!     day_remainder_time = rint(day_remainder * USECS_PER_DAY);
!
!     result->time = rint(span->time * factor + day_remainder_time +
!                        month_remainder_time);
   #else
!     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;
!
!     month_remainder_time = rint((month_remainder_days -
!                            (int32)month_remainder_days) * SECS_PER_DAY);
!
!     day_remainder_time = rint(day_remainder * SECS_PER_DAY);
!
!     result->time = span->time * factor + day_remainder_time +
!                     month_remainder_time;
!
   #endif

       PG_RETURN_INTERVAL_P(result);
***************
*** 2548,2554 ****
       float8        factor = PG_GETARG_FLOAT8(1);
       double        month_remainder,
                   day_remainder,
!                 month_remainder_days;
       Interval   *result;

       result = (Interval *) palloc(sizeof(Interval));
--- 2616,2624 ----
       float8        factor = PG_GETARG_FLOAT8(1);
       double        month_remainder,
                   day_remainder,
!                 month_remainder_days,
!                 month_remainder_time,
!                 day_remainder_time;
       Interval   *result;

       result = (Interval *) palloc(sizeof(Interval));
***************
*** 2571,2584 ****

       /* fractional months full days into days */
       month_remainder_days = month_remainder * DAYS_PER_MONTH;
       result->day += (int32) month_remainder_days;
-     /* fractional months partial days into time */
-     day_remainder += month_remainder_days - (int32) month_remainder_days;

! #ifdef HAVE_INT64_TIMESTAMP
!     result->time = rint(span->time / factor + day_remainder *
USECS_PER_DAY);
! #else
!     result->time = span->time / factor + day_remainder * SECS_PER_DAY;
   #endif

       PG_RETURN_INTERVAL_P(result);
--- 2641,2680 ----

       /* 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;

!     day_remainder_time = day_remainder * SECS_PER_DAY;
!     if (day_remainder_time != (int32)day_remainder_time &&
!         TSROUND(day_remainder_time) ==
!         rint(day_remainder_time))
!         day_remainder_time = rint(day_remainder_time);
!
! #ifdef HAVE_INT64_TIMESTAMP
!     day_remainder_time = day_remainder * USECS_PER_DAY;
!     if (day_remainder_time != (int32)day_remainder_time &&
!         TSROUND(day_remainder_time) == rint(day_remainder_time))
!         day_remainder_time = rint(day_remainder_time);
!
!     result->time = rint(span->time / factor + day_remainder_time +
!        (month_remainder_days - (int32)month_remainder_days) *
USECS_PER_DAY;
! #else
!     day_remainder_time = day_remainder * SECS_PER_DAY;
!     if (day_remainder_time != (int32)day_remainder_time &&
!         TSROUND(day_remainder_time) == rint(day_remainder_time))
!         day_remainder_time = rint(day_remainder_time);
!
!     result->time = span->time / factor + day_remainder_time +
!        (month_remainder_days - (int32)month_remainder_days) *
SECS_PER_DAY;
   #endif

       PG_RETURN_INTERVAL_P(result);
Index: src/include/utils/timestamp.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/timestamp.h,v
retrieving revision 1.62
diff -c -r1.62 timestamp.h
*** src/include/utils/timestamp.h    13 Jul 2006 16:49:20 -0000    1.62
--- src/include/utils/timestamp.h    1 Sep 2006 11:26:15 -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))