[Patch] Log10 and hyperbolic functions for SQL:2016 compliance

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Lætitia Avrot
Hello hackers,

In his blog post (What's new in SQL 2016)[https://modern-sql.com/blog/2017-06/whats-new-in-sql-2016], Markus Winand explained some of the changes added to SQL:2016. I spotted that Postgres was behind other RDBMS on hyperbolic functions and log10 function. 
The log10 function existed but under the name log(<value>).

The new functions can be called in a simple select statement :

    select log10(100);
    select sinh(0);
    select cosh(0);
    select tanh(0);

Even if Markus Winand had added hyperbolic functions in the paragraph "Trigonometric and Logarithmic Functions", I didn't add hyperbolic function with the trigonometric functions in the documentation, because hyperbolic functions are not trigonometric functions.

I added regression tests for the new functions, but I didn't for log10 function, assuming that if log function worked, log10 will work too.

You'll find enclosed the first version of the patch that can build successfully on my laptop against master. I'm open to any improvement.

Cheers,

Lætitia
--
Think! Do you really need to print this email ?
There is no Planet B.

adding_log10_and_hyperbolic_functions_v1.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Tom Lane-2
=?UTF-8?Q?L=C3=A6titia_Avrot?= <[hidden email]> writes:
> [ adding_log10_and_hyperbolic_functions_v1.patch ]

No objection to the feature, but

- Why are you using the float4-width library functions (coshf etc)
rather than the float8-width ones (cosh etc)?

- I wonder whether these library functions exist everywhere.
If they don't, what will we do about it ... throw an error?

I see that SUSv2 requires cosh() and so on, so it's quite possible
that these do exist everywhere we care about.  I'd be okay with
throwing a patch onto the buildfarm to see, and adding an autoconf
test only if the buildfarm is unhappy.  But we should be clear on
what we're going to do about it if that happens.

> I added regression tests for the new functions, but I didn't for log10
> function, assuming that if log function worked, log10 will work too.

Not quite sure I believe that.

Actually, what I'd counsel is that you *not* include tests of what
these do with Inf and NaN.  There is no upside to doing so, and lots
of downside if older platforms are squirrely in their behavior, which
is hardly unlikely (cf opossum ...)

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Lætitia Avrot
Hi,

Thanks for your time and advice, Tom!


> [ adding_log10_and_hyperbolic_functions_v1.patch ]

No objection to the feature, but

- Why are you using the float4-width library functions (coshf etc)
rather than the float8-width ones (cosh etc)?

Well, I guess the only reason is that I wasn't paying attention enough... I changed for the float8-width library functions.
 
- I wonder whether these library functions exist everywhere.
If they don't, what will we do about it ... throw an error?

I see that SUSv2 requires cosh() and so on, so it's quite possible
that these do exist everywhere we care about.  I'd be okay with
throwing a patch onto the buildfarm to see, and adding an autoconf
test only if the buildfarm is unhappy.  But we should be clear on
what we're going to do about it if that happens.

I think I was too confident because of the "it works on my laptop" syndrome... I don't know how to answer to this point.
 
> I added regression tests for the new functions, but I didn't for log10
> function, assuming that if log function worked, log10 will work too.

Not quite sure I believe that.

Do you mean I should also add a regression test for the new log10 function too ?
 
Actually, what I'd counsel is that you *not* include tests of what
these do with Inf and NaN.  There is no upside to doing so, and lots
of downside if older platforms are squirrely in their behavior, which
is hardly unlikely (cf opossum ...)

I changed the regression tests for hyperbolic functions, so it doesn't test for Inf and NaN.  

You'll find enclosed the new version of the patch.

Cheers,

Lætitia

adding_log10_and_hyperbolic_functions_v2.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Lætitia Avrot
Hi,

Thanks to Emil Iggland's kind review, I added precision on documentation for hyperbolic functions.

I added the patch to the next commitfest.

Cheers,

Lætitia

Le dim. 27 janv. 2019 à 20:39, Lætitia Avrot <[hidden email]> a écrit :
Hi,

Thanks for your time and advice, Tom!


> [ adding_log10_and_hyperbolic_functions_v1.patch ]

No objection to the feature, but

- Why are you using the float4-width library functions (coshf etc)
rather than the float8-width ones (cosh etc)?

Well, I guess the only reason is that I wasn't paying attention enough... I changed for the float8-width library functions.
 
- I wonder whether these library functions exist everywhere.
If they don't, what will we do about it ... throw an error?

I see that SUSv2 requires cosh() and so on, so it's quite possible
that these do exist everywhere we care about.  I'd be okay with
throwing a patch onto the buildfarm to see, and adding an autoconf
test only if the buildfarm is unhappy.  But we should be clear on
what we're going to do about it if that happens.

I think I was too confident because of the "it works on my laptop" syndrome... I don't know how to answer to this point.
 
> I added regression tests for the new functions, but I didn't for log10
> function, assuming that if log function worked, log10 will work too.

Not quite sure I believe that.

Do you mean I should also add a regression test for the new log10 function too ?
 
Actually, what I'd counsel is that you *not* include tests of what
these do with Inf and NaN.  There is no upside to doing so, and lots
of downside if older platforms are squirrely in their behavior, which
is hardly unlikely (cf opossum ...)

I changed the regression tests for hyperbolic functions, so it doesn't test for Inf and NaN.  

You'll find enclosed the new version of the patch.

Cheers,

Lætitia


--
Think! Do you really need to print this email ?
There is no Planet B.

adding_log10_and_hyperbolic_functions_v3.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Alvaro Herrera-9
On 2019-Jan-31, Lætitia Avrot wrote:

> Hi,
>
> Thanks to Emil Iggland's kind review, I added precision on documentation
> for hyperbolic functions.

Hello

I see that in dtanh() you set errno to 0 before calling tanh(), but 1)
you don't check for it afterwards (seems like you should be checking for
ERANGE, as well as checking the return value for isinf()), and 2) you
don't do that in dsinh() and dcosh() and I'm not quite sure I see why.
What's up with that?

