You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/05/17 19:26:44 UTC
[GitHub] [arrow] anthonylouisbsb opened a new pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
anthonylouisbsb opened a new pull request #10350:
URL: https://github.com/apache/arrow/pull/10350
Implements the `ABS`, `FLOOR`, `PI`, `SQRT`, `SIGN`, `LSHIFT`, `RSHIFT`, and `TRUNC`.
The [RANDOM functions](https://github.com/apache/arrow/blob/master/cpp/src/gandiva/random_generator_holder.h) are already implemented in Gandiva.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r638959942
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -290,4 +299,53 @@ TEST(TestGdvFnStubs, TestCastVARCHARFromDouble) {
EXPECT_FALSE(ctx.has_error());
}
+TEST(TestGdvFnStubs, TestAbs) {
+ gandiva::ExecutionContext ctx;
+ uint64_t ctx_ptr = reinterpret_cast<int64_t>(&ctx);
+
+ // Abs functions
+ EXPECT_EQ(gdv_fn_abs_int32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_uint32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, 0), 0L);
+ EXPECT_EQ(gdv_fn_abs_uint64(ctx_ptr, 0), 0L);
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, 0.0f), abs(0.0f));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, 0.0), abs(0.0));
+
+ VerifyAlmostEquals(gdv_fn_abs_int32(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_int64(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, static_cast<float>(INT32_MIN + 1)),
+ abs(static_cast<float>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, (INT64_MIN + 1)),
Review comment:
Sorry, I thought the previous version of abs is to cast int64 to double long. But, now, `gdv_fn_abs_int64` handles as int64. Correct?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-1020984593
I would rather if a Gandiva developer vetted this PR. @pravindra perhaps?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r639275965
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -290,4 +299,53 @@ TEST(TestGdvFnStubs, TestCastVARCHARFromDouble) {
EXPECT_FALSE(ctx.has_error());
}
+TEST(TestGdvFnStubs, TestAbs) {
+ gandiva::ExecutionContext ctx;
+ uint64_t ctx_ptr = reinterpret_cast<int64_t>(&ctx);
+
+ // Abs functions
+ EXPECT_EQ(gdv_fn_abs_int32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_uint32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, 0), 0L);
+ EXPECT_EQ(gdv_fn_abs_uint64(ctx_ptr, 0), 0L);
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, 0.0f), abs(0.0f));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, 0.0), abs(0.0));
+
+ VerifyAlmostEquals(gdv_fn_abs_int32(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_int64(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, static_cast<float>(INT32_MIN + 1)),
+ abs(static_cast<float>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, (INT64_MIN + 1)),
Review comment:
@kiszk Yes, [this line](https://github.com/apache/arrow/blob/aeea171560efb33cdbfbc47f8d85f7ee863cefda/cpp/src/gandiva/gdv_function_stubs.cc#L696) has the changes I made in the function. I handle the number as int64 and I add a check similar to the compute kernel to avoid overflow when getting the absolute value for the `INT64_MIN` value.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-1020984593
I would rather if a Gandiva developer vetted this PR. @pravindra perhaps?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-865104198
It would certainly be desirable to have the same semantics in Gandiva and in Arrow compute kernels.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r637712623
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -212,6 +230,32 @@ ENUMERIC_TYPES_UNARY(RADIANS, float64)
}
ENUMERIC_TYPES_UNARY(DEGREES, float64)
+// Abs
+#define ABS(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE abs_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##IN_TYPE>(abs(static_cast<long double>(in))); \
Review comment:
@kiszk I checked that by the `IEE-754` convention, the numbers in double precision [should use at least 8 bytes](https://en.wikipedia.org/wiki/IEEE_754) to construct their representation, so I think that there would not have any digit loss in conversion.
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -273,4 +324,137 @@ TEST(TestExtendedMathOps, TestTrigonometricFunctions) {
VerifyFuzzyEquals(cot_float64(M_PI / 2), tan(M_PI / 2 - M_PI / 2));
}
+TEST(TestExtendedMathOps, TestAbs) {
+ // Abs functions
+ VerifyFuzzyEquals(abs_int32(0), abs(0));
+ VerifyFuzzyEquals(abs_int64(0), abs(0));
+ VerifyFuzzyEquals(abs_float32(0), abs(0));
+ VerifyFuzzyEquals(abs_float64(0), abs(0));
+
+ VerifyFuzzyEquals(abs_int32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_int64(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float64(-5), abs(-5));
+
+ VerifyFuzzyEquals(abs_int32(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_int64(-2371041), abs(-2371041));
Review comment:
Test added
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r654332132
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -484,4 +493,53 @@ TEST(TestGdvFnStubs, TestInitCap) {
"unexpected byte \\e0 encountered while decoding utf8 string"));
ctx.Reset();
}
+
+TEST(TestGdvFnStubs, TestAbs) {
+ gandiva::ExecutionContext ctx;
+ uint64_t ctx_ptr = reinterpret_cast<int64_t>(&ctx);
+
+ // Abs functions
+ EXPECT_EQ(gdv_fn_abs_int32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_uint32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, 0), 0L);
+ EXPECT_EQ(gdv_fn_abs_uint64(ctx_ptr, 0), 0L);
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, 0.0f), abs(0.0f));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, 0.0), abs(0.0));
+
+ VerifyAlmostEquals(gdv_fn_abs_int32(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_int64(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, static_cast<float>(INT32_MIN + 1)),
+ abs(static_cast<float>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, (INT64_MIN + 1)),
+ abs(static_cast<double>(INT64_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT64_MIN + 1)),
+ abs(static_cast<double>(INT64_MIN + 1)));
+
Review comment:
How about adding tests for min and max values of double and float by using `std::numeric_limits`?
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -273,4 +333,142 @@ TEST(TestExtendedMathOps, TestTrigonometricFunctions) {
VerifyFuzzyEquals(cot_float64(M_PI / 2), tan(M_PI / 2 - M_PI / 2));
}
+TEST(TestExtendedMathOps, TestCeil) {
+ // Ceil functions
+ VerifyFuzzyEquals(ceil_float32(0), ceil(0));
+ VerifyFuzzyEquals(ceil_float64(0), ceil(0));
+
+ VerifyFuzzyEquals(ceil_float32(-5), ceil(-5));
+ VerifyFuzzyEquals(ceil_float64(-5), ceil(-5));
+
+ VerifyFuzzyEquals(ceil_float32(-2371041), ceil(-2371041));
+ VerifyFuzzyEquals(ceil_float64(-2371041), ceil(-2371041));
+
+ VerifyFuzzyEquals(ceil_float32(5.45f), ceil(5.45));
+ VerifyFuzzyEquals(ceil_float64(5.45), ceil(5.45));
+
+ VerifyFuzzyEquals(ceil_float32(-3600.50f), ceil(-3600.50));
+ VerifyFuzzyEquals(ceil_float64(-3600.50), ceil(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestFloor) {
+ // Ceil functions
+ VerifyFuzzyEquals(floor_float32(0), floor(0));
+ VerifyFuzzyEquals(floor_float64(0), floor(0));
+
+ VerifyFuzzyEquals(floor_float32(-5), floor(-5));
+ VerifyFuzzyEquals(floor_float64(-5), floor(-5));
+
+ VerifyFuzzyEquals(floor_float32(-2371041), floor(-2371041));
+ VerifyFuzzyEquals(floor_float64(-2371041), floor(-2371041));
+
+ VerifyFuzzyEquals(floor_float32(5.45f), floor(5.45));
+ VerifyFuzzyEquals(floor_float64(5.45), floor(5.45));
+
+ VerifyFuzzyEquals(floor_float32(-3600.50f), floor(-3600.50));
+ VerifyFuzzyEquals(floor_float64(-3600.50), floor(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestConstants) {
+ // Constants functions
+ VerifyFuzzyEquals(pi(), M_PI);
+ VerifyFuzzyEquals(e(), exp(1.0));
+}
+
+TEST(TestExtendedMathOps, TestSqrt) {
+ // Sqrt functions
+ VerifyFuzzyEquals(sqrt_int32(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_int64(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_float32(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_float64(0), sqrt(0));
+
+ VerifyFuzzyEquals(sqrt_int32(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_int64(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_float32(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_float64(5), sqrt(5));
+
+ VerifyFuzzyEquals(sqrt_int32(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_int64(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_float32(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_float64(2371041), sqrt(2371041));
+
+ VerifyFuzzyEquals(sqrt_float32(3600.50f), sqrt(3600.50));
+ VerifyFuzzyEquals(sqrt_float64(3600.50), sqrt(3600.50));
+}
+
+TEST(TestExtendedMathOps, TestSign) {
+ // Sqrt functions
+ EXPECT_EQ(sign_int32(0), 0);
+ EXPECT_EQ(sign_int64(0), 0);
+ EXPECT_EQ(sign_float32(0), 0);
+ EXPECT_EQ(sign_float64(0), 0);
+
+ EXPECT_EQ(sign_int32(5), 1);
+ EXPECT_EQ(sign_int64(5), 1);
+ EXPECT_EQ(sign_float32(5), 1);
+ EXPECT_EQ(sign_float64(5), 1);
+
+ EXPECT_EQ(sign_int32(2371041), 1);
+ EXPECT_EQ(sign_int64(2371041), 1);
+ EXPECT_EQ(sign_float32(2371041), 1);
+ EXPECT_EQ(sign_float64(2371041), 1);
+
+ EXPECT_EQ(sign_int32(-3600), -1);
+ EXPECT_EQ(sign_int64(-3600), -1);
+ EXPECT_EQ(sign_float32(-3600.50f), -1);
+ EXPECT_EQ(sign_float64(-3600.50), -1);
+}
+
+TEST(TestExtendedMathOps, TestLshiftRshift) {
Review comment:
How about adding tests with large shift values?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-844033861
#10274 handles abs operation well.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r635046384
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -273,4 +324,137 @@ TEST(TestExtendedMathOps, TestTrigonometricFunctions) {
VerifyFuzzyEquals(cot_float64(M_PI / 2), tan(M_PI / 2 - M_PI / 2));
}
+TEST(TestExtendedMathOps, TestAbs) {
+ // Abs functions
+ VerifyFuzzyEquals(abs_int32(0), abs(0));
+ VerifyFuzzyEquals(abs_int64(0), abs(0));
+ VerifyFuzzyEquals(abs_float32(0), abs(0));
+ VerifyFuzzyEquals(abs_float64(0), abs(0));
+
+ VerifyFuzzyEquals(abs_int32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_int64(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float64(-5), abs(-5));
+
+ VerifyFuzzyEquals(abs_int32(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_int64(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_float32(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_float64(-2371041), abs(-2371041));
+
+ VerifyFuzzyEquals(abs_int32(-3600.50), abs(-3600));
+ VerifyFuzzyEquals(abs_int64(-3600.50), abs(-3600));
+ VerifyFuzzyEquals(abs_float32(-3600.50), abs(-3600.50));
+ VerifyFuzzyEquals(abs_float64(-3600.50), abs(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestCeil) {
+ // Ceil functions
+ VerifyFuzzyEquals(ceil_float32(0), ceil(0));
+ VerifyFuzzyEquals(ceil_float64(0), ceil(0));
+
+ VerifyFuzzyEquals(ceil_float32(-5), ceil(-5));
+ VerifyFuzzyEquals(ceil_float64(-5), ceil(-5));
+
+ VerifyFuzzyEquals(ceil_float32(-2371041), ceil(-2371041));
+ VerifyFuzzyEquals(ceil_float64(-2371041), ceil(-2371041));
+
+ VerifyFuzzyEquals(ceil_float32(5.45), ceil(5.45));
+ VerifyFuzzyEquals(ceil_float64(5.45), ceil(5.45));
+
+ VerifyFuzzyEquals(ceil_float32(-3600.50), ceil(-3600.50));
+ VerifyFuzzyEquals(ceil_float64(-3600.50), ceil(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestFloor) {
+ // Ceil functions
+ VerifyFuzzyEquals(floor_float32(0), floor(0));
+ VerifyFuzzyEquals(floor_float64(0), floor(0));
+
+ VerifyFuzzyEquals(floor_float32(-5), floor(-5));
+ VerifyFuzzyEquals(floor_float64(-5), floor(-5));
+
+ VerifyFuzzyEquals(floor_float32(-2371041), floor(-2371041));
+ VerifyFuzzyEquals(floor_float64(-2371041), floor(-2371041));
+
+ VerifyFuzzyEquals(floor_float32(5.45), floor(5.45));
+ VerifyFuzzyEquals(floor_float64(5.45), floor(5.45));
+
+ VerifyFuzzyEquals(floor_float32(-3600.50), floor(-3600.50));
+ VerifyFuzzyEquals(floor_float64(-3600.50), floor(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestConstants) {
+ // Constants functions
+ VerifyFuzzyEquals(pi(), M_PI);
+ VerifyFuzzyEquals(e(), exp(1.0));
+}
+
+TEST(TestExtendedMathOps, TestSqrt) {
+ // Sqrt functions
+ VerifyFuzzyEquals(sqrt_int32(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_int64(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_float32(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_float64(0), sqrt(0));
+
+ VerifyFuzzyEquals(sqrt_int32(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_int64(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_float32(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_float64(5), sqrt(5));
+
+ VerifyFuzzyEquals(sqrt_int32(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_int64(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_float32(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_float64(2371041), sqrt(2371041));
+
+ VerifyFuzzyEquals(sqrt_int32(3600.50), sqrt(3600));
+ VerifyFuzzyEquals(sqrt_int64(3600.50), sqrt(3600));
+ VerifyFuzzyEquals(sqrt_float32(3600.50), sqrt(3600.50));
+ VerifyFuzzyEquals(sqrt_float64(3600.50), sqrt(3600.50));
+}
+
+TEST(TestExtendedMathOps, TestSign) {
+ // Sqrt functions
+ EXPECT_EQ(sign_int32(0), 0);
+ EXPECT_EQ(sign_int64(0), 0);
+ EXPECT_EQ(sign_float32(0), 0);
+ EXPECT_EQ(sign_float64(0), 0);
+
+ EXPECT_EQ(sign_int32(5), 1);
+ EXPECT_EQ(sign_int64(5), 1);
+ EXPECT_EQ(sign_float32(5), 1);
+ EXPECT_EQ(sign_float64(5), 1);
+
+ EXPECT_EQ(sign_int32(2371041), 1);
+ EXPECT_EQ(sign_int64(2371041), 1);
+ EXPECT_EQ(sign_float32(2371041), 1);
+ EXPECT_EQ(sign_float64(2371041), 1);
+
+ EXPECT_EQ(sign_int32(-3600.50), -1);
+ EXPECT_EQ(sign_int64(-3600.50), -1);
+ EXPECT_EQ(sign_float32(-3600.50), -1);
+ EXPECT_EQ(sign_float64(-3600.50), -1);
+}
+
+TEST(TestExtendedMathOps, TestLshiftRshift) {
+ // Lshift functions
+ EXPECT_EQ(lshift_int32(0, 1), 0 << 1);
+ EXPECT_EQ(lshift_int64(0, 1), 0 << 1);
+
+ EXPECT_EQ(lshift_int32(5, 20), 5 << 20);
+ EXPECT_EQ(lshift_int64(5, 20), 5 << 20);
+
+ EXPECT_EQ(lshift_int32(1, 20), 1 << 20);
+ EXPECT_EQ(lshift_int64(1, 20), 1 << 20);
+
+ // Rshift functions
+ EXPECT_EQ(rshift_int32(0, 1), 0 >> 1);
+ EXPECT_EQ(rshift_int64(0, 1), 0 >> 1);
+
+ EXPECT_EQ(rshift_int32(5, 20), 5 >> 20);
+ EXPECT_EQ(rshift_int64(5, 20), 5 >> 20);
+
+ EXPECT_EQ(rshift_int32(1, 20), 1 >> 20);
+ EXPECT_EQ(rshift_int64(1, 20), 1 >> 20);
+}
Review comment:
It would be worth adding tests for rshift with a negative value.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-865104198
It would certainly be desirable to have the same semantics in Gandiva and in Arrow compute kernels.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r637037791
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -290,4 +299,53 @@ TEST(TestGdvFnStubs, TestCastVARCHARFromDouble) {
EXPECT_FALSE(ctx.has_error());
}
+TEST(TestGdvFnStubs, TestAbs) {
+ gandiva::ExecutionContext ctx;
+ uint64_t ctx_ptr = reinterpret_cast<int64_t>(&ctx);
+
+ // Abs functions
+ EXPECT_EQ(gdv_fn_abs_int32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_uint32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, 0), 0L);
+ EXPECT_EQ(gdv_fn_abs_uint64(ctx_ptr, 0), 0L);
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, 0.0f), abs(0.0f));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, 0.0), abs(0.0));
+
+ VerifyAlmostEquals(gdv_fn_abs_int32(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_int64(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, static_cast<float>(INT32_MIN + 1)),
+ abs(static_cast<float>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, (INT64_MIN + 1)),
Review comment:
@kiszk Thanks for your revision! Please, could you explain how would occur digit loss here? I did not understand what you said.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC and LN functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-852402117
The `LN` function was added in this PR too.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r720702352
##########
File path: cpp/src/gandiva/function_registry_math_ops.cc
##########
@@ -16,6 +16,7 @@
// under the License.
#include "gandiva/function_registry_math_ops.h"
+
Review comment:
nit: we can remove this line
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r671183835
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -20,10 +20,19 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <cmath>
+
#include "gandiva/execution_context.h"
namespace gandiva {
+static const double DEFAULT_THRESHOLD = 0.00005;
Review comment:
I will change the method to use the `FLT_EPSILON` from the macro
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r637720694
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -220,6 +264,51 @@ ENUMERIC_TYPES_UNARY(DEGREES, float64)
}
POWER(float64, float64, float64)
+// Sqrt
+#define SQRT(IN_TYPE, OUT_TYPE) \
+ FORCE_INLINE \
+ gdv_##OUT_TYPE sqrt_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##OUT_TYPE>(sqrt(static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES_UNARY(SQRT, float64)
+
+// Sign
+#define SIGN(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE sign_##IN_TYPE(gdv_##IN_TYPE in) { \
+ if (in == 0.0) return in; \
+ return static_cast<gdv_##IN_TYPE>(copysign(1.0, static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES(SIGN)
+
+// Lshift
+#define LSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE lshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 << in2); \
+ }
+LSHIFT(int32)
+LSHIFT(int64)
+LSHIFT(uint32)
+LSHIFT(uint64)
+
+// Lshift
+#define RSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE rshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 >> in2); \
Review comment:
@kiszk Thanks for your revision, I checked the [C++ reference](https://en.cppreference.com/w/cpp/language/operator_arithmetic), and I noticed that the shift operators have undefined or implementation-choice behavior for signed and negative numbers.
So as in this Pull Request, we are implementing the `Arithmetical Shift`, we can cast the input parameters to their respective `unsigned` types and then apply the shift operators and they will have the same behavior in all platforms. Do you agree with the solution?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r798088243
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -384,19 +493,61 @@ gdv_int64 get_power_of_10(gdv_int32 exp) {
return power_of_10[exp];
}
-FORCE_INLINE
-gdv_int64 truncate_int64_int32(gdv_int64 in, gdv_int32 out_scale) {
- bool overflow = false;
- arrow::BasicDecimal128 decimal = gandiva::decimalops::FromInt64(in, 38, 0, &overflow);
- arrow::BasicDecimal128 decimal_with_outscale =
- gandiva::decimalops::Truncate(gandiva::BasicDecimalScalar128(decimal, 38, 0), 38,
- out_scale, out_scale, &overflow);
- if (out_scale < 0) {
- out_scale = 0;
- }
- return gandiva::decimalops::ToInt64(
- gandiva::BasicDecimalScalar128(decimal_with_outscale, 38, out_scale), &overflow);
-}
+#define TRUNCATE_INTEGER(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE truncate_##TYPE(gdv_##TYPE value) { return value; }
+
+#define TRUNCATE_INTEGER_WITH_OUT_SCALE(TYPE) \
+ FORCE_INLINE \
Review comment:
@vvellanki I add a check for that case and it will throw an error if that situation occurs, check that commit: https://github.com/apache/arrow/pull/10350/commits/d25a630ed388e244b4414affb99554f05efe15cc
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb edited a comment on pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb edited a comment on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-845101610
I included the `CEIL` function in the title because it was implemented too in Pull Request
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r671183835
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -20,10 +20,19 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <cmath>
+
#include "gandiva/execution_context.h"
namespace gandiva {
+static const double DEFAULT_THRESHOLD = 0.00005;
Review comment:
I will change the method to use the `DBL_EPSILON` from the macro
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r637712786
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -220,6 +264,51 @@ ENUMERIC_TYPES_UNARY(DEGREES, float64)
}
POWER(float64, float64, float64)
+// Sqrt
+#define SQRT(IN_TYPE, OUT_TYPE) \
+ FORCE_INLINE \
+ gdv_##OUT_TYPE sqrt_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##OUT_TYPE>(sqrt(static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES_UNARY(SQRT, float64)
+
+// Sign
+#define SIGN(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE sign_##IN_TYPE(gdv_##IN_TYPE in) { \
+ if (in == 0.0) return in; \
+ return static_cast<gdv_##IN_TYPE>(copysign(1.0, static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES(SIGN)
+
+// Lshift
+#define LSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE lshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 << in2); \
+ }
+LSHIFT(int32)
+LSHIFT(int64)
+LSHIFT(uint32)
+LSHIFT(uint64)
+
+// Lshift
Review comment:
The definition was added
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r671183620
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -20,10 +20,19 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <cmath>
+
#include "gandiva/execution_context.h"
namespace gandiva {
+static const double DEFAULT_THRESHOLD = 0.00005;
Review comment:
I think choosing an adequate does not have a simple and unique answer like is shown in these articles:
- https://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html
- https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/
I choose that number because it is the value defined in the [float header as default threshold](https://www.cplusplus.com/reference/cfloat/) . For huge and small numbers, I think the developer should set the threshold manually when he is calling the method.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-865117615
@pitrou @kiszk I will check the reference that you sent in Arrow Compute and I will try to apply the same semantic in this Pull Request
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r670455687
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -273,4 +333,142 @@ TEST(TestExtendedMathOps, TestTrigonometricFunctions) {
VerifyFuzzyEquals(cot_float64(M_PI / 2), tan(M_PI / 2 - M_PI / 2));
}
+TEST(TestExtendedMathOps, TestCeil) {
+ // Ceil functions
+ VerifyFuzzyEquals(ceil_float32(0), ceil(0));
+ VerifyFuzzyEquals(ceil_float64(0), ceil(0));
+
+ VerifyFuzzyEquals(ceil_float32(-5), ceil(-5));
+ VerifyFuzzyEquals(ceil_float64(-5), ceil(-5));
+
+ VerifyFuzzyEquals(ceil_float32(-2371041), ceil(-2371041));
+ VerifyFuzzyEquals(ceil_float64(-2371041), ceil(-2371041));
+
+ VerifyFuzzyEquals(ceil_float32(5.45f), ceil(5.45));
+ VerifyFuzzyEquals(ceil_float64(5.45), ceil(5.45));
+
+ VerifyFuzzyEquals(ceil_float32(-3600.50f), ceil(-3600.50));
+ VerifyFuzzyEquals(ceil_float64(-3600.50), ceil(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestFloor) {
+ // Ceil functions
+ VerifyFuzzyEquals(floor_float32(0), floor(0));
+ VerifyFuzzyEquals(floor_float64(0), floor(0));
+
+ VerifyFuzzyEquals(floor_float32(-5), floor(-5));
+ VerifyFuzzyEquals(floor_float64(-5), floor(-5));
+
+ VerifyFuzzyEquals(floor_float32(-2371041), floor(-2371041));
+ VerifyFuzzyEquals(floor_float64(-2371041), floor(-2371041));
+
+ VerifyFuzzyEquals(floor_float32(5.45f), floor(5.45));
+ VerifyFuzzyEquals(floor_float64(5.45), floor(5.45));
+
+ VerifyFuzzyEquals(floor_float32(-3600.50f), floor(-3600.50));
+ VerifyFuzzyEquals(floor_float64(-3600.50), floor(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestConstants) {
+ // Constants functions
+ VerifyFuzzyEquals(pi(), M_PI);
+ VerifyFuzzyEquals(e(), exp(1.0));
+}
+
+TEST(TestExtendedMathOps, TestSqrt) {
+ // Sqrt functions
+ VerifyFuzzyEquals(sqrt_int32(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_int64(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_float32(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_float64(0), sqrt(0));
+
+ VerifyFuzzyEquals(sqrt_int32(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_int64(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_float32(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_float64(5), sqrt(5));
+
+ VerifyFuzzyEquals(sqrt_int32(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_int64(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_float32(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_float64(2371041), sqrt(2371041));
+
+ VerifyFuzzyEquals(sqrt_float32(3600.50f), sqrt(3600.50));
+ VerifyFuzzyEquals(sqrt_float64(3600.50), sqrt(3600.50));
+}
+
+TEST(TestExtendedMathOps, TestSign) {
+ // Sqrt functions
+ EXPECT_EQ(sign_int32(0), 0);
+ EXPECT_EQ(sign_int64(0), 0);
+ EXPECT_EQ(sign_float32(0), 0);
+ EXPECT_EQ(sign_float64(0), 0);
+
+ EXPECT_EQ(sign_int32(5), 1);
+ EXPECT_EQ(sign_int64(5), 1);
+ EXPECT_EQ(sign_float32(5), 1);
+ EXPECT_EQ(sign_float64(5), 1);
+
+ EXPECT_EQ(sign_int32(2371041), 1);
+ EXPECT_EQ(sign_int64(2371041), 1);
+ EXPECT_EQ(sign_float32(2371041), 1);
+ EXPECT_EQ(sign_float64(2371041), 1);
+
+ EXPECT_EQ(sign_int32(-3600), -1);
+ EXPECT_EQ(sign_int64(-3600), -1);
+ EXPECT_EQ(sign_float32(-3600.50f), -1);
+ EXPECT_EQ(sign_float64(-3600.50), -1);
+}
+
+TEST(TestExtendedMathOps, TestLshiftRshift) {
Review comment:
Tests added
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-847475275
@kiszk I applied all suggestions you made, could you check if the corrections are fine? Thanks in advance for the review!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-845101610
I included the `CEIL` function in the title because it was implemented too.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r639275965
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -290,4 +299,53 @@ TEST(TestGdvFnStubs, TestCastVARCHARFromDouble) {
EXPECT_FALSE(ctx.has_error());
}
+TEST(TestGdvFnStubs, TestAbs) {
+ gandiva::ExecutionContext ctx;
+ uint64_t ctx_ptr = reinterpret_cast<int64_t>(&ctx);
+
+ // Abs functions
+ EXPECT_EQ(gdv_fn_abs_int32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_uint32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, 0), 0L);
+ EXPECT_EQ(gdv_fn_abs_uint64(ctx_ptr, 0), 0L);
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, 0.0f), abs(0.0f));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, 0.0), abs(0.0));
+
+ VerifyAlmostEquals(gdv_fn_abs_int32(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_int64(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, static_cast<float>(INT32_MIN + 1)),
+ abs(static_cast<float>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, (INT64_MIN + 1)),
Review comment:
@kiszk Yes, [this line](https://github.com/apache/arrow/blob/aeea171560efb33cdbfbc47f8d85f7ee863cefda/cpp/src/gandiva/gdv_function_stubs.cc#L696) has the new version of the function. I handle the number as int64 and I add a check similar to the compute kernel to avoid overflow when getting the absolute value for the `INT64_MIN` value.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r637720694
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -220,6 +264,51 @@ ENUMERIC_TYPES_UNARY(DEGREES, float64)
}
POWER(float64, float64, float64)
+// Sqrt
+#define SQRT(IN_TYPE, OUT_TYPE) \
+ FORCE_INLINE \
+ gdv_##OUT_TYPE sqrt_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##OUT_TYPE>(sqrt(static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES_UNARY(SQRT, float64)
+
+// Sign
+#define SIGN(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE sign_##IN_TYPE(gdv_##IN_TYPE in) { \
+ if (in == 0.0) return in; \
+ return static_cast<gdv_##IN_TYPE>(copysign(1.0, static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES(SIGN)
+
+// Lshift
+#define LSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE lshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 << in2); \
+ }
+LSHIFT(int32)
+LSHIFT(int64)
+LSHIFT(uint32)
+LSHIFT(uint64)
+
+// Lshift
+#define RSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE rshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 >> in2); \
Review comment:
@kiszk Thanks for your revision, I checked the [C++ reference](https://en.cppreference.com/w/cpp/language/operator_arithmetic), and I noticed that the shift operators have undefined or implementation-choice behavior for signed and negative numbers.
So as in this Pull Request, we are implementing the `Logical Shift`, we can cast the input parameters to their respective `unsigned` types and then apply the shift operators and they will have the same behavior in all platforms. Do you agree with the solution?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-865185500
@lidavidm thanks for the reference about the shift operators in ARM architecture.
I used the [Java implementation](https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-ShiftExpression) as a base for the `SHIFT` behavior.
I choose it just to keep the compatibility between the languages, but I can change to match the same specifications that the Compute module.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-1020750278
@pitrou can you take a look at it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-842574833
https://issues.apache.org/jira/browse/ARROW-12814
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r635030461
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -212,6 +230,32 @@ ENUMERIC_TYPES_UNARY(RADIANS, float64)
}
ENUMERIC_TYPES_UNARY(DEGREES, float64)
+// Abs
+#define ABS(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE abs_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##IN_TYPE>(abs(static_cast<long double>(in))); \
Review comment:
I am curious whether all platforms can convert 64-bit integer to `long double` without loss of digits. (i.e. size(long double) > size(int64)).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r670455134
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -484,4 +493,53 @@ TEST(TestGdvFnStubs, TestInitCap) {
"unexpected byte \\e0 encountered while decoding utf8 string"));
ctx.Reset();
}
+
+TEST(TestGdvFnStubs, TestAbs) {
+ gandiva::ExecutionContext ctx;
+ uint64_t ctx_ptr = reinterpret_cast<int64_t>(&ctx);
+
+ // Abs functions
+ EXPECT_EQ(gdv_fn_abs_int32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_uint32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, 0), 0L);
+ EXPECT_EQ(gdv_fn_abs_uint64(ctx_ptr, 0), 0L);
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, 0.0f), abs(0.0f));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, 0.0), abs(0.0));
+
+ VerifyAlmostEquals(gdv_fn_abs_int32(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_int64(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, static_cast<float>(INT32_MIN + 1)),
+ abs(static_cast<float>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, (INT64_MIN + 1)),
+ abs(static_cast<double>(INT64_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT64_MIN + 1)),
+ abs(static_cast<double>(INT64_MIN + 1)));
+
Review comment:
Tests added
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-880693100
@kiszk @pitrou and @lidavidm I follow your suggestions and I changed the shift functions to have the same behavior as the Compute module. Thanks for your help!
@kiszk I also applied your suggestions to add more cases in the tests, when you be available, could you take another look in the changes?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] augustoasilva commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r670892726
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -212,6 +239,24 @@ ENUMERIC_TYPES_UNARY(RADIANS, float64)
}
ENUMERIC_TYPES_UNARY(DEGREES, float64)
+// Ceil
+#define CEIL(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE ceil_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##IN_TYPE>(ceil(static_cast<long double>(in))); \
+ }
+CEIL(float32)
+CEIL(float64)
+
+// Ceil
Review comment:
I think this comment was wrong, please fix it.
```suggestion
// Floor```
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -20,10 +20,19 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <cmath>
+
#include "gandiva/execution_context.h"
namespace gandiva {
+static const double DEFAULT_THRESHOLD = 0.00005;
Review comment:
Why did you choose this threshold? On fabs documentation it uses 6 places for decimal part, shouldn't it be **0.000005** a better value for AlmostEquals threshold?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r637720694
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -220,6 +264,51 @@ ENUMERIC_TYPES_UNARY(DEGREES, float64)
}
POWER(float64, float64, float64)
+// Sqrt
+#define SQRT(IN_TYPE, OUT_TYPE) \
+ FORCE_INLINE \
+ gdv_##OUT_TYPE sqrt_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##OUT_TYPE>(sqrt(static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES_UNARY(SQRT, float64)
+
+// Sign
+#define SIGN(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE sign_##IN_TYPE(gdv_##IN_TYPE in) { \
+ if (in == 0.0) return in; \
+ return static_cast<gdv_##IN_TYPE>(copysign(1.0, static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES(SIGN)
+
+// Lshift
+#define LSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE lshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 << in2); \
+ }
+LSHIFT(int32)
+LSHIFT(int64)
+LSHIFT(uint32)
+LSHIFT(uint64)
+
+// Lshift
+#define RSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE rshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 >> in2); \
Review comment:
@kiszk Thanks for your revision, I checked the [C++ reference](https://en.cppreference.com/w/cpp/language/operator_arithmetic), and I noticed that the shift operators have undefined or implementation-choice behavior for signed and negative numbers.
So as in this Pull Request, we are implementing the `Logical Shift`, we can cast the input parameters to their respective `unsigned` types and apply the shift operators and they will have the same behavior in all platforms. Do you agree with the solution?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r635032175
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -273,4 +324,137 @@ TEST(TestExtendedMathOps, TestTrigonometricFunctions) {
VerifyFuzzyEquals(cot_float64(M_PI / 2), tan(M_PI / 2 - M_PI / 2));
}
+TEST(TestExtendedMathOps, TestAbs) {
+ // Abs functions
+ VerifyFuzzyEquals(abs_int32(0), abs(0));
+ VerifyFuzzyEquals(abs_int64(0), abs(0));
+ VerifyFuzzyEquals(abs_float32(0), abs(0));
+ VerifyFuzzyEquals(abs_float64(0), abs(0));
+
+ VerifyFuzzyEquals(abs_int32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_int64(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float64(-5), abs(-5));
+
+ VerifyFuzzyEquals(abs_int32(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_int64(-2371041), abs(-2371041));
Review comment:
Can you add a test case using `abs_int64(LONG_MIN)`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk edited a comment on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
kiszk edited a comment on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-864323035
Regarding rshift and lshift, is it better to keep the same semantics as [this](https://github.com/apache/arrow/pull/10530)? cc @pitrou and @lidavidm
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r671195453
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -212,6 +239,24 @@ ENUMERIC_TYPES_UNARY(RADIANS, float64)
}
ENUMERIC_TYPES_UNARY(DEGREES, float64)
+// Ceil
+#define CEIL(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE ceil_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##IN_TYPE>(ceil(static_cast<long double>(in))); \
+ }
+CEIL(float32)
+CEIL(float64)
+
+// Ceil
Review comment:
Solved
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-976893632
@pitrou When you be free, could you take a look at this PR? I applied all changes that you and @kiszk suggested.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-1009394697
@pitrou can you take a look at this PR? Because I want to close it as soon as possible
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] vvellanki commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
vvellanki commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r795639530
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -384,19 +493,61 @@ gdv_int64 get_power_of_10(gdv_int32 exp) {
return power_of_10[exp];
}
-FORCE_INLINE
-gdv_int64 truncate_int64_int32(gdv_int64 in, gdv_int32 out_scale) {
- bool overflow = false;
- arrow::BasicDecimal128 decimal = gandiva::decimalops::FromInt64(in, 38, 0, &overflow);
- arrow::BasicDecimal128 decimal_with_outscale =
- gandiva::decimalops::Truncate(gandiva::BasicDecimalScalar128(decimal, 38, 0), 38,
- out_scale, out_scale, &overflow);
- if (out_scale < 0) {
- out_scale = 0;
- }
- return gandiva::decimalops::ToInt64(
- gandiva::BasicDecimalScalar128(decimal_with_outscale, 38, out_scale), &overflow);
-}
+#define TRUNCATE_INTEGER(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE truncate_##TYPE(gdv_##TYPE value) { return value; }
+
+#define TRUNCATE_INTEGER_WITH_OUT_SCALE(TYPE) \
+ FORCE_INLINE \
Review comment:
What should happen if there is overflow?
overflow is an in-out parameter.. for some of the later calls overflow could be false - will that cause an issue?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r635040464
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -220,6 +264,51 @@ ENUMERIC_TYPES_UNARY(DEGREES, float64)
}
POWER(float64, float64, float64)
+// Sqrt
+#define SQRT(IN_TYPE, OUT_TYPE) \
+ FORCE_INLINE \
+ gdv_##OUT_TYPE sqrt_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##OUT_TYPE>(sqrt(static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES_UNARY(SQRT, float64)
+
+// Sign
+#define SIGN(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE sign_##IN_TYPE(gdv_##IN_TYPE in) { \
+ if (in == 0.0) return in; \
+ return static_cast<gdv_##IN_TYPE>(copysign(1.0, static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES(SIGN)
+
+// Lshift
+#define LSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE lshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 << in2); \
+ }
+LSHIFT(int32)
+LSHIFT(int64)
+LSHIFT(uint32)
+LSHIFT(uint64)
+
+// Lshift
+#define RSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE rshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 >> in2); \
Review comment:
Can we ensure that the same semantics (shift logical or shift arithmetic) on all platforms using just `>>`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jpedroantunes commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
jpedroantunes commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-853778213
The `LOG2` function was added to the Pull Request
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r722267212
##########
File path: cpp/src/gandiva/function_registry_math_ops.cc
##########
@@ -16,6 +16,7 @@
// under the License.
#include "gandiva/function_registry_math_ops.h"
+
Review comment:
@kiszk I removed the line
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-865118448
It looks like the difference is that here the shift amount is reduced modulo the type precision (what x86 generally does but *not* what ARM does, e.g. see this post about [MSVC on ARM](https://devblogs.microsoft.com/cppblog/hello-arm-exploring-undefined-unspecified-and-implementation-defined-behavior-in-c/#shift-operators); allows negative shifts) and in the other PR, such shifts (including negative shifts) are disallowed (following the C++ specification).
The Arrow Compute behavior reflects the presence of having checked/unchecked variants; it seems the Gandiva variants must always produce a result? But it would be good to have them agree.
Looking at some other systems:
- T-SQL (MS SQL Server) has no bit-shift operators,
- Postgres doesn't specify a behavior.
- Python disallows negative shift values (overflow doesn't apply due to infinite-precision integers)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-865118448
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-986828337
ping @pitrou
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jpedroantunes commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
jpedroantunes commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-853778213
The `LOG2` function was added to the Pull Request
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r636696619
##########
File path: cpp/src/gandiva/gdv_function_stubs_test.cc
##########
@@ -290,4 +299,53 @@ TEST(TestGdvFnStubs, TestCastVARCHARFromDouble) {
EXPECT_FALSE(ctx.has_error());
}
+TEST(TestGdvFnStubs, TestAbs) {
+ gandiva::ExecutionContext ctx;
+ uint64_t ctx_ptr = reinterpret_cast<int64_t>(&ctx);
+
+ // Abs functions
+ EXPECT_EQ(gdv_fn_abs_int32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_uint32(ctx_ptr, 0), 0);
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, 0), 0L);
+ EXPECT_EQ(gdv_fn_abs_uint64(ctx_ptr, 0), 0L);
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, 0.0f), abs(0.0f));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, 0.0), abs(0.0));
+
+ VerifyAlmostEquals(gdv_fn_abs_int32(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_int64(ctx_ptr, (INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float32(ctx_ptr, static_cast<float>(INT32_MIN + 1)),
+ abs(static_cast<float>(INT32_MIN + 1)));
+ VerifyAlmostEquals(gdv_fn_abs_float64(ctx_ptr, static_cast<double>(INT32_MIN + 1)),
+ abs(static_cast<double>(INT32_MIN + 1)));
+
+ EXPECT_EQ(gdv_fn_abs_int64(ctx_ptr, (INT64_MIN + 1)),
Review comment:
Does it mean that we will accept the loss of digits in INT64?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r637720694
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -220,6 +264,51 @@ ENUMERIC_TYPES_UNARY(DEGREES, float64)
}
POWER(float64, float64, float64)
+// Sqrt
+#define SQRT(IN_TYPE, OUT_TYPE) \
+ FORCE_INLINE \
+ gdv_##OUT_TYPE sqrt_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##OUT_TYPE>(sqrt(static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES_UNARY(SQRT, float64)
+
+// Sign
+#define SIGN(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE sign_##IN_TYPE(gdv_##IN_TYPE in) { \
+ if (in == 0.0) return in; \
+ return static_cast<gdv_##IN_TYPE>(copysign(1.0, static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES(SIGN)
+
+// Lshift
+#define LSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE lshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 << in2); \
+ }
+LSHIFT(int32)
+LSHIFT(int64)
+LSHIFT(uint32)
+LSHIFT(uint64)
+
+// Lshift
+#define RSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE rshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 >> in2); \
Review comment:
@kiszk Thanks for your revision, I checked the [C++ reference](https://en.cppreference.com/w/cpp/language/operator_arithmetic) and I checked that the shift operators have undefined or implementation-choice behavior for signed and negative numbers.
So as in this Pull Request, we are implementing the `Logical Shift`, we can cast the input parameters to their respective `unsigned` types and apply the shift operators and they will have the same behavior in all platforms. Do you agree with the solution?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-865200245
Well it's perhaps worth discussing - which of the various languages' behaviors we want to mimic. Though as Antoine noted in the other PR, since there's not really a consensus, it's safer to just error on out-of-range values.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-864323035
Regarding rshift and lshift, is it better to keep the same semantics as [this](https://github.com/apache/arrow/pull/10530)? cc @pitrou
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb edited a comment on pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb edited a comment on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-847475275
@kiszk I applied all suggestions you proposed, could you check if the corrections are fine? Thanks in advance for the review!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-865117615
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r635039248
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -220,6 +264,51 @@ ENUMERIC_TYPES_UNARY(DEGREES, float64)
}
POWER(float64, float64, float64)
+// Sqrt
+#define SQRT(IN_TYPE, OUT_TYPE) \
+ FORCE_INLINE \
+ gdv_##OUT_TYPE sqrt_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##OUT_TYPE>(sqrt(static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES_UNARY(SQRT, float64)
+
+// Sign
+#define SIGN(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE sign_##IN_TYPE(gdv_##IN_TYPE in) { \
+ if (in == 0.0) return in; \
+ return static_cast<gdv_##IN_TYPE>(copysign(1.0, static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES(SIGN)
+
+// Lshift
+#define LSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE lshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 << in2); \
+ }
+LSHIFT(int32)
+LSHIFT(int64)
+LSHIFT(uint32)
+LSHIFT(uint64)
+
+// Lshift
Review comment:
Lshift -> Rshift
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r635032175
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -273,4 +324,137 @@ TEST(TestExtendedMathOps, TestTrigonometricFunctions) {
VerifyFuzzyEquals(cot_float64(M_PI / 2), tan(M_PI / 2 - M_PI / 2));
}
+TEST(TestExtendedMathOps, TestAbs) {
+ // Abs functions
+ VerifyFuzzyEquals(abs_int32(0), abs(0));
+ VerifyFuzzyEquals(abs_int64(0), abs(0));
+ VerifyFuzzyEquals(abs_float32(0), abs(0));
+ VerifyFuzzyEquals(abs_float64(0), abs(0));
+
+ VerifyFuzzyEquals(abs_int32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_int64(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float64(-5), abs(-5));
+
+ VerifyFuzzyEquals(abs_int32(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_int64(-2371041), abs(-2371041));
Review comment:
Can you add a test case using `abs_int64(LONG_MIN)` and `abs_int64(LONG_MIN+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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r637713009
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -273,4 +324,137 @@ TEST(TestExtendedMathOps, TestTrigonometricFunctions) {
VerifyFuzzyEquals(cot_float64(M_PI / 2), tan(M_PI / 2 - M_PI / 2));
}
+TEST(TestExtendedMathOps, TestAbs) {
+ // Abs functions
+ VerifyFuzzyEquals(abs_int32(0), abs(0));
+ VerifyFuzzyEquals(abs_int64(0), abs(0));
+ VerifyFuzzyEquals(abs_float32(0), abs(0));
+ VerifyFuzzyEquals(abs_float64(0), abs(0));
+
+ VerifyFuzzyEquals(abs_int32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_int64(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float32(-5), abs(-5));
+ VerifyFuzzyEquals(abs_float64(-5), abs(-5));
+
+ VerifyFuzzyEquals(abs_int32(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_int64(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_float32(-2371041), abs(-2371041));
+ VerifyFuzzyEquals(abs_float64(-2371041), abs(-2371041));
+
+ VerifyFuzzyEquals(abs_int32(-3600.50), abs(-3600));
+ VerifyFuzzyEquals(abs_int64(-3600.50), abs(-3600));
+ VerifyFuzzyEquals(abs_float32(-3600.50), abs(-3600.50));
+ VerifyFuzzyEquals(abs_float64(-3600.50), abs(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestCeil) {
+ // Ceil functions
+ VerifyFuzzyEquals(ceil_float32(0), ceil(0));
+ VerifyFuzzyEquals(ceil_float64(0), ceil(0));
+
+ VerifyFuzzyEquals(ceil_float32(-5), ceil(-5));
+ VerifyFuzzyEquals(ceil_float64(-5), ceil(-5));
+
+ VerifyFuzzyEquals(ceil_float32(-2371041), ceil(-2371041));
+ VerifyFuzzyEquals(ceil_float64(-2371041), ceil(-2371041));
+
+ VerifyFuzzyEquals(ceil_float32(5.45), ceil(5.45));
+ VerifyFuzzyEquals(ceil_float64(5.45), ceil(5.45));
+
+ VerifyFuzzyEquals(ceil_float32(-3600.50), ceil(-3600.50));
+ VerifyFuzzyEquals(ceil_float64(-3600.50), ceil(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestFloor) {
+ // Ceil functions
+ VerifyFuzzyEquals(floor_float32(0), floor(0));
+ VerifyFuzzyEquals(floor_float64(0), floor(0));
+
+ VerifyFuzzyEquals(floor_float32(-5), floor(-5));
+ VerifyFuzzyEquals(floor_float64(-5), floor(-5));
+
+ VerifyFuzzyEquals(floor_float32(-2371041), floor(-2371041));
+ VerifyFuzzyEquals(floor_float64(-2371041), floor(-2371041));
+
+ VerifyFuzzyEquals(floor_float32(5.45), floor(5.45));
+ VerifyFuzzyEquals(floor_float64(5.45), floor(5.45));
+
+ VerifyFuzzyEquals(floor_float32(-3600.50), floor(-3600.50));
+ VerifyFuzzyEquals(floor_float64(-3600.50), floor(-3600.50));
+}
+
+TEST(TestExtendedMathOps, TestConstants) {
+ // Constants functions
+ VerifyFuzzyEquals(pi(), M_PI);
+ VerifyFuzzyEquals(e(), exp(1.0));
+}
+
+TEST(TestExtendedMathOps, TestSqrt) {
+ // Sqrt functions
+ VerifyFuzzyEquals(sqrt_int32(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_int64(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_float32(0), sqrt(0));
+ VerifyFuzzyEquals(sqrt_float64(0), sqrt(0));
+
+ VerifyFuzzyEquals(sqrt_int32(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_int64(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_float32(5), sqrt(5));
+ VerifyFuzzyEquals(sqrt_float64(5), sqrt(5));
+
+ VerifyFuzzyEquals(sqrt_int32(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_int64(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_float32(2371041), sqrt(2371041));
+ VerifyFuzzyEquals(sqrt_float64(2371041), sqrt(2371041));
+
+ VerifyFuzzyEquals(sqrt_int32(3600.50), sqrt(3600));
+ VerifyFuzzyEquals(sqrt_int64(3600.50), sqrt(3600));
+ VerifyFuzzyEquals(sqrt_float32(3600.50), sqrt(3600.50));
+ VerifyFuzzyEquals(sqrt_float64(3600.50), sqrt(3600.50));
+}
+
+TEST(TestExtendedMathOps, TestSign) {
+ // Sqrt functions
+ EXPECT_EQ(sign_int32(0), 0);
+ EXPECT_EQ(sign_int64(0), 0);
+ EXPECT_EQ(sign_float32(0), 0);
+ EXPECT_EQ(sign_float64(0), 0);
+
+ EXPECT_EQ(sign_int32(5), 1);
+ EXPECT_EQ(sign_int64(5), 1);
+ EXPECT_EQ(sign_float32(5), 1);
+ EXPECT_EQ(sign_float64(5), 1);
+
+ EXPECT_EQ(sign_int32(2371041), 1);
+ EXPECT_EQ(sign_int64(2371041), 1);
+ EXPECT_EQ(sign_float32(2371041), 1);
+ EXPECT_EQ(sign_float64(2371041), 1);
+
+ EXPECT_EQ(sign_int32(-3600.50), -1);
+ EXPECT_EQ(sign_int64(-3600.50), -1);
+ EXPECT_EQ(sign_float32(-3600.50), -1);
+ EXPECT_EQ(sign_float64(-3600.50), -1);
+}
+
+TEST(TestExtendedMathOps, TestLshiftRshift) {
+ // Lshift functions
+ EXPECT_EQ(lshift_int32(0, 1), 0 << 1);
+ EXPECT_EQ(lshift_int64(0, 1), 0 << 1);
+
+ EXPECT_EQ(lshift_int32(5, 20), 5 << 20);
+ EXPECT_EQ(lshift_int64(5, 20), 5 << 20);
+
+ EXPECT_EQ(lshift_int32(1, 20), 1 << 20);
+ EXPECT_EQ(lshift_int64(1, 20), 1 << 20);
+
+ // Rshift functions
+ EXPECT_EQ(rshift_int32(0, 1), 0 >> 1);
+ EXPECT_EQ(rshift_int64(0, 1), 0 >> 1);
+
+ EXPECT_EQ(rshift_int32(5, 20), 5 >> 20);
+ EXPECT_EQ(rshift_int64(5, 20), 5 >> 20);
+
+ EXPECT_EQ(rshift_int32(1, 20), 1 >> 20);
+ EXPECT_EQ(rshift_int64(1, 20), 1 >> 20);
+}
Review comment:
The test was added.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-918876863
@kiszk I applied all suggestions you made in the PR. Could you take another look at the PR and see if it is ready to be merged?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #10350: ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#discussion_r635039850
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -220,6 +264,51 @@ ENUMERIC_TYPES_UNARY(DEGREES, float64)
}
POWER(float64, float64, float64)
+// Sqrt
+#define SQRT(IN_TYPE, OUT_TYPE) \
+ FORCE_INLINE \
+ gdv_##OUT_TYPE sqrt_##IN_TYPE(gdv_##IN_TYPE in) { \
+ return static_cast<gdv_##OUT_TYPE>(sqrt(static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES_UNARY(SQRT, float64)
+
+// Sign
+#define SIGN(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE sign_##IN_TYPE(gdv_##IN_TYPE in) { \
+ if (in == 0.0) return in; \
+ return static_cast<gdv_##IN_TYPE>(copysign(1.0, static_cast<long double>(in))); \
+ }
+ENUMERIC_TYPES(SIGN)
+
+// Lshift
+#define LSHIFT(IN_TYPE) \
+ FORCE_INLINE \
+ gdv_##IN_TYPE lshift_##IN_TYPE(gdv_##IN_TYPE in1, gdv_##IN_TYPE in2) { \
+ return static_cast<gdv_##IN_TYPE>(in1 << in2); \
+ }
+LSHIFT(int32)
+LSHIFT(int64)
+LSHIFT(uint32)
+LSHIFT(uint64)
+
+// Lshift
Review comment:
It would be good to write the definition of Rshift. Shift logical or shift arithmetic?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] anthonylouisbsb commented on pull request #10350: ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions
Posted by GitBox <gi...@apache.org>.
anthonylouisbsb commented on pull request #10350:
URL: https://github.com/apache/arrow/pull/10350#issuecomment-1020750278
@pitrou can you take a look at it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org