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/15 10:01:44 UTC
[GitHub] [arrow] sagnikc-dremio opened a new pull request #8467: ARROW-10310: [C++][Gandiva] Add single argument round() in Gandiva
sagnikc-dremio opened a new pull request #8467:
URL: https://github.com/apache/arrow/pull/8467
----------------------------------------------------------------
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 #8467: ARROW-10310: [C++][Gandiva] Add single argument round() in Gandiva
Posted by GitBox <gi...@apache.org>.
praveenbingo closed pull request #8467:
URL: https://github.com/apache/arrow/pull/8467
----------------------------------------------------------------
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 #8467: ARROW-10310: [C++][Gandiva] Add single argument round() in Gandiva
Posted by GitBox <gi...@apache.org>.
pprudhvi commented on a change in pull request #8467:
URL: https://github.com/apache/arrow/pull/8467#discussion_r506180824
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -122,8 +138,8 @@ POWER(float64, float64, float64)
scale_multiplier); \
}
-ROUND_DECIMAL(float32)
-ROUND_DECIMAL(float64)
+ROUND_DECIMAL_TWO_ARG(float32)
Review comment:
can we rename this to say something like 'round_decimal_to_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] pprudhvi commented on pull request #8467: ARROW-10310: [C++][Gandiva] Add single argument round() in Gandiva
Posted by GitBox <gi...@apache.org>.
pprudhvi commented on pull request #8467:
URL: https://github.com/apache/arrow/pull/8467#issuecomment-709915839
LGTM, added comments to rename macros.
@praveenbingo , could you please take a look and merge?
----------------------------------------------------------------
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 #8467: ARROW-10310: [C++][Gandiva] Add single argument round() in Gandiva
Posted by GitBox <gi...@apache.org>.
sagnikc-dremio commented on a change in pull request #8467:
URL: https://github.com/apache/arrow/pull/8467#discussion_r506215354
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -122,8 +138,8 @@ POWER(float64, float64, float64)
scale_multiplier); \
}
-ROUND_DECIMAL(float32)
-ROUND_DECIMAL(float64)
+ROUND_DECIMAL_TWO_ARG(float32)
Review comment:
Addressed the comments.
----------------------------------------------------------------
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 #8467: ARROW-10310: [C++][Gandiva] Add single argument round() in Gandiva
Posted by GitBox <gi...@apache.org>.
pprudhvi commented on a change in pull request #8467:
URL: https://github.com/apache/arrow/pull/8467#discussion_r506181737
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -112,8 +112,24 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
-// round
-#define ROUND_DECIMAL(TYPE) \
+FORCE_INLINE
+gdv_int32 round_int32(gdv_int32 num) { return num; }
+
+FORCE_INLINE
+gdv_int64 round_int64(gdv_int64 num) { return num; }
+
+// one-argument round
+#define ROUND_DECIMAL_ONE_ARG(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE(gdv_##TYPE num) { \
+ return static_cast<gdv_##TYPE>(trunc(num + ((num > 0) ? 0.5 : -0.5))); \
+ }
+
+ROUND_DECIMAL_ONE_ARG(float32)
+ROUND_DECIMAL_ONE_ARG(float64)
+
+// two-argument round
Review comment:
could you change the comment to say that this rounds to the given 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] github-actions[bot] commented on pull request #8467: ARROW-10310: [C++][Gandiva] Add single argument round() in Gandiva
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8467:
URL: https://github.com/apache/arrow/pull/8467#issuecomment-709073663
https://issues.apache.org/jira/browse/ARROW-10310
----------------------------------------------------------------
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 #8467: ARROW-10310: [C++][Gandiva] Add single argument round() in Gandiva
Posted by GitBox <gi...@apache.org>.
pprudhvi commented on a change in pull request #8467:
URL: https://github.com/apache/arrow/pull/8467#discussion_r506181132
##########
File path: cpp/src/gandiva/precompiled/extended_math_ops.cc
##########
@@ -112,8 +112,24 @@ LOG_WITH_BASE(float64, float64, float64)
POWER(float64, float64, float64)
-// round
-#define ROUND_DECIMAL(TYPE) \
+FORCE_INLINE
+gdv_int32 round_int32(gdv_int32 num) { return num; }
+
+FORCE_INLINE
+gdv_int64 round_int64(gdv_int64 num) { return num; }
+
+// one-argument round
+#define ROUND_DECIMAL_ONE_ARG(TYPE) \
+ FORCE_INLINE \
+ gdv_##TYPE round_##TYPE(gdv_##TYPE num) { \
+ return static_cast<gdv_##TYPE>(trunc(num + ((num > 0) ? 0.5 : -0.5))); \
+ }
+
+ROUND_DECIMAL_ONE_ARG(float32)
Review comment:
let this be just 'round_decimal'
----------------------------------------------------------------
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