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/10/08 11:56:04 UTC
[GitHub] [arrow] sagnikc-dremio opened a new pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
sagnikc-dremio opened a new pull request #8398:
URL: https://github.com/apache/arrow/pull/8398
This patch attempts to resolve the bug introduced by the addition of round() in ARROW-9641
round() for floats is returning incorrect results for some edge cases, like round(cast(1.55 as float), 1) gives 1.6, but it should be 1.5, since the result after casting 1.55 to float comes to 1.5499999523162842, due to inaccurate representation of floating point numbers in memory.
Removing an intermediate explicit cast to float statement for a double value, which is used in subsequent computations, minimises the error introduced due to the incorrect representation.
----------------------------------------------------------------
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 #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#issuecomment-705524893
https://issues.apache.org/jira/browse/ARROW-10234
----------------------------------------------------------------
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 #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#discussion_r501676306
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -93,6 +93,9 @@ TEST(TestExtendedMathOps, TestRoundDecimal) {
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, 3), -1234.457f);
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, -3), -1000);
EXPECT_FLOAT_EQ(round_float32_int32(1234.4567f, 0), 1234);
+ EXPECT_FLOAT_EQ(round_float32_int32(static_cast<float>(1.55), 1), 1.5f);
Review comment:
Done.
----------------------------------------------------------------
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] vvellanki commented on a change in pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
vvellanki commented on a change in pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#discussion_r501670246
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -93,6 +93,9 @@ TEST(TestExtendedMathOps, TestRoundDecimal) {
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, 3), -1234.457f);
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, -3), -1000);
EXPECT_FLOAT_EQ(round_float32_int32(1234.4567f, 0), 1234);
+ EXPECT_FLOAT_EQ(round_float32_int32(static_cast<float>(1.55), 1), 1.5f);
Review comment:
Please add an explicit test for round_float32_int32(1.5499999523162842f, 1) = 1.5f
The test here is confusing - it relies on people knowing that (float)1.55 = 1.5499999523162842
----------------------------------------------------------------
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 pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#issuecomment-705519546
----------------------------------------------------------------
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 #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
praveenbingo closed pull request #8398:
URL: https://github.com/apache/arrow/pull/8398
----------------------------------------------------------------
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 #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#issuecomment-705641941
Revision: b1ef5438a46892d8ee826bb6095291ad2685bebc
Submitted crossbow builds: [ursa-labs/crossbow @ actions-625](https://github.com/ursa-labs/crossbow/branches/all?query=actions-625)
|Task|Status|
|----|------|
|gandiva-jar-osx|[](https://travis-ci.org/ursa-labs/crossbow/branches)|
|gandiva-jar-xenial|[](https://travis-ci.org/ursa-labs/crossbow/branches)|
----------------------------------------------------------------
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 #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#issuecomment-705524893
----------------------------------------------------------------
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] pprudhvi commented on a change in pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
pprudhvi commented on a change in pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#discussion_r501669631
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -93,6 +93,9 @@ TEST(TestExtendedMathOps, TestRoundDecimal) {
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, 3), -1234.457f);
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, -3), -1000);
EXPECT_FLOAT_EQ(round_float32_int32(1234.4567f, 0), 1234);
+ EXPECT_FLOAT_EQ(round_float32_int32(static_cast<float>(1.55), 1), 1.5f);
Review comment:
verified that this fails without this patch
----------------------------------------------------------------
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] pprudhvi commented on a change in pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
pprudhvi commented on a change in pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#discussion_r501669631
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -93,6 +93,9 @@ TEST(TestExtendedMathOps, TestRoundDecimal) {
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, 3), -1234.457f);
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, -3), -1000);
EXPECT_FLOAT_EQ(round_float32_int32(1234.4567f, 0), 1234);
+ EXPECT_FLOAT_EQ(round_float32_int32(static_cast<float>(1.55), 1), 1.5f);
Review comment:
verified that this fails without this patch
----------------------------------------------------------------
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 pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#issuecomment-706087612
> Hi @sagnikc-dremio does this by chance resolve the build failure observed on [ARROW-10177](https://issues.apache.org/jira/browse/ARROW-10177)? Actually--let's run the tests, and if it doesn't, can you please take a look at that as well?
This is a separate issue, not related to the gandiva-jar-xenial failures. I will look into 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#issuecomment-705640747
----------------------------------------------------------------
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] vvellanki commented on a change in pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
vvellanki commented on a change in pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#discussion_r501670246
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -93,6 +93,9 @@ TEST(TestExtendedMathOps, TestRoundDecimal) {
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, 3), -1234.457f);
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, -3), -1000);
EXPECT_FLOAT_EQ(round_float32_int32(1234.4567f, 0), 1234);
+ EXPECT_FLOAT_EQ(round_float32_int32(static_cast<float>(1.55), 1), 1.5f);
Review comment:
Please add an explicit test for round_float32_int32(1.5499999523162842f, 1) = 1.5f
The test here is confusing - it relies on people knowing that (float)1.55 = 1.5499999523162842
----------------------------------------------------------------
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 #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#discussion_r501676101
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -93,6 +93,9 @@ TEST(TestExtendedMathOps, TestRoundDecimal) {
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, 3), -1234.457f);
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, -3), -1000);
EXPECT_FLOAT_EQ(round_float32_int32(1234.4567f, 0), 1234);
+ EXPECT_FLOAT_EQ(round_float32_int32(static_cast<float>(1.55), 1), 1.5f);
Review comment:
Yes, I have verified that these tests fail without this 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 pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#issuecomment-705519546
@praveenbingo @pprudhvi Can you please review this 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 #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#discussion_r501676101
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -93,6 +93,9 @@ TEST(TestExtendedMathOps, TestRoundDecimal) {
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, 3), -1234.457f);
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, -3), -1000);
EXPECT_FLOAT_EQ(round_float32_int32(1234.4567f, 0), 1234);
+ EXPECT_FLOAT_EQ(round_float32_int32(static_cast<float>(1.55), 1), 1.5f);
Review comment:
Yes, I have verified that these tests fail without this change.
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -93,6 +93,9 @@ TEST(TestExtendedMathOps, TestRoundDecimal) {
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, 3), -1234.457f);
EXPECT_FLOAT_EQ(round_float32_int32(-1234.4567f, -3), -1000);
EXPECT_FLOAT_EQ(round_float32_int32(1234.4567f, 0), 1234);
+ EXPECT_FLOAT_EQ(round_float32_int32(static_cast<float>(1.55), 1), 1.5f);
Review comment:
Done.
----------------------------------------------------------------
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] nealrichardson commented on pull request #8398: ARROW-10234: [C++][Gandiva] Fix logic of round() for floats/decimals in Gandiva
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8398:
URL: https://github.com/apache/arrow/pull/8398#issuecomment-705640747
----------------------------------------------------------------
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