Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
zanmato1984 commented on code in PR #46792:
URL: https://github.com/apache/arrow/pull/46792#discussion_r2163594171
##
cpp/src/arrow/status.h:
##
@@ -463,13 +480,16 @@ Status& Status::operator&=(Status&& s) noexcept {
}
/// \endcond
-namespace internal {
-
-// Extract Status from Status or Result
-// Useful for the status check macros such as RETURN_NOT_OK.
-inline const Status& GenericToStatus(const Status& st) { return st; }
-inline Status GenericToStatus(Status&& st) { return std::move(st); }
+constexpr inline const Status& ToArrowStatus(const Status& st) { return st; }
Review Comment:
+1 to @bkietz 's proposal.
--
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]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
pitrou commented on code in PR #46792:
URL: https://github.com/apache/arrow/pull/46792#discussion_r2163175379
##
cpp/src/arrow/status.h:
##
@@ -463,13 +480,16 @@ Status& Status::operator&=(Status&& s) noexcept {
}
/// \endcond
-namespace internal {
-
-// Extract Status from Status or Result
-// Useful for the status check macros such as RETURN_NOT_OK.
-inline const Status& GenericToStatus(const Status& st) { return st; }
-inline Status GenericToStatus(Status&& st) { return std::move(st); }
+constexpr inline const Status& ToArrowStatus(const Status& st) { return st; }
Review Comment:
That's an interesting suggestion. On the one hand it's more verbose. On the
other hand it avoids the ADL shenanigans, so it might be for the better.
@zanmato1984 What do you 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
pitrou commented on code in PR #46792:
URL: https://github.com/apache/arrow/pull/46792#discussion_r2163173654
##
cpp/src/arrow/status.h:
##
@@ -463,13 +480,16 @@ Status& Status::operator&=(Status&& s) noexcept {
}
/// \endcond
-namespace internal {
-
-// Extract Status from Status or Result
-// Useful for the status check macros such as RETURN_NOT_OK.
-inline const Status& GenericToStatus(const Status& st) { return st; }
-inline Status GenericToStatus(Status&& st) { return std::move(st); }
+constexpr inline const Status& ToArrowStatus(const Status& st) { return st; }
+static inline Status ToArrowStatus(Status&& st) { return st; }
+namespace internal {
+// A trivial wrapper so that macros don't have to namespace-quality
ArrowToStatus
+// (otherwise argument-dependent lookup would be disabled).
+template
+Status CallToArrowStatus(T&& t) {
Review Comment:
Yes, it is.
--
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]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
bkietz commented on code in PR #46792:
URL: https://github.com/apache/arrow/pull/46792#discussion_r2150698929
##
cpp/src/arrow/status.h:
##
@@ -350,6 +343,30 @@ class ARROW_EXPORT [[nodiscard]] Status : public
util::EqualityComparable(args)...).WithDetail(detail());
}
+ /// \brief Apply a functor if the status indicates an error
+ ///
+ /// If the status indicates a success, it is returned as-is.
+ ///
+ /// If the status indicates an error, the given functor is called with the
status
+ /// as argument.
+ /// If the functor returns a new Status, it is returned.
+ /// If the functor returns a Status-compatible object such as Result, it
is
+ /// converted to Status and returned.
+ /// If the functor returns void, the original Status is returned.
+ template
+ Status OrElse(OnError&& on_error) {
+using RT = decltype(on_error(Status(StatusCode::UnknownError, "")));
Review Comment:
```suggestion
using RT = decltype(on_error(Status()));
```
Since this is unevaluated, we don't need to force an error status
##
cpp/src/arrow/status.h:
##
@@ -463,13 +480,16 @@ Status& Status::operator&=(Status&& s) noexcept {
}
/// \endcond
-namespace internal {
-
-// Extract Status from Status or Result
-// Useful for the status check macros such as RETURN_NOT_OK.
-inline const Status& GenericToStatus(const Status& st) { return st; }
-inline Status GenericToStatus(Status&& st) { return std::move(st); }
+constexpr inline const Status& ToArrowStatus(const Status& st) { return st; }
Review Comment:
What you're proposing here is essentially a trait; in order to extend
`my_namespace::Err` with convertibility to status, we implement
`my_namespace::ToArrowStatus`. This extensibility sounds quite useful, but it
might be more robust and easier to explain if it were provided as a
user-specializable template struct as with many other traits:
```c++
namespace arrow::internal {
template
struct IntoStatus;
}
namespace arrow {
/// The actual entry point
template
constexpr decltype(auto) ToStatus(T&& t) {
return IntoStatus::ToStatus(std::forward(t));
}
}
namespace arrow::internal {
template <>
struct IntoStatus {
static constexpr Status& ToStatus(const Status& st) { return st; }
static constexpr Status&& ToStatus(Status&& st) { return st; }
};
template
struct IntoStatus> {
static constexpr Status& ToStatus(const Result& r) { return r.status();
}
static constexpr Status&& ToStatus(Result&& st) { return
std::move(r).status(); }
};
} // namespace arrow
// ...
namespace arrow::internal {
template <>
struct IntoStatus {
static Status ToStatus(my_namespace::StatusLike v) {
if (v.value == 42) {
return Status::OK();
} else {
return Status::UnknownError("StatusLike: ", v.value);
}
}
};
} // namespace arrow::internal
```
##
cpp/src/arrow/status.h:
##
@@ -350,6 +343,30 @@ class ARROW_EXPORT [[nodiscard]] Status : public
util::EqualityComparable(args)...).WithDetail(detail());
}
+ /// \brief Apply a functor if the status indicates an error
+ ///
+ /// If the status indicates a success, it is returned as-is.
+ ///
+ /// If the status indicates an error, the given functor is called with the
status
+ /// as argument.
+ /// If the functor returns a new Status, it is returned.
+ /// If the functor returns a Status-compatible object such as Result, it
is
+ /// converted to Status and returned.
+ /// If the functor returns void, the original Status is returned.
+ template
+ Status OrElse(OnError&& on_error) {
+using RT = decltype(on_error(Status(StatusCode::UnknownError, "")));
+if (ARROW_PREDICT_TRUE(ok())) {
+ return *this;
+}
+if constexpr (std::is_same_v) {
Review Comment:
```suggestion
if constexpr (std::is_void_v) {
```
--
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]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
zanmato1984 commented on code in PR #46792:
URL: https://github.com/apache/arrow/pull/46792#discussion_r2143811007
##
cpp/src/arrow/status.h:
##
@@ -463,13 +480,16 @@ Status& Status::operator&=(Status&& s) noexcept {
}
/// \endcond
-namespace internal {
-
-// Extract Status from Status or Result
-// Useful for the status check macros such as RETURN_NOT_OK.
-inline const Status& GenericToStatus(const Status& st) { return st; }
-inline Status GenericToStatus(Status&& st) { return std::move(st); }
+constexpr inline const Status& ToArrowStatus(const Status& st) { return st; }
+static inline Status ToArrowStatus(Status&& st) { return st; }
+namespace internal {
+// A trivial wrapper so that macros don't have to namespace-quality
ArrowToStatus
+// (otherwise argument-dependent lookup would be disabled).
+template
+Status CallToArrowStatus(T&& t) {
Review Comment:
This is to enable macros like `RETURN_NOT_OK()` to accept customized
`ToArrowStatus()` by ADL?
##
cpp/src/arrow/status.h:
##
@@ -463,13 +480,16 @@ Status& Status::operator&=(Status&& s) noexcept {
}
/// \endcond
-namespace internal {
-
-// Extract Status from Status or Result
-// Useful for the status check macros such as RETURN_NOT_OK.
-inline const Status& GenericToStatus(const Status& st) { return st; }
-inline Status GenericToStatus(Status&& st) { return std::move(st); }
+constexpr inline const Status& ToArrowStatus(const Status& st) { return st; }
+static inline Status ToArrowStatus(Status&& st) { return st; }
+namespace internal {
+// A trivial wrapper so that macros don't have to namespace-quality
ArrowToStatus
Review Comment:
```suggestion
// A trivial wrapper so that macros don't have to namespace-qualify
ArrowToStatus
```
--
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]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
pitrou commented on PR #46792: URL: https://github.com/apache/arrow/pull/46792#issuecomment-2967464422 > The behaviour seems to line up exactly with Rust's `.or_else()` You can guess it's intended :) -- 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]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
paleolimbot commented on code in PR #46792: URL: https://github.com/apache/arrow/pull/46792#discussion_r2143031199 ## cpp/src/arrow/status.h: ## @@ -350,6 +343,30 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable(args)...).WithDetail(detail()); } + /// \brief Apply a functor if the status indicates an error + /// + /// If the status indicates a success, it is returned as-is. Review Comment: ```suggestion /// /// This can be used to perform cleanup actions or integrate /// Status handling with non-Arrow error handling. /// If the status indicates a success, it is returned as-is. ``` (Or the actual reason if this isn't it!) -- 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]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
pitrou commented on PR #46792: URL: https://github.com/apache/arrow/pull/46792#issuecomment-2965902116 @zanmato1984 @bkietz @paleolimbot What do you 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
github-actions[bot] commented on PR #46792: URL: https://github.com/apache/arrow/pull/46792#issuecomment-2965886040 :warning: GitHub issue #46791 **has been automatically assigned in GitHub** to PR creator. -- 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]
Re: [PR] GH-46791: [C++] Add `Status::OrElse` [arrow]
pitrou commented on PR #46792:
URL: https://github.com/apache/arrow/pull/46792#issuecomment-2965930461
Ah, clang is being picky.
```
/Users/runner/work/arrow/arrow/cpp/src/arrow/status.h:363:16: error: cannot
pass object of non-trivial type 'arrow::Status' through variadic method; call
will abort at runtime [-Wnon-pod-varargs]
on_error(*this);
^
/Users/runner/work/arrow/arrow/cpp/src/parquet/arrow/writer.cc:392:31: note:
in instantiation of function template specialization
'arrow::Status::OrElse<(lambda at
/Users/runner/work/arrow/arrow/cpp/src/parquet/arrow/writer.cc:391:7)>'
requested here
WriteRowGroup(0, 0).OrElse([&](...) {
PARQUET_IGNORE_NOT_OK(Close()); }));
^
```
--
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]
