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