Re: NaNs in numeric_power (was Re: Postgres 11 release notes)

[Tom Lane]

Huong Dangminh  writes:
> Thank you. The patch looks fine to me.
> Also, I have done the "make check" in Windows and Linux environment with no 
> problem.

Pushed, thanks for reviewing/testing.

regards, tom lane

RE: NaNs in numeric_power (was Re: Postgres 11 release notes)

[Huong Dangminh]

Hi, 

> From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> David Rowley  writes:
> > On 16 May 2018 at 02:01, Tom Lane  wrote:
> >> I'm not particularly fussed about getting credit for that.  However,
> >> looking again at how that patch series turned out --- ie, that we
> >> ensured POSIX behavior for NaNs only in HEAD --- I wonder whether we
> >> shouldn't do what was mentioned in the commit log for 6bdf1303, and
> >> teach numeric_pow() about these same special cases.
> >> It seems like it would be more consistent to change both functions
> >> for v11, rather than letting that other shoe drop in some future
> >> major release.
> 
> > I'm inclined to agree. It's hard to imagine these two functions
> > behaving differently in regards to NaN input is useful to anyone.
> 

Thank you. The patch looks fine to me.
Also, I have done the "make check" in Windows and Linux environment with no 
problem.


Thanks and best regards,
---
Dang Minh Huong
NEC Solution Innovators, Ltd.
http://www.nec-solutioninnovators.co.jp/en/

Re: NaNs in numeric_power (was Re: Postgres 11 release notes)

[Dean Rasheed]

On 16 May 2018 at 14:44, Tom Lane  wrote:
> Dean Rasheed  writes:
>> In the case 1 ^ NaN = 1, what should the result scale be?
>
> The result is exact, so I don't see a reason to be worried about its
> scale.  Messing with the scale would also require expending even
> more code on what is, in the end, a corner case that no users have
> expressed concern about.
>

OK, fine. Not really worth worrying about.

Regards,
Dean

Re: NaNs in numeric_power (was Re: Postgres 11 release notes)

[David Rowley]

On 16 May 2018 at 09:55, Tom Lane  wrote:
> David Rowley  writes:
>> On 16 May 2018 at 02:01, Tom Lane  wrote:
>>> I'm not particularly fussed about getting credit for that.  However,
>>> looking again at how that patch series turned out --- ie, that
>>> we ensured POSIX behavior for NaNs only in HEAD --- I wonder
>>> whether we shouldn't do what was mentioned in the commit log for
>>> 6bdf1303, and teach numeric_pow() about these same special cases.
>>> It seems like it would be more consistent to change both functions
>>> for v11, rather than letting that other shoe drop in some future
>>> major release.
>
>> I'm inclined to agree. It's hard to imagine these two functions
>> behaving differently in regards to NaN input is useful to anyone.
>
> Here's a proposed patch for that.

Looks good to me.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: NaNs in numeric_power (was Re: Postgres 11 release notes)

[Tom Lane]

Dean Rasheed  writes:
> On 15 May 2018 at 22:55, Tom Lane  wrote:
>> Here's a proposed patch for that.

> In the case 1 ^ NaN = 1, what should the result scale be?

The result is exact, so I don't see a reason to be worried about its
scale.  Messing with the scale would also require expending even
more code on what is, in the end, a corner case that no users have
expressed concern about.

regards, tom lane

Re: NaNs in numeric_power (was Re: Postgres 11 release notes)

[Dean Rasheed]

On 15 May 2018 at 22:55, Tom Lane  wrote:
> David Rowley  writes:
>> On 16 May 2018 at 02:01, Tom Lane  wrote:
>>> I'm not particularly fussed about getting credit for that.  However,
>>> looking again at how that patch series turned out --- ie, that
>>> we ensured POSIX behavior for NaNs only in HEAD --- I wonder
>>> whether we shouldn't do what was mentioned in the commit log for
>>> 6bdf1303, and teach numeric_pow() about these same special cases.
>>> It seems like it would be more consistent to change both functions
>>> for v11, rather than letting that other shoe drop in some future
>>> major release.
>
>> I'm inclined to agree. It's hard to imagine these two functions
>> behaving differently in regards to NaN input is useful to anyone.
>
> Here's a proposed patch for that.
>

In the case 1 ^ NaN = 1, what should the result scale be?

For other inputs, the result scale is at least as large as the scale
of the first input, so I would suggest that the same ought to be the
case here.

