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/03 03:56:15 UTC
[GitHub] [arrow] sagnikc-dremio opened a new pull request #7885: Implement round() function
sagnikc-dremio opened a new pull request #7885:
URL: https://github.com/apache/arrow/pull/7885
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r467352487
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,19 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRound) {
+ EXPECT_EQ(round_int32(7589, -1), 7590);
Review comment:
what I mean is I don't think anything here test for the addition of .5, I think there are currently other values for the rounding addition that would allow these tests to pass
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r478829379
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,22 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRound) {
+ EXPECT_EQ(round_int32_int32(85, -1), 90);
Review comment:
Yes, the result should be -90. Thanks for pointing it out. Looking into the test failures.
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: Implement round() function
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r464573782
##########
File path: cpp/src/gandiva/precompiled/arithmetic_ops_test.cc
##########
@@ -101,4 +101,10 @@ TEST(TestArithmeticOps, TestDiv) {
context.Reset();
}
+TEST(TestArithmeticOps, TestRound) {
+ EXPECT_EQ(round_int32(758, -1), 760);
Review comment:
test cases should cover negative numbers, and boundary conditions (i.e. .5 5).
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r465480139
##########
File path: cpp/src/gandiva/precompiled/arithmetic_ops.cc
##########
@@ -234,6 +234,25 @@ DIV_FLOAT(float64)
#undef DIV_FLOAT
+#define ROUND(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE(gdv_##TYPE number, gdv_int32 place) { \
+ if (number == 0) { \
+ return 0; \
+ } \
+ if (number < 0) { \
+ return -round_##TYPE(-number, place); \
+ } \
+ return floor(number * pow(10, place) + 0.5) * pow(10, -place); \
Review comment:
Modified this PR to handle only integers and long integers. For floats and doubles, handling for inf/NaN will be necessary, so will be taking it up in another PR.
For this particular case, we need to be dealing with cases only where the precision for rounding is a negative number, and hence pow(10, precision) will never run into overflow or underflow (providing a check if the precision is larger than the number of digits in the number, return 0).
Please let me know if you have any suggestions.
----------------------------------------------------------------
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 #7885: Implement round() function
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#issuecomment-667791168
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r478803947
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +112,76 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+FORCE_INLINE
+gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
+ // for integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 9 is
+ // the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
+ if (abs_precision > 9) {
+ return 0;
+ }
+ gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
+ gdv_int32 remainder = number % power_of_10, quotient = number / power_of_10;
+ // if the fractional part of the quotient >= 0.5, round to next higher integer
+ if (remainder >= power_of_10 / 2) {
+ quotient++;
+ }
+ return quotient * power_of_10;
+}
+
+FORCE_INLINE
+gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
+ // for long integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 19 is
+ // the smallest N for which 10^N does not fit into 64 bits, so we can safely return 0
+ if (abs_precision > 18) {
+ return 0;
+ }
+ gdv_int64 power_of_10 = get_power_of_10(abs_precision);
+ gdv_int64 remainder = number % power_of_10, quotient = number / power_of_10;
Review comment:
no, I think that is fine. I just didn't attempt to analyze all cases in my head, if unit tests pass I think this is OK.
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r465485532
##########
File path: cpp/src/gandiva/precompiled/arithmetic_ops.cc
##########
@@ -234,6 +234,25 @@ DIV_FLOAT(float64)
#undef DIV_FLOAT
+#define ROUND(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE(gdv_##TYPE number, gdv_int32 place) { \
+ if (number == 0) { \
+ return 0; \
+ } \
+ if (number < 0) { \
+ return -round_##TYPE(-number, place); \
+ } \
+ return floor(number * pow(10, place) + 0.5) * pow(10, -place); \
Review comment:
My suggestion would be to avoid floating point operations altogether. I haven't tested this but for pow(10, ...) on integers I think either a lookup table or recursive/squaring would like be more efficient. Then for rounding I think something like:
((num + (pow(10, -(precision + 1)) * 5) / pow(10, -precision)) * pow(10, -precision) should work.
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: Implement round() function
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r464575120
##########
File path: cpp/src/gandiva/precompiled/arithmetic_ops.cc
##########
@@ -234,6 +234,25 @@ DIV_FLOAT(float64)
#undef DIV_FLOAT
+#define ROUND(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE(gdv_##TYPE number, gdv_int32 place) { \
+ if (number == 0) { \
+ return 0; \
+ } \
+ if (number < 0) { \
+ return -round_##TYPE(-number, place); \
+ } \
+ return floor(number * pow(10, place) + 0.5) * pow(10, -place); \
Review comment:
it seems inefficient to use a floating point operations for integer values?
Also for floats is special hanlding for inf/Nan necessary?
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#issuecomment-668579349
https://issues.apache.org/jira/browse/ARROW-9640
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r472717430
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +112,76 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+FORCE_INLINE
+gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
+ // for integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 9 is
+ // the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
+ if (abs_precision > 9) {
+ return 0;
+ }
+ gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
+ gdv_int32 remainder = number % power_of_10, quotient = number / power_of_10;
+ // if the fractional part of the quotient >= 0.5, round to next higher integer
+ if (remainder >= power_of_10 / 2) {
+ quotient++;
+ }
+ return quotient * power_of_10;
+}
+
+FORCE_INLINE
+gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
+ // for long integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 19 is
+ // the smallest N for which 10^N does not fit into 64 bits, so we can safely return 0
+ if (abs_precision > 18) {
+ return 0;
+ }
+ gdv_int64 power_of_10 = get_power_of_10(abs_precision);
+ gdv_int64 remainder = number % power_of_10, quotient = number / power_of_10;
Review comment:
nit: two separate lines.
Further nit: I think you can get rid of a multiply if you make the code:
```
number -= remainder;
if (remainder >= power_of_10 / 2) {
number += power_of_10;
}
return number;
```
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r472736648
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +112,76 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+FORCE_INLINE
+gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
+ // for integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 9 is
+ // the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
+ if (abs_precision > 9) {
+ return 0;
+ }
+ gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
+ gdv_int32 remainder = number % power_of_10, quotient = number / power_of_10;
+ // if the fractional part of the quotient >= 0.5, round to next higher integer
+ if (remainder >= power_of_10 / 2) {
+ quotient++;
+ }
+ return quotient * power_of_10;
+}
+
+FORCE_INLINE
+gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
+ // for long integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
Review comment:
I don't think number == 0 is needed?
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r478803356
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +112,76 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+FORCE_INLINE
+gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
+ // for integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 9 is
+ // the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
+ if (abs_precision > 9) {
+ return 0;
+ }
+ gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
+ gdv_int32 remainder = number % power_of_10, quotient = number / power_of_10;
+ // if the fractional part of the quotient >= 0.5, round to next higher integer
+ if (remainder >= power_of_10 / 2) {
+ quotient++;
+ }
+ return quotient * power_of_10;
+}
+
+FORCE_INLINE
+gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
+ // for long integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 19 is
+ // the smallest N for which 10^N does not fit into 64 bits, so we can safely return 0
+ if (abs_precision > 18) {
+ return 0;
+ }
+ gdv_int64 power_of_10 = get_power_of_10(abs_precision);
+ gdv_int64 remainder = number % power_of_10, quotient = number / power_of_10;
Review comment:
What is your concern regarding negative numbers? As far as the unit tests with negative numbers are concerned, verified that the output is expected. Do you think more unit tests should be 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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r465485532
##########
File path: cpp/src/gandiva/precompiled/arithmetic_ops.cc
##########
@@ -234,6 +234,25 @@ DIV_FLOAT(float64)
#undef DIV_FLOAT
+#define ROUND(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE(gdv_##TYPE number, gdv_int32 place) { \
+ if (number == 0) { \
+ return 0; \
+ } \
+ if (number < 0) { \
+ return -round_##TYPE(-number, place); \
+ } \
+ return floor(number * pow(10, place) + 0.5) * pow(10, -place); \
Review comment:
My suggestion would be to avoid floating point operations altogether. I haven't tested this but for pow(10, ...) on integers I think either a lookup table or recursive/squaring would like be more efficient. Then for rounding I think something like:
((num + (pow(10, -(precision + 1)) * 5) / pow(10, -precision)) * pow(10, -precision) should work.
You would need to account for overflow on the addition here I think.
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r465480283
##########
File path: cpp/src/gandiva/precompiled/arithmetic_ops_test.cc
##########
@@ -101,4 +101,10 @@ TEST(TestArithmeticOps, TestDiv) {
context.Reset();
}
+TEST(TestArithmeticOps, TestRound) {
+ EXPECT_EQ(round_int32(758, -1), 760);
Review comment:
Added more test cases for negative numbers.
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r472734667
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +112,76 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+FORCE_INLINE
+gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
+ // for integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 9 is
+ // the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
+ if (abs_precision > 9) {
+ return 0;
+ }
+ gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
+ gdv_int32 remainder = number % power_of_10, quotient = number / power_of_10;
+ // if the fractional part of the quotient >= 0.5, round to next higher integer
+ if (remainder >= power_of_10 / 2) {
+ quotient++;
+ }
+ return quotient * power_of_10;
+}
+
+FORCE_INLINE
+gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
+ // for long integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
Review comment:
also, I don'tthink || mumber == 0 is needed any more?
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +112,76 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+FORCE_INLINE
+gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
+ // for integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 9 is
+ // the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
+ if (abs_precision > 9) {
+ return 0;
+ }
+ gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
+ gdv_int32 remainder = number % power_of_10, quotient = number / power_of_10;
+ // if the fractional part of the quotient >= 0.5, round to next higher integer
+ if (remainder >= power_of_10 / 2) {
+ quotient++;
+ }
+ return quotient * power_of_10;
+}
+
+FORCE_INLINE
+gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
+ // for long integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
Review comment:
I don'tthink || mumber == 0 is needed any more?
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r478826768
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,22 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRound) {
+ EXPECT_EQ(round_int32_int32(85, -1), 90);
Review comment:
@sagnikc-dremio this was what I referring to on my previous testing comments, do these seem reasonable?
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r465485532
##########
File path: cpp/src/gandiva/precompiled/arithmetic_ops.cc
##########
@@ -234,6 +234,25 @@ DIV_FLOAT(float64)
#undef DIV_FLOAT
+#define ROUND(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE(gdv_##TYPE number, gdv_int32 place) { \
+ if (number == 0) { \
+ return 0; \
+ } \
+ if (number < 0) { \
+ return -round_##TYPE(-number, place); \
+ } \
+ return floor(number * pow(10, place) + 0.5) * pow(10, -place); \
Review comment:
My suggestion would be to avoid floating point operations altogether. I haven't tested this but for pow(10, ...) on integers I think either a lookup table or recursive/squaring would like be more efficient. Then for rounding I think something like:
(num + (pow(10, -(precision + 1)) * 5) / pow(10, -precision) should work.
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r465496907
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,19 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRound) {
+ EXPECT_EQ(round_int32(7589, -1), 7590);
Review comment:
EXPECT_EQ(round_int32(8612, -5), 0) tests the boundary conditions. For 5 decimal places, the num before rounding is 0.08612 which rounds down to 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 closed pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
praveenbingo closed pull request #7885:
URL: https://github.com/apache/arrow/pull/7885
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r472724359
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +112,76 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+FORCE_INLINE
+gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
+ // for integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 9 is
+ // the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
+ if (abs_precision > 9) {
+ return 0;
+ }
+ gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
+ gdv_int32 remainder = number % power_of_10, quotient = number / power_of_10;
+ // if the fractional part of the quotient >= 0.5, round to next higher integer
+ if (remainder >= power_of_10 / 2) {
+ quotient++;
+ }
+ return quotient * power_of_10;
+}
+
+FORCE_INLINE
+gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
+ // for long integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 19 is
+ // the smallest N for which 10^N does not fit into 64 bits, so we can safely return 0
+ if (abs_precision > 18) {
+ return 0;
+ }
+ gdv_int64 power_of_10 = get_power_of_10(abs_precision);
+ gdv_int64 remainder = number % power_of_10, quotient = number / power_of_10;
Review comment:
would need to double check negative numbers.
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r478829379
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,22 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRound) {
+ EXPECT_EQ(round_int32_int32(85, -1), 90);
Review comment:
Yes, the result for round_int32_int32(-85, -1) should be -90. Thanks for pointing it out. Looking into the test failures.
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r472717430
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -111,6 +112,76 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
+FORCE_INLINE
+gdv_int32 round_int32_int32(gdv_int32 number, gdv_int32 precision) {
+ // for integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 9 is
+ // the smallest N for which 10^N does not fit into 32 bits, so we can safely return 0
+ if (abs_precision > 9) {
+ return 0;
+ }
+ gdv_int32 power_of_10 = static_cast<gdv_int32>(get_power_of_10(abs_precision));
+ gdv_int32 remainder = number % power_of_10, quotient = number / power_of_10;
+ // if the fractional part of the quotient >= 0.5, round to next higher integer
+ if (remainder >= power_of_10 / 2) {
+ quotient++;
+ }
+ return quotient * power_of_10;
+}
+
+FORCE_INLINE
+gdv_int64 round_int64_int32(gdv_int64 number, gdv_int32 precision) {
+ // for long integers, there is nothing following the decimal point,
+ // so round() always returns the same number if precision >= 0
+ if (precision >= 0 || number == 0) {
+ return number;
+ }
+ gdv_int32 abs_precision = -precision;
+ // This is to ensure that there is no overflow while calculating 10^precision, 19 is
+ // the smallest N for which 10^N does not fit into 64 bits, so we can safely return 0
+ if (abs_precision > 18) {
+ return 0;
+ }
+ gdv_int64 power_of_10 = get_power_of_10(abs_precision);
+ gdv_int64 remainder = number % power_of_10, quotient = number / power_of_10;
Review comment:
nit: two separate lines.
Further nit: I think you can get rid of a multiply and a divide you make the code:
```
number -= remainder;
if (remainder >= power_of_10 / 2) {
number += power_of_10;
}
return number;
```
----------------------------------------------------------------
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] emkornfield commented on a change in pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r465490152
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,19 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRound) {
+ EXPECT_EQ(round_int32(7589, -1), 7590);
Review comment:
these tests are a little hard to read but I don't think any of them actually check rounding at the boundary (digit = 5)
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#issuecomment-682297850
> With the two changes mentioned round_int32_int32 can be vectorized.
>
> https://godbolt.org/z/fabEn6
>
> With LLVM it actually doesn't necessarily seem to matter.
With the vectorized round_int32_int32, why are we passing -5 as the second argument? Would be glad if you could clarify.
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r478829379
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops_test.cc
##########
@@ -87,6 +87,22 @@ TEST(TestExtendedMathOps, TestLogWithBase) {
EXPECT_EQ(context1.has_error(), false);
}
+TEST(TestExtendedMathOps, TestRound) {
+ EXPECT_EQ(round_int32_int32(85, -1), 90);
Review comment:
Yes, the result for round_int32_int32(-85, -1) should be -90. The others seem reasonable too. Thanks for pointing it out. Looking into the test failures.
----------------------------------------------------------------
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 #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#discussion_r465480283
##########
File path: cpp/src/gandiva/precompiled/arithmetic_ops_test.cc
##########
@@ -101,4 +101,10 @@ TEST(TestArithmeticOps, TestDiv) {
context.Reset();
}
+TEST(TestArithmeticOps, TestRound) {
+ EXPECT_EQ(round_int32(758, -1), 760);
Review comment:
Added more test cases for negative numbers and boundary conditions.
----------------------------------------------------------------
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] emkornfield commented on pull request #7885: ARROW-9640: [C++][Gandiva] Implement round() for integers and long integers
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7885:
URL: https://github.com/apache/arrow/pull/7885#issuecomment-682300209
> With the vectorized round_int32_int32, why are we passing -5 as the second argument? Would be glad if you could clarify.
I wanted to verify that with a static value, the function could get the multiplications necessary vectorized, since I believe this is the common case, -5 was arbitrary. With a variable value it does not appear that the multiplications get vectorized.
----------------------------------------------------------------
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