js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1225211390
##########
cpp/src/arrow/compute/function_internal.h:
##########
@@ -510,6 +491,29 @@ static inline enable_if_same_result<T, Datum>
GenericFromScalar(
return Status::Invalid("Cannot deserialize Datum from ", value->ToString());
}
+template <typename>
+constexpr bool is_optional_impl = false;
+template <typename T>
+constexpr bool is_optional_impl<std::optional<T>> = true;
+
+template <typename T>
+using is_optional =
+ std::integral_constant<bool, is_optional_impl<std::decay_t<T>> ||
+ std::is_same<T, std::nullopt_t>::value>;
+
+template <typename T, typename R = void>
+using enable_if_optional = enable_if_t<is_optional<T>::value, Result<T>>;
+
+template <typename T>
+static inline enable_if_optional<T> GenericFromScalar(
Review Comment:
The current implementation for `GenericFromScalar` and `GenericToScalar` is
buggy for `std::optional`.` GenericToScalar(std::nullopt)` creates a
StringScalar with an empty string. Thus
`GenericFromScalar(GenericToScalar(std::nullopt))` != std::nullopt`.
I used Scalar(NullType) to represent std::nullopt which should be fine
because AFAIU nothing else would be serialized to NullType.
--
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]
