Skip site navigation (1) Skip section navigation (2)

Re: [HACKERS] Interval aggregate regression failure

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Michael Glaesemann <grzm(at)seespotcode(dot)net>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Michael Paesold <mpaesold(at)gmx(dot)at>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: [HACKERS] Interval aggregate regression failure
Date: 2006-08-30 03:50:40
Message-ID: 200608300350.k7U3oeH06639@momjian.us (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-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(at)momjian(dot)us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Attachment: /pgpatches/interval
Description: text/x-diff (3.8 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Bruce MomjianDate: 2006-08-30 04:22:05
Subject: Re: Interval month, week -> day
Previous:From: Tom LaneDate: 2006-08-30 03:28:53
Subject: Re: TODO Request

pgsql-patches by date

Next:From: Bruce MomjianDate: 2006-08-30 04:22:05
Subject: Re: Interval month, week -> day
Previous:From: Michael GlaesemannDate: 2006-08-30 01:30:43
Subject: Re: [HACKERS] Interval aggregate regression failure (expected seems

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group