lidavidm commented on a change in pull request #10349:
URL: https://github.com/apache/arrow/pull/10349#discussion_r696590014
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -53,16 +52,10 @@ class ARROW_EXPORT ElementWiseAggregateOptions : public
FunctionOptions {
bool skip_nulls;
};
-/// Functor to calculate hash of an enum.
-template <typename T, typename R = typename std::underlying_type<T>::type,
- typename = std::enable_if<std::is_enum<T>::value>>
-struct EnumHash {
- R operator()(T val) const { return static_cast<R>(val); }
-};
-
/// Rounding and tie-breaking modes for round compute functions.
/// Additional details and examples are provided in compute.rst.
enum class RoundMode : int8_t {
+ // Note: The HALF values need to be last and the first HALF entry is
HALF_DOWN.
Review comment:
Why?
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -85,26 +78,32 @@ enum class RoundMode : int8_t {
HALF_TO_ODD,
};
+static constexpr double kDefaultAbsoluteTolerance = 1E-5;
+
class ARROW_EXPORT RoundOptions : public FunctionOptions {
public:
explicit RoundOptions(int64_t ndigits = 0,
- RoundMode round_mode = RoundMode::HALF_TO_EVEN);
+ RoundMode round_mode = RoundMode::HALF_TO_EVEN,
+ double atol = kDefaultAbsoluteTolerance);
constexpr static char const kTypeName[] = "RoundOptions";
static RoundOptions Defaults() { return RoundOptions(); }
/// Rounding precision (number of digits to round to).
int64_t ndigits;
RoundMode round_mode;
+ double atol;
Review comment:
Also it needs Python bindings.
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -85,26 +78,32 @@ enum class RoundMode : int8_t {
HALF_TO_ODD,
};
+static constexpr double kDefaultAbsoluteTolerance = 1E-5;
+
class ARROW_EXPORT RoundOptions : public FunctionOptions {
public:
explicit RoundOptions(int64_t ndigits = 0,
- RoundMode round_mode = RoundMode::HALF_TO_EVEN);
+ RoundMode round_mode = RoundMode::HALF_TO_EVEN,
+ double atol = kDefaultAbsoluteTolerance);
constexpr static char const kTypeName[] = "RoundOptions";
static RoundOptions Defaults() { return RoundOptions(); }
/// Rounding precision (number of digits to round to).
int64_t ndigits;
RoundMode round_mode;
+ double atol;
Review comment:
It would be good to document what this field does.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -856,10 +860,23 @@ struct LogbChecked {
struct RoundUtil {
template <typename T>
- static enable_if_t<std::is_floating_point<T>::value, bool> ApproxHalfInt(
- const T val, const T atol = 1e-5) {
- // |frac| ~ 0.5?
- return std::fabs(std::fmod(std::fabs(val), T(1)) - T(0.5)) <=
std::fabs(atol);
+ static constexpr enable_if_t<std::is_floating_point<T>::value, bool>
IsApproxEqual(
+ const T a, const T b, const T atol = kDefaultAbsoluteTolerance) {
+ return std::fabs(a - b) <= std::fabs(atol);
Review comment:
I would assume atol is always nonnegative.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1444,7 +1463,7 @@ std::shared_ptr<ScalarFunction>
MakeUnaryRoundFunction(std::string name,
auto func = std::make_shared<ArithmeticFloatingPointFunction>(name,
Arity::Unary(), doc,
&kDefaultOptions);
for (const auto& ty : FloatingPointTypes()) {
- std::unordered_map<RoundMode, ArrayKernelExec, EnumHash<RoundMode>>
+ std::unordered_map<RoundMode, ArrayKernelExec,
::arrow::internal::EnumHash<RoundMode>>
Review comment:
Is a map really necessary here vs a switch-case?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1444,7 +1463,7 @@ std::shared_ptr<ScalarFunction>
MakeUnaryRoundFunction(std::string name,
auto func = std::make_shared<ArithmeticFloatingPointFunction>(name,
Arity::Unary(), doc,
&kDefaultOptions);
for (const auto& ty : FloatingPointTypes()) {
- std::unordered_map<RoundMode, ArrayKernelExec, EnumHash<RoundMode>>
+ std::unordered_map<RoundMode, ArrayKernelExec,
::arrow::internal::EnumHash<RoundMode>>
Review comment:
I suppose it's not a big deal since this is only run once.
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -53,16 +52,10 @@ class ARROW_EXPORT ElementWiseAggregateOptions : public
FunctionOptions {
bool skip_nulls;
};
-/// Functor to calculate hash of an enum.
-template <typename T, typename R = typename std::underlying_type<T>::type,
- typename = std::enable_if<std::is_enum<T>::value>>
-struct EnumHash {
- R operator()(T val) const { return static_cast<R>(val); }
-};
-
/// Rounding and tie-breaking modes for round compute functions.
/// Additional details and examples are provided in compute.rst.
enum class RoundMode : int8_t {
+ // Note: The HALF values need to be last and the first HALF entry is
HALF_DOWN.
Review comment:
Ok, I see why below - but it is a little confusing to put this into a
public API header. Some static asserts in the cc file might serve you better?
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -85,26 +78,32 @@ enum class RoundMode : int8_t {
HALF_TO_ODD,
};
+static constexpr double kDefaultAbsoluteTolerance = 1E-5;
+
class ARROW_EXPORT RoundOptions : public FunctionOptions {
public:
explicit RoundOptions(int64_t ndigits = 0,
- RoundMode round_mode = RoundMode::HALF_TO_EVEN);
+ RoundMode round_mode = RoundMode::HALF_TO_EVEN,
+ double atol = kDefaultAbsoluteTolerance);
Review comment:
'atol' makes me think of std::atol even though it seems it's 'absolute
tolerance' - maybe expand the name?
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -896,7 +915,8 @@ struct RoundImpl<T, RoundMode::UP> {
template <typename T>
struct RoundImpl<T, RoundMode::TOWARDS_ZERO> {
static constexpr enable_if_floating_point<T> Round(const T val) {
- return std::trunc(val);
+ // return std::trunc(val);
Review comment:
nit: commented code?
--
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]
