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

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

From
Lætitia Avrot
Date:
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.
Attachment

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

From
Tom Lane
Date:
=?UTF-8?Q?L=C3=A6titia_Avrot?= <laetitia.avrot@gmail.com> 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


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

From
Lætitia Avrot
Date:
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
Attachment

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

From
Lætitia Avrot
Date:
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 <laetitia.avrot@gmail.com> 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.
Attachment

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

From
Alvaro Herrera
Date:
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


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

From
Lætitia Avrot
Date:
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
Attachment

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

From
Andrew Gierth
Date:
>>>>> "Lætitia" == Lætitia Avrot <laetitia.avrot@gmail.com> 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)


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

From
Tom Lane
Date:
Andrew Gierth <andrew@tao11.riddles.org.uk> 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


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

From
Lætitia Avrot
Date:
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 <tgl@sss.pgh.pa.us> a écrit :
Andrew Gierth <andrew@tao11.riddles.org.uk> 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.
Attachment

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

From
Gavin Flower
Date:
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 <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> a écrit :
>
>     Andrew Gierth <andrew@tao11.riddles.org.uk
>     <mailto:andrew@tao11.riddles.org.uk>> 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



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

From
Tom Lane
Date:
Gavin Flower <GavinFlower@archidevsys.co.nz> 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


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

From
Dean Rasheed
Date:
On Sun, 3 Feb 2019 at 15:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrew Gierth <andrew@tao11.riddles.org.uk> 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.
>

+1 for including the inverse functions. However, it looks to me like
the inverse functions are C99-specific, so they might not be available
on all supported platforms. If they're not, we may need to provide our
own implementations.

I did a bit of research and had play. It looks like it might not be
too hard to provide our own implementations, but it does take a bit of
care to avoid rounding and overflow errors. Attached are some
standalone C programs where I tested various algorithms. A decent
approach seems to be to either use log1p() (which is itself
C99-specific, hence that's the first thing I played with) or to use a
single round of Newton-Raphson to correct rounding errors, in a
similar way to how we implement cbrt() on platforms that don't have
that.

Of course that may all be moot -- those functions may in fact be
available everywhere we care about, but it was interesting to play
around with them anyway.

Regards,
Dean

Attachment

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

From
Tom Lane
Date:
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> +1 for including the inverse functions. However, it looks to me like
> the inverse functions are C99-specific, so they might not be available
> on all supported platforms. If they're not, we may need to provide our
> own implementations.

FWIW, I'm pretty sure they're available everywhere.  It's true C89
doesn't mention them, but POSIX has had them for a long time.  The
SUSv2 version of POSIX has them, and so does my pet dinosaur HPUX 10.20,
which has this to say about their origin:

$ man asinh
...
STANDARDS CONFORMANCE
      asinh(): SVID3, XPG4.2

Windows, as usual, is a wild card, but as far as I can tell by googling
they exist in Windows too (at least recent versions).

It's definitely possible that there are substandard implementations
out there, though.  Hopefully the buildfarm will alert us to any
problems.

> Of course that may all be moot -- those functions may in fact be
> available everywhere we care about, but it was interesting to play
> around with them anyway.

Yeah, math functions are fun to play around with ... and we could end
up needing the code.  We'll see.

            regards, tom lane


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

From
Tom Lane
Date:
=?UTF-8?Q?L=C3=A6titia_Avrot?= <laetitia.avrot@gmail.com> writes:
> 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).

Pushed with some minor adjustments (mainly cleanup of the error handling).

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

Yeah, I agree that sech() and so on are not worth the trouble.  If they
were commonly used, they'd be in POSIX ...

            regards, tom lane


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

From
Lætitia Avrot
Date:
Thanks,  Tom ! 

Thank you everyone for your help and patience. 

Cheers, 

Lætitia 

Le mar. 12 mars 2019 à 20:57, Tom Lane <tgl@sss.pgh.pa.us> a écrit :
Lætitia Avrot <laetitia.avrot@gmail.com> writes:
> 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).

Pushed with some minor adjustments (mainly cleanup of the error handling).

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

Yeah, I agree that sech() and so on are not worth the trouble.  If they
were commonly used, they'd be in POSIX ...

                        regards, tom lane

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

From
Darafei "Komяpa" Praliaskouski
Date:
I really appreciate the addition of tanh into core postgres.

If someone doubts it is useful: it is used as a part of math in geographical calculations. 

Say you have your cars in planar Mercator projection and want to move them "1 second forward by this heading with this speed". sin/cos and the distance on X/Y, but the distance must be scaled properly - and that scaling coefficient is cosd(latitude), which you don't have directly - you have it in projected meters. If you don't want to fire up full-featured PostGIS on this hot path you inline all formulas together, result is nice and small - but has tanh in it, which I was surprised to find only in Oracle Compatibility extensions. Pure sql tanh was good enough, but gave me disturbance :)

On Wed, Mar 13, 2019 at 5:34 PM Lætitia Avrot <laetitia.avrot@gmail.com> wrote:
Thanks,  Tom ! 

Thank you everyone for your help and patience. 

Cheers, 

Lætitia 

Le mar. 12 mars 2019 à 20:57, Tom Lane <tgl@sss.pgh.pa.us> a écrit :
Lætitia Avrot <laetitia.avrot@gmail.com> writes:
> 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).

Pushed with some minor adjustments (mainly cleanup of the error handling).

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

Yeah, I agree that sech() and so on are not worth the trouble.  If they
were commonly used, they'd be in POSIX ...

                        regards, tom lane


--
Darafei Praliaskouski