[SQL] Add trigonometric functions sin() and cos() #1118
Conversation
* Also changes the precision to which floats display Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>
abhizer
added
enhancement
Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>
| @@ -84,4 +84,12 @@ REAL '1.23' -- string style | |||
| <td><code>LOG10(value)</code></td> | |||
| <td>The logarithm base 10 of value. Produces a runtime error for values less than or equal to zero.</td> | |||
| </tr> | |||
| <tr> | |||
| <td><code>SIN(value)</code></td> | |||
There was a problem hiding this comment.
You have to document the fact that the only type supported is double, and everything else is cast to double, and also that the result is always double. We will have to revisit the other functions, like ln too. Maybe you can file an issue about that.
crates/dbsp/src/algebra/floats.rs
Outdated
| @@ -361,7 +364,9 @@ macro_rules! float { | |||
| impl Display for $outer { | |||
| #[rustfmt::skip] | |||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
| Display::fmt(&self.0.0, f) | |||
| let num = format!("{1:.0$}", FLOAT_DISPLAY_PRECISION, &self.0.0); | |||
There was a problem hiding this comment.
Let's do this in the cast to string method, which is in sqllib/src/casts.rs
| case "cos": | ||
| { | ||
| // Turn the argument into Double | ||
| ops.set(0, ops.get(0).cast(new DBSPTypeDouble(node, false))); |
There was a problem hiding this comment.
The false here is not correct, it indicates that the value cannot be null.
You should instead get the current type of the operand and copy its mayBeNull value.
This also proves that there is no test for NULL as an argument, so add that too.
| @Test | ||
| public void testSin() { | ||
| this.qs( | ||
| "SELECT sin(0);\n" + |
There was a problem hiding this comment.
All these values are decimal values.
You probably should write the same test with double values as arguments.
And replace 3.14 with pi when we have implemented that.
Maybe you can push another commit in this PR for PI.
There was a problem hiding this comment.
you should also document where the tests are coming from. For example "tested using Postgres".
| @@ -889,6 +889,24 @@ some_polymorphic_function1!(sign, f, F32, F32); | |||
| some_polymorphic_function1!(sign, d, F64, F64); | |||
| some_polymorphic_function1!(sign, decimal, Decimal, Decimal); | |||
|
|
|||
| ///////////////////// sin ////////////////////// | |||
There was a problem hiding this comment.
this many comments are overkill.
Just add one comment for all trigonometric functions
* Accept null argument in sin and cos * Limit max precision to 6 and 15 decimal places for float and double respectively when casting to string * Update the tests including a test for null argument * Update the documentation of the functions Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>
| @Test | ||
| public void testSinDouble() { | ||
| this.qs( | ||
| "SELECT sin(CAST(0 AS DOUBLE));\n" + |
There was a problem hiding this comment.
this works, but you can get a double by writing e0 at the end. 3.14e0 is a double, while 3.14 is a decimal.
Fixes: #1094
Is this a user-visible change (yes/no): yes