wgtmac commented on code in PR #404:
URL: https://github.com/apache/iceberg-cpp/pull/404#discussion_r2605785144
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -96,16 +100,37 @@ bool IsSuccessful(int32_t status_code) {
|| status_code == 304; // Not Modified
}
+/// \brief Builds a default ErrorResponse when the response body cannot be
parsed.
+ErrorResponse BuildDefaultErrorResponse(const cpr::Response& response) {
+ return {
+ .code = static_cast<uint32_t>(response.status_code),
+ .type = std::string(kRestExceptionType),
+ .message = !response.reason.empty() ? response.reason
+ :
GetStandardReasonPhrase(response.status_code),
+ };
+}
+
+/// \brief Tries to parse the response body as an ErrorResponse.
+Result<ErrorResponse> TryParseErrorResponse(const std::string& text) {
+ if (text.empty()) {
+ return InvalidArgument("Empty response body");
+ }
+ ICEBERG_ASSIGN_OR_RAISE(auto json_result, FromJsonString(text));
+ ICEBERG_ASSIGN_OR_RAISE(auto error_result,
ErrorResponseFromJson(json_result));
+ return error_result;
+}
+
/// \brief Handles failure responses by invoking the provided error handler.
Status HandleFailureResponse(const cpr::Response& response,
const ErrorHandler& error_handler) {
- if (!IsSuccessful(response.status_code)) {
- // TODO(gangwu): response status code is lost, wrap it with RestError.
- ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.text));
- ICEBERG_ASSIGN_OR_RAISE(auto error_response, ErrorResponseFromJson(json));
- return error_handler.Accept(error_response);
+ if (IsSuccessful(response.status_code)) {
+ return {};
}
- return {};
+ auto parse_result = TryParseErrorResponse(response.text);
+ const ErrorResponse final_error = parse_result.has_value()
Review Comment:
You may use `parse_result.value_or(...)` to make it simpler.
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -115,29 +117,67 @@ Result<std::vector<Namespace>>
RestCatalog::ListNamespaces(const Namespace& ns)
}
Status RestCatalog::CreateNamespace(
- [[maybe_unused]] const Namespace& ns,
- [[maybe_unused]] const std::unordered_map<std::string, std::string>&
properties) {
- return NotImplemented("Not implemented");
+ const Namespace& ns, const std::unordered_map<std::string, std::string>&
properties) {
+ ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespaces());
+ CreateNamespaceRequest request{.namespace_ = ns, .properties = properties};
+ ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request)));
+ ICEBERG_ASSIGN_OR_RAISE(const auto& response,
+ client_->Post(endpoint, json_request, /*headers=*/{},
+ *NamespaceErrorHandler::Instance()));
+ ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body()));
+ ICEBERG_ASSIGN_OR_RAISE(auto create_response,
CreateNamespaceResponseFromJson(json));
+ return {};
}
Result<std::unordered_map<std::string, std::string>>
RestCatalog::GetNamespaceProperties(
- [[maybe_unused]] const Namespace& ns) const {
- return NotImplemented("Not implemented");
-}
-
-Status RestCatalog::DropNamespace([[maybe_unused]] const Namespace& ns) {
- return NotImplemented("Not implemented");
+ const Namespace& ns) const {
+ ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespace_(ns));
+ ICEBERG_ASSIGN_OR_RAISE(const auto& response,
+ client_->Get(endpoint, /*params=*/{}, /*headers=*/{},
+ *NamespaceErrorHandler::Instance()));
+ ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body()));
+ ICEBERG_ASSIGN_OR_RAISE(auto get_response,
GetNamespaceResponseFromJson(json));
+ return get_response.properties;
}
-Result<bool> RestCatalog::NamespaceExists([[maybe_unused]] const Namespace&
ns) const {
- return NotImplemented("Not implemented");
+Status RestCatalog::DropNamespace(const Namespace& ns) {
+ ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespace_(ns));
+ ICEBERG_ASSIGN_OR_RAISE(
+ const auto& response,
+ client_->Delete(endpoint, /*headers=*/{},
*DropNamespaceErrorHandler::Instance()));
+ return {};
+}
+
+Result<bool> RestCatalog::NamespaceExists(const Namespace& ns) const {
+ ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespace_(ns));
+ auto response_or_error =
Review Comment:
This impl is not consistent with Java where backward compatibility is
considered to use GetNamespaceProperties as a work around.
##########
src/iceberg/catalog/rest/rest_util.cc:
##########
@@ -120,4 +122,45 @@ std::unordered_map<std::string, std::string> MergeConfigs(
return merged;
}
+std::string GetStandardReasonPhrase(int32_t status_code) {
+ switch (status_code) {
Review Comment:
Why only a subset of the Java impl?
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -115,29 +117,67 @@ Result<std::vector<Namespace>>
RestCatalog::ListNamespaces(const Namespace& ns)
}
Status RestCatalog::CreateNamespace(
- [[maybe_unused]] const Namespace& ns,
- [[maybe_unused]] const std::unordered_map<std::string, std::string>&
properties) {
- return NotImplemented("Not implemented");
+ const Namespace& ns, const std::unordered_map<std::string, std::string>&
properties) {
+ ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespaces());
+ CreateNamespaceRequest request{.namespace_ = ns, .properties = properties};
+ ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request)));
+ ICEBERG_ASSIGN_OR_RAISE(const auto& response,
Review Comment:
```suggestion
ICEBERG_ASSIGN_OR_RAISE(const auto response,
```
This usage is not correct. Please also fix all similar places below and that
in the `RestCatalog::ListNamespaces`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
