Обсуждение: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance
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>).
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 log10(100);
select sinh(0);
select cosh(0);
select tanh(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.
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.
There is no Planet B.
Вложения
Hi,
> [ 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,
You'll find enclosed the new version of the patch.
Cheers,
Lætitia
Вложения
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
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.
There is no Planet B.
Вложения
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.
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.
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
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
Вложения
Hi Andrew and Tom,
I considered that option before writing my patch but I refrained for 2 reasons:
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,
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
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.
There is no Planet B.