[GitHub] [arrow] pitrou commented on a change in pull request #10712: ARROW-13307: [C++][Python][R] Use reflection-based enums for function options

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #10712:
URL: https://github.com/apache/arrow/pull/10712#discussion_r674084175



##########
File path: cpp/src/arrow/compute/function_internal.h
##########
@@ -112,9 +81,23 @@ static inline std::string GenericToString(const std::string& value) {
 }
 
 template <typename T>
-static inline enable_if_t<has_enum_traits<T>::value, std::string> GenericToString(
-    const T value) {
-  return EnumTraits<T>::value_name(value);
+static inline std::string GenericToString(const TimeUnit::type value) {
+  switch (value) {
+    case TimeUnit::type::SECOND:
+      return "SECOND";
+    case TimeUnit::type::MILLI:
+      return "MILLI";
+    case TimeUnit::type::MICRO:
+      return "MICRO";
+    case TimeUnit::type::NANO:
+      return "NANO";

Review comment:
       Shouldn't we use lowercase as for other option enums?
   (for example, I see you changed "Ascending" to "ascending")

##########
File path: cpp/src/arrow/compute/function_internal.h
##########
@@ -376,16 +359,31 @@ GenericFromScalar(const std::shared_ptr<Scalar>& value) {
   return holder.value;
 }
 
+template <typename T, typename U>
+using enable_if_same_result = enable_if_same<T, U, Result<T>>;
+
 template <typename T>
-static inline enable_if_primitive_ctype<typename EnumTraits<T>::Type, Result<T>>
-GenericFromScalar(const std::shared_ptr<Scalar>& value) {
-  ARROW_ASSIGN_OR_RAISE(auto raw_val,
-                        GenericFromScalar<typename EnumTraits<T>::CType>(value));
-  return ValidateEnumValue<T>(raw_val);
+static inline enable_if_same_result<T, TimeUnit::type> GenericFromScalar(
+    const std::shared_ptr<Scalar>& value) {
+  ARROW_ASSIGN_OR_RAISE(auto raw_val, GenericFromScalar<int32_t>(value));
+  switch (raw_val) {
+    case static_cast<int32_t>(TimeUnit::type::SECOND):
+      return TimeUnit::type::SECOND;
+    case static_cast<int32_t>(TimeUnit::type::MILLI):
+      return TimeUnit::type::MILLI;
+    case static_cast<int32_t>(TimeUnit::type::MICRO):
+      return TimeUnit::type::MICRO;
+    case static_cast<int32_t>(TimeUnit::type::NANO):
+      return TimeUnit::type::NANO;

Review comment:
       It looks bad that we have to spell this explicitly.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org