Otherwise, this looks fine, and I agree that it makes thinks neater /
more consistent.

Regards,
Dean

NaNs in numeric_power (was Re: Postgres 11 release notes)

[Tom Lane]

David Rowley  writes:
> On 16 May 2018 at 02:01, Tom Lane  wrote:
>> I'm not particularly fussed about getting credit for that.  However,
>> looking again at how that patch series turned out --- ie, that
>> we ensured POSIX behavior for NaNs only in HEAD --- I wonder
>> whether we shouldn't do what was mentioned in the commit log for
>> 6bdf1303, and teach numeric_pow() about these same special cases.
>> It seems like it would be more consistent to change both functions
>> for v11, rather than letting that other shoe drop in some future
>> major release.

> I'm inclined to agree. It's hard to imagine these two functions
> behaving differently in regards to NaN input is useful to anyone.

Here's a proposed patch for that.

regards, tom lane

diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index dcf31e3..8dfdffc 100644
*** a/src/backend/utils/adt/numeric.c
--- b/src/backend/utils/adt/numeric.c
*** numeric_power(PG_FUNCTION_ARGS)
*** 2972,2981 
  	NumericVar	result;
  
  	/*
! 	 * Handle NaN
  	 */
! 	if (NUMERIC_IS_NAN(num1) || NUMERIC_IS_NAN(num2))
  		PG_RETURN_NUMERIC(make_result(_nan));
  
  	/*
  	 * Initialize things
--- 2972,2998 
  	NumericVar	result;
  
  	/*
! 	 * Handle NaN cases.  We follow the POSIX spec for pow(3), which says that
! 	 * NaN ^ 0 = 1, and 1 ^ NaN = 1, while all other cases with NaN inputs
! 	 * yield NaN (with no error).
  	 */
! 	if (NUMERIC_IS_NAN(num1))
! 	{
! 		if (!NUMERIC_IS_NAN(num2))
! 		{
! 			init_var_from_num(num2, );
! 			if (cmp_var(, _zero) == 0)
! PG_RETURN_NUMERIC(make_result(_one));
! 		}
! 		PG_RETURN_NUMERIC(make_result(_nan));
! 	}
! 	if (NUMERIC_IS_NAN(num2))
! 	{
! 		init_var_from_num(num1, );
! 		if (cmp_var(, _one) == 0)
! 			PG_RETURN_NUMERIC(make_result(_one));
  		PG_RETURN_NUMERIC(make_result(_nan));
+ 	}
  
  	/*
  	 * Initialize things
diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out
index 17985e8..1cb3c3b 100644
*** a/src/test/regress/expected/numeric.out
--- b/src/test/regress/expected/numeric.out
*** select 0.0 ^ 12.34;
*** 1664,1669 
--- 1664,1700 
   0.
  (1 row)
  
+ -- NaNs
+ select 'NaN'::numeric ^ 'NaN'::numeric;
+  ?column? 
+ --
+   NaN
+ (1 row)
+ 
+ select 'NaN'::numeric ^ 0;
+  ?column? 
+ --
+ 1
+ (1 row)
+ 
+ select 'NaN'::numeric ^ 1;
+  ?column? 
+ --
+   NaN
+ (1 row)
+ 
+ select 0 ^ 'NaN'::numeric;
+  ?column? 
+ --
+   NaN
+ (1 row)
+ 
+ select 1 ^ 'NaN'::numeric;
+  ?column? 
+ --
+ 1
+ (1 row)
+ 
  -- invalid inputs
  select 0.0 ^ (-12.34);
  ERROR:  zero raised to a negative power is undefined
diff --git a/src/test/regress/sql/numeric.sql b/src/test/regress/sql/numeric.sql
index d77504e..a939412 100644
*** a/src/test/regress/sql/numeric.sql
--- b/src/test/regress/sql/numeric.sql
*** select (-12.34) ^ 0.0;
*** 911,916 
--- 911,923 
  select 12.34 ^ 0.0;
  select 0.0 ^ 12.34;
  
+ -- NaNs
+ select 'NaN'::numeric ^ 'NaN'::numeric;
+ select 'NaN'::numeric ^ 0;
+ select 'NaN'::numeric ^ 1;
+ select 0 ^ 'NaN'::numeric;
+ select 1 ^ 'NaN'::numeric;
+ 
  -- invalid inputs
  select 0.0 ^ (-12.34);
  select (-12.34) ^ 1.2;