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 2020/08/24 03:28:53 UTC
[GitHub] [arrow] sagnikc-dremio opened a new pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
sagnikc-dremio opened a new pull request #8035:
URL: https://github.com/apache/arrow/pull/8035
----------------------------------------------------------------
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] sagnikc-dremio commented on a change in pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#discussion_r495881628
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +111,20 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+// round
+#define ROUND_DECIMAL(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE##_int32(gdv_##TYPE number, gdv_int32 precision) { \
Review comment:
Yes, made the change.
----------------------------------------------------------------
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] sagnikc-dremio commented on a change in pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#discussion_r478802287
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,20 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRoundDecimal) {
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.789, 2), 1234.79);
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.12345, -3), 1000);
+ EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567, 3), -1234.457);
+ EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567, -3), -1000);
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.4567, 0), 1234);
+
+ VerifyFuzzyEquals(round_float64_int32(1234.789, 2), 1234.79);
Review comment:
I am sorry, I did not fully understand your concern. Are you suggesting that some unit tests be added for `round_float64_int32()` where the first argument is INT_MAX+1 or INT_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] praveenbingo commented on a change in pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
praveenbingo commented on a change in pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#discussion_r497271462
##########
File path: cpp/src/gandiva/precompiled/types.h
##########
@@ -132,6 +132,10 @@ gdv_int64 div_int64_int64(gdv_int64 context, gdv_int64 in1, gdv_int64 in2);
gdv_float32 div_float32_float32(gdv_int64 context, gdv_float32 in1, gdv_float32 in2);
gdv_float64 div_float64_float64(gdv_int64 context, gdv_float64 in1, gdv_float64 in2);
+gdv_float32 round_float32_int32(gdv_float32 number, gdv_int32 precision);
Review comment:
looks like precision is misnamed..can you please rename this 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] sagnikc-dremio commented on a change in pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#discussion_r487679330
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,20 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRoundDecimal) {
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.789, 2), 1234.79);
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.12345, -3), 1000);
+ EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567, 3), -1234.457);
+ EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567, -3), -1000);
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.4567, 0), 1234);
+
+ VerifyFuzzyEquals(round_float64_int32(1234.789, 2), 1234.79);
Review comment:
Added the unit tests.
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#issuecomment-678885179
https://issues.apache.org/jira/browse/ARROW-9641
----------------------------------------------------------------
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] sagnikc-dremio commented on a change in pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#discussion_r497271804
##########
File path: cpp/src/gandiva/precompiled/types.h
##########
@@ -132,6 +132,10 @@ gdv_int64 div_int64_int64(gdv_int64 context, gdv_int64 in1, gdv_int64 in2);
gdv_float32 div_float32_float32(gdv_int64 context, gdv_float32 in1, gdv_float32 in2);
gdv_float64 div_float64_float64(gdv_int64 context, gdv_float64 in1, gdv_float64 in2);
+gdv_float32 round_float32_int32(gdv_float32 number, gdv_int32 precision);
Review comment:
Oh right sure. Missed that.
----------------------------------------------------------------
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 #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#discussion_r478827719
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,20 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRoundDecimal) {
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.789, 2), 1234.79);
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.12345, -3), 1000);
+ EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567, 3), -1234.457);
+ EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567, -3), -1000);
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.4567, 0), 1234);
+
+ VerifyFuzzyEquals(round_float64_int32(1234.789, 2), 1234.79);
Review comment:
Sure, there are examples. I do not know the results in `int32`.
```
VerifyFuzzyEquals(round_float64_int32((double)INT_MAX+1, 0), ???);
VerifyFuzzyEquals(round_float64_int32((double)INT_MIN-1, 0), ???);
```
----------------------------------------------------------------
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] praveenbingo commented on a change in pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
praveenbingo commented on a change in pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#discussion_r495804184
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +111,20 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+// round
+#define ROUND_DECIMAL(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE##_int32(gdv_##TYPE number, gdv_int32 precision) { \
Review comment:
should this be called output scale?
----------------------------------------------------------------
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 #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8035:
URL: https://github.com/apache/arrow/pull/8035#discussion_r476395110
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,20 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRoundDecimal) {
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.789, 2), 1234.79);
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.12345, -3), 1000);
+ EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567, 3), -1234.457);
+ EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567, -3), -1000);
+ EXPECT_FLOAT_EQ(round_float32_int32(1234.4567, 0), 1234);
+
+ VerifyFuzzyEquals(round_float64_int32(1234.789, 2), 1234.79);
Review comment:
What happens when we convert (double)INT_MAX+1 or (double)INT_MIN-1 int32?
----------------------------------------------------------------
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] praveenbingo closed pull request #8035: ARROW-9641: [C++][Gandiva] Implement round() for floating point and double floating point numbers
Posted by GitBox <gi...@apache.org>.
praveenbingo closed pull request #8035:
URL: https://github.com/apache/arrow/pull/8035
----------------------------------------------------------------
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