kou commented on code in PR #15208:
URL: https://github.com/apache/arrow/pull/15208#discussion_r1063099410
##########
cpp/src/gandiva/precompiled/arithmetic_ops_test.cc:
##########
@@ -629,6 +629,45 @@ TEST(TestArithmeticOps, TestSignIntFloatDouble) {
EXPECT_EQ(sign_float64(-2147483647), -1.0);
}
+TEST(TestArithmeticOps, TestAbsIntFloatDouble) {
+ // abs from int32
+ EXPECT_EQ(abs_int32(43), 43);
+ EXPECT_EQ(abs_int32(-54), 54);
+ EXPECT_EQ(abs_int32(63), 63);
+ EXPECT_EQ(abs_int32(INT32_MAX), INT32_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int32(INT32_MIN), -INT32_MIN);
Review Comment:
Why do we keep this as a comment?
How about removing this entirely if this isn't needed? Or can we add an
expectation instead of keeping commented out code?
##########
cpp/src/gandiva/precompiled/arithmetic_ops_test.cc:
##########
@@ -629,6 +629,45 @@ TEST(TestArithmeticOps, TestSignIntFloatDouble) {
EXPECT_EQ(sign_float64(-2147483647), -1.0);
}
+TEST(TestArithmeticOps, TestAbsIntFloatDouble) {
+ // abs from int32
+ EXPECT_EQ(abs_int32(43), 43);
+ EXPECT_EQ(abs_int32(-54), 54);
+ EXPECT_EQ(abs_int32(63), 63);
+ EXPECT_EQ(abs_int32(INT32_MAX), INT32_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int32(INT32_MIN), -INT32_MIN);
+
+ // abs from int64
+ EXPECT_EQ(abs_int64(90), 90);
+ EXPECT_EQ(abs_int64(-7), 7);
+ EXPECT_EQ(abs_int64(INT64_MAX), INT64_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int64(INT64_MIN), -INT64_MIN);
+
+ // abs from floats
+ EXPECT_EQ(abs_float32(6.6f), 6.6f);
+ EXPECT_EQ(abs_float32(-6.6f), 6.6f);
+ EXPECT_EQ(abs_float32(-6.3f), 6.3f);
+ EXPECT_EQ(abs_float32(0.0f), 0.0f);
+ EXPECT_EQ(abs_float32(-0.0f), -0.0f);
+ EXPECT_EQ(abs_float32(INFINITY), INFINITY);
+ EXPECT_EQ(abs_float32(-INFINITY), INFINITY);
+
+ // sign from doubles
+ EXPECT_EQ(abs_float64(6.6), 6.6);
+ EXPECT_EQ(abs_float64(-6.6), 6.6);
+ EXPECT_EQ(abs_float64(-6.3), 6.3);
Review Comment:
ditto.
##########
cpp/src/gandiva/precompiled/arithmetic_ops_test.cc:
##########
@@ -629,6 +629,45 @@ TEST(TestArithmeticOps, TestSignIntFloatDouble) {
EXPECT_EQ(sign_float64(-2147483647), -1.0);
}
+TEST(TestArithmeticOps, TestAbsIntFloatDouble) {
+ // abs from int32
+ EXPECT_EQ(abs_int32(43), 43);
+ EXPECT_EQ(abs_int32(-54), 54);
+ EXPECT_EQ(abs_int32(63), 63);
+ EXPECT_EQ(abs_int32(INT32_MAX), INT32_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int32(INT32_MIN), -INT32_MIN);
+
+ // abs from int64
+ EXPECT_EQ(abs_int64(90), 90);
+ EXPECT_EQ(abs_int64(-7), 7);
+ EXPECT_EQ(abs_int64(INT64_MAX), INT64_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int64(INT64_MIN), -INT64_MIN);
+
+ // abs from floats
+ EXPECT_EQ(abs_float32(6.6f), 6.6f);
+ EXPECT_EQ(abs_float32(-6.6f), 6.6f);
+ EXPECT_EQ(abs_float32(-6.3f), 6.3f);
+ EXPECT_EQ(abs_float32(0.0f), 0.0f);
+ EXPECT_EQ(abs_float32(-0.0f), -0.0f);
+ EXPECT_EQ(abs_float32(INFINITY), INFINITY);
+ EXPECT_EQ(abs_float32(-INFINITY), INFINITY);
+
+ // sign from doubles
+ EXPECT_EQ(abs_float64(6.6), 6.6);
+ EXPECT_EQ(abs_float64(-6.6), 6.6);
+ EXPECT_EQ(abs_float64(-6.3), 6.3);
+ EXPECT_EQ(abs_float64(0.0), 0.);
+ EXPECT_EQ(abs_float64(-0), 0);
+ EXPECT_EQ(abs_float64(999999.99999999999999999999999),
999999.99999999999999999999999);
+ EXPECT_EQ(abs_float64(-999999.99999999999999999999999),
999999.99999999999999999999999);
+ EXPECT_EQ(abs_float64(INFINITY), INFINITY);
+ EXPECT_EQ(abs_float64(-INFINITY), INFINITY);
+
EXPECT_TRUE(std::isnan(abs_float64(std::numeric_limits<double>::quiet_NaN())));
+ EXPECT_EQ(abs_float64(-2147483647), 2147483647);
Review Comment:
Why do we need this case?
##########
cpp/src/gandiva/tests/projector_test.cc:
##########
@@ -1752,6 +1752,112 @@ TEST_F(TestProjector, TestSign) {
EXPECT_ARROW_ARRAY_EQUALS(out_float64, outputs4.at(0));
}
+TEST_F(TestProjector, TestAbs) {
Review Comment:
Could you cleanup this like the followings?
*
https://github.com/apache/arrow/pull/13656/commits/7d866b5a6e3589e619b042fcde37a18e87fd9121
*
https://github.com/apache/arrow/pull/13656/commits/7918e72b5f921ad5f0ac22a5f048c423ccac4582
##########
cpp/src/gandiva/precompiled/arithmetic_ops_test.cc:
##########
@@ -629,6 +629,45 @@ TEST(TestArithmeticOps, TestSignIntFloatDouble) {
EXPECT_EQ(sign_float64(-2147483647), -1.0);
}
+TEST(TestArithmeticOps, TestAbsIntFloatDouble) {
+ // abs from int32
+ EXPECT_EQ(abs_int32(43), 43);
+ EXPECT_EQ(abs_int32(-54), 54);
+ EXPECT_EQ(abs_int32(63), 63);
+ EXPECT_EQ(abs_int32(INT32_MAX), INT32_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int32(INT32_MIN), -INT32_MIN);
+
+ // abs from int64
+ EXPECT_EQ(abs_int64(90), 90);
+ EXPECT_EQ(abs_int64(-7), 7);
+ EXPECT_EQ(abs_int64(INT64_MAX), INT64_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int64(INT64_MIN), -INT64_MIN);
+
+ // abs from floats
+ EXPECT_EQ(abs_float32(6.6f), 6.6f);
+ EXPECT_EQ(abs_float32(-6.6f), 6.6f);
+ EXPECT_EQ(abs_float32(-6.3f), 6.3f);
Review Comment:
Why do we need both of them?
It seems that they are the same category (negative and non-border) value.
##########
cpp/src/gandiva/precompiled/arithmetic_ops_test.cc:
##########
@@ -629,6 +629,45 @@ TEST(TestArithmeticOps, TestSignIntFloatDouble) {
EXPECT_EQ(sign_float64(-2147483647), -1.0);
}
+TEST(TestArithmeticOps, TestAbsIntFloatDouble) {
+ // abs from int32
+ EXPECT_EQ(abs_int32(43), 43);
+ EXPECT_EQ(abs_int32(-54), 54);
+ EXPECT_EQ(abs_int32(63), 63);
+ EXPECT_EQ(abs_int32(INT32_MAX), INT32_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int32(INT32_MIN), -INT32_MIN);
+
+ // abs from int64
+ EXPECT_EQ(abs_int64(90), 90);
+ EXPECT_EQ(abs_int64(-7), 7);
+ EXPECT_EQ(abs_int64(INT64_MAX), INT64_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int64(INT64_MIN), -INT64_MIN);
Review Comment:
ditto.
##########
cpp/src/gandiva/precompiled/arithmetic_ops_test.cc:
##########
@@ -629,6 +629,45 @@ TEST(TestArithmeticOps, TestSignIntFloatDouble) {
EXPECT_EQ(sign_float64(-2147483647), -1.0);
}
+TEST(TestArithmeticOps, TestAbsIntFloatDouble) {
+ // abs from int32
+ EXPECT_EQ(abs_int32(43), 43);
+ EXPECT_EQ(abs_int32(-54), 54);
+ EXPECT_EQ(abs_int32(63), 63);
+ EXPECT_EQ(abs_int32(INT32_MAX), INT32_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int32(INT32_MIN), -INT32_MIN);
+
+ // abs from int64
+ EXPECT_EQ(abs_int64(90), 90);
+ EXPECT_EQ(abs_int64(-7), 7);
+ EXPECT_EQ(abs_int64(INT64_MAX), INT64_MAX);
+ // Overflow
+ // EXPECT_EQ(abs_int64(INT64_MIN), -INT64_MIN);
+
+ // abs from floats
+ EXPECT_EQ(abs_float32(6.6f), 6.6f);
+ EXPECT_EQ(abs_float32(-6.6f), 6.6f);
+ EXPECT_EQ(abs_float32(-6.3f), 6.3f);
+ EXPECT_EQ(abs_float32(0.0f), 0.0f);
+ EXPECT_EQ(abs_float32(-0.0f), -0.0f);
+ EXPECT_EQ(abs_float32(INFINITY), INFINITY);
+ EXPECT_EQ(abs_float32(-INFINITY), INFINITY);
+
+ // sign from doubles
Review Comment:
```suggestion
// abs from doubles
```
##########
cpp/src/gandiva/function_registry_arithmetic.cc:
##########
@@ -192,11 +192,17 @@ std::vector<NativeFunction>
GetArithmeticFunctionRegistry() {
day_time_interval(), kResultNullIfNull,
"negative_daytimeinterval",
NativeFunction::kNeedsContext |
NativeFunction::kCanReturnErrors),
+
Review Comment:
```suggestion
```
--
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]
