Re: [HACKERS] Interval aggregate regression failure

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