Re: [HACKERS] Interval aggregate regression failure

On Sep 1, 2006, at 20:39 , Michael Glaesemann wrote:

> 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.

Okay. Here's the patch. Bruce, does it work for you?

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 12:28:23 -0000
***************
*** 2494,2504 ****
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;
--- 2494,2507 ----
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;
***************
*** 2506,2511 ****
--- 2509,2553 ----
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 12:28:24 -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))