mihaibudiu commented on code in PR #3862:
URL: https://github.com/apache/calcite/pull/3862#discussion_r1681374495
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3053,6 +3053,17 @@ public static double acos(double b0) {
return Math.acos(b0);
}
+ // ACOSD
+ /** SQL <code>ACOSD</code> operator applied to BigDecimal values. */
+ public static double acosd(BigDecimal b0) {
+ return Math.toDegrees(Math.acos(b0.doubleValue()));
Review Comment:
acos will supposedly throw if the value is out of range.
But the value of decimal can also be out of range for double, which will
cause doubleValue to throw, and you will get a different error depending on its
value.
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3078,6 +3089,17 @@ public static double asin(double b0) {
return Math.asin(b0);
}
+ // ASIND
Review Comment:
Frankly, the little comment is not particularly useful
##########
site/_docs/reference.md:
##########
@@ -2689,6 +2689,7 @@ In the following:
|:- |:-----------------------------------------------|:-----------
| p | expr :: type | Casts *expr* to *type*
| m | expr1 <=> expr2 | Whether two values are
equal, treating null values as the same, and it's similar to `IS NOT DISTINCT
FROM`
+| p | ACOSD(numeric) | Returns the inverse
cosine of *numeric* in degrees
Review Comment:
the behavior for out of range values should also be documented
##########
site/_docs/reference.md:
##########
@@ -2689,6 +2689,7 @@ In the following:
|:- |:-----------------------------------------------|:-----------
| p | expr :: type | Casts *expr* to *type*
| m | expr1 <=> expr2 | Whether two values are
equal, treating null values as the same, and it's similar to `IS NOT DISTINCT
FROM`
+| p | ACOSD(numeric) | Returns the inverse
cosine of *numeric* in degrees
Review Comment:
I personally would like it if the return type was documented as double
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -8628,6 +8628,27 @@ void checkArrayReverseFunc(SqlOperatorFixture f0,
SqlFunction function,
f.checkNull("acos(cast(null as double))");
}
+ @Test void testAcosdFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.POSTGRESQL);
+ f.setFor(SqlLibraryOperators.ACOSD, VmName.EXPAND);
+ f.checkType("acosd(0)", "DOUBLE NOT NULL");
+ f.checkType("acosd(cast(1 as float))", "DOUBLE NOT NULL");
+ f.checkType("acosd(case when false then 0.5 else null end)", "DOUBLE");
+ f.enableTypeCoercion(false)
+ .checkFails("^acosd('abc')^",
+ "Cannot apply 'ACOSD' to arguments of type "
+ + "'ACOSD\\(<CHAR\\(3\\)>\\)'\\. Supported form\\(s\\): "
+ + "'ACOSD\\(<NUMERIC>\\)'",
+ false);
+ f.checkType("acosd('abc')", "DOUBLE NOT NULL");
+ f.checkScalarApprox("acosd(0.5)", "DOUBLE NOT NULL",
+ isWithin(59.99d, 60.01d));
+ f.checkScalarApprox("acosd(cast(0.5 as decimal(3, 1)))", "DOUBLE NOT NULL",
+ isWithin(59.9d, 60.1d));
+ f.checkNull("acosd(cast(null as integer))");
+ f.checkNull("acosd(cast(null as double))");
Review Comment:
how about some tests with values outside the range -1..1?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