Thanks,

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Lætitia Avrot
Hi Alvaro,

Thank you so much for taking the time to review the patch and for taking the time again to sort things
out with me this evening.



I see that in dtanh() you set errno to 0 before calling tanh(), but 1)
you don't check for it afterwards (seems like you should be checking for
ERANGE, as well as checking the return value for isinf()), and 2) you
don't do that in dsinh() and dcosh() and I'm not quite sure I see why.
What's up with that?


At the time I wrote that patch, I tried to include errno testing.Then, I think again and 
came back with the wrong idea everything would be fine.

By re-reading math.h documentation, it is now clear that the three functions can raise a 
ERANGE error.

There are two cases :
- range error due to overflow occurs.
- range error occurs due to underflow. In that case, the correct result (after rounding) is returned. So I assume we can ignore that case.

For sinh and cosh, we can have both cases and we added support for overflow.

For tanh, the only possible case is underflow and then, the result is correct.

We included comments to explain errno handling in those functions.

Cheers,

Lætitia

adding_log10_and_hyperbolic_functions_v4.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Andrew Gierth
>>>>> "Lætitia" == Lætitia Avrot <[hidden email]> writes:

 [snip patch]

The spec doesn't require the inverse functions (asinh, acosh, atanh),
but surely there is no principled reason to omit them?

--
Andrew (irc:RhodiumToad)

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Tom Lane-2
Andrew Gierth <[hidden email]> writes:
> The spec doesn't require the inverse functions (asinh, acosh, atanh),
> but surely there is no principled reason to omit them?

+1 --- AFAICS, the C library has offered all six since C89.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Lætitia Avrot
Hi Andrew and Tom,

I considered that option before writing my patch but I refrained for 2 reasons:

- There is no consensus about how to name these functions. The standard 8000-2 goes with arsinh, arcosh and artanh,
  but you will find easily arcsinh, arccosh and arctanh or even argsinh, argcosh and argtanh. In IT, the names asinh, 
  acosh and atanh are commonly used too. We might implement them with asinh, acosh and atanh names and add
  aliases if SQL standard decide to add it under other names though.
- If we go with inverse hyperbolic functions, I guess we could add other hyperbolic functions as hyperbolic cosecant,
  secant and cotangent too. Then it adds the inverse hyperbolic functions of these three functions. These six functions
  are not described in math.h library. I guess it's because these functions are quite simple to deduce from the others.

So, as you're asking that too, maybe my reasons weren't good enough. You'll find enclosed a new version of the patch
with asinh, acosh and atanh (v5).

Then I tried for several days to implement the 6 last hyperbolic functions, but I wasn't satisfied with the result, so I just dropped it.

Cheers,

Lætitia

Le dim. 3 févr. 2019 à 16:12, Tom Lane <[hidden email]> a écrit :
Andrew Gierth <[hidden email]> writes:
> The spec doesn't require the inverse functions (asinh, acosh, atanh),
> but surely there is no principled reason to omit them?

+1 --- AFAICS, the C library has offered all six since C89.

                        regards, tom lane


--
Think! Do you really need to print this email ?
There is no Planet B.

adding_log10_and_hyperbolic_functions_v5.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Gavin Flower-2
On 12/02/2019 06:44, Lætitia Avrot wrote:

> Hi Andrew and Tom,
>
> I considered that option before writing my patch but I refrained for 2
> reasons:
>
> - There is no consensus about how to name these functions. The
> standard 8000-2 goes with arsinh, arcosh and artanh,
>   but you will find easily arcsinh, arccosh and arctanh or even
> argsinh, argcosh and argtanh. In IT, the names asinh,
>   acosh and atanh are commonly used too. We might implement them with
> asinh, acosh and atanh names and add
>   aliases if SQL standard decide to add it under other names though.
[...]

>
> Le dim. 3 févr. 2019 à 16:12, Tom Lane <[hidden email]
> <mailto:[hidden email]>> a écrit :
>
>     Andrew Gierth <[hidden email]
>     <mailto:[hidden email]>> writes:
>     > The spec doesn't require the inverse functions (asinh, acosh,
>     atanh),
>     > but surely there is no principled reason to omit them?
>
>     +1 --- AFAICS, the C library has offered all six since C89.
>
>                             regards, tom lane
>
[...]

I can only remember coming across the asinh, acosh, and atanh forms.  In
45 years of programming.


Cheers,
Gavin


Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

Tom Lane-2
Gavin Flower <[hidden email]> writes:

> On 12/02/2019 06:44, Lætitia Avrot wrote:
>> I considered that option before writing my patch but I refrained for 2
>> reasons:
>>
>> - There is no consensus about how to name these functions. The
>> standard 8000-2 goes with arsinh, arcosh and artanh,
>>   but you will find easily arcsinh, arccosh and arctanh or even
>> argsinh, argcosh and argtanh. In IT, the names asinh,
>>   acosh and atanh are commonly used too. We might implement them with
>> asinh, acosh and atanh names and add
>>   aliases if SQL standard decide to add it under other names though.

> I can only remember coming across the asinh, acosh, and atanh forms.  In
> 45 years of programming.

I don't think this is a problem.  Postgres has never had any hesitation
about adopting C-standard function names if there's nothing in the SQL
standard.  The C standard says asinh etc, so those are the names to use.

As Lætitia says, there'd be little problem with adding aliases if someday
the SQL committee decides to add these with other spellings.

                        regards, tom lane