wgtmac commented on code in PR #484:
URL: https://github.com/apache/iceberg-cpp/pull/484#discussion_r2663593102
##########
CMakeLists.txt:
##########
@@ -56,6 +56,7 @@ set(ICEBERG_INSTALL_CMAKEDIR "${CMAKE_INSTALL_LIBDIR}/cmake")
set(ICEBERG_INSTALL_DOCDIR "share/doc/iceberg")
if(WIN32 AND NOT MINGW)
+ add_compile_options(/bigobj)
Review Comment:
We need to use target oriented approach like below:
```
diff --git a/cmake_modules/IcebergBuildUtils.cmake
b/cmake_modules/IcebergBuildUtils.cmake
index 99f57d92..74b02970 100644
--- a/cmake_modules/IcebergBuildUtils.cmake
+++ b/cmake_modules/IcebergBuildUtils.cmake
@@ -157,6 +157,10 @@ function(add_iceberg_lib LIB_NAME)
hidden
VISIBILITY_INLINES_HIDDEN 1)
+ if(MSVC_TOOLCHAIN)
+ target_compile_options(${LIB_NAME}_shared PRIVATE /bigobj)
+ endif()
+
install(TARGETS ${LIB_NAME}_shared
EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
@@ -220,6 +224,10 @@ function(add_iceberg_lib LIB_NAME)
target_compile_definitions(${LIB_NAME}_static PUBLIC
${VISIBILITY_NAME}_STATIC)
endif()
+ if(MSVC_TOOLCHAIN)
+ target_compile_options(${LIB_NAME}_static PRIVATE /bigobj)
+ endif()
+
install(TARGETS ${LIB_NAME}_static
EXPORT iceberg_targets
ARCHIVE DESTINATION ${INSTALL_ARCHIVE_DIR}
(END)
```
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -140,6 +140,10 @@ void HttpClient::PrepareSession(
session_->SetUrl(cpr::Url{path});
session_->SetParameters(GetParameters(params));
session_->RemoveContent();
+ // clear lingering POST mode state from prior requests. CURLOPT_POST is
implicitly set
+ // to 1 by POST requests, and this state is not reset by RemoveContent(), so
we must
+ // manually enforce HTTP GET to clear it.
+ curl_easy_setopt(session_->GetCurlHolder()->handle, CURLOPT_HTTPGET, 1L);
Review Comment:
I just quickly scanned cpr code and found these:
```cpp
void Session::PrepareDelete() {
curl_easy_setopt(curl_->handle, CURLOPT_HTTPGET, 0L);
curl_easy_setopt(curl_->handle, CURLOPT_NOBODY, 0L);
curl_easy_setopt(curl_->handle, CURLOPT_CUSTOMREQUEST, "DELETE");
prepareCommon();
}
void Session::PrepareGet() {
// In case there is a body or payload for this request, we create a
custom GET-Request since a
// GET-Request with body is based on the HTTP RFC **not** a leagal
request.
if (hasBodyOrPayload()) {
curl_easy_setopt(curl_->handle, CURLOPT_NOBODY, 0L);
curl_easy_setopt(curl_->handle, CURLOPT_CUSTOMREQUEST, "GET");
} else {
curl_easy_setopt(curl_->handle, CURLOPT_NOBODY, 0L);
curl_easy_setopt(curl_->handle, CURLOPT_CUSTOMREQUEST, nullptr);
curl_easy_setopt(curl_->handle, CURLOPT_HTTPGET, 1L);
}
prepareCommon();
}
void Session::PrepareHead() {
curl_easy_setopt(curl_->handle, CURLOPT_NOBODY, 1L);
curl_easy_setopt(curl_->handle, CURLOPT_CUSTOMREQUEST, nullptr);
prepareCommon();
}
void Session::PreparePost() {
curl_easy_setopt(curl_->handle, CURLOPT_NOBODY, 0L);
// In case there is no body or payload set it to an empty post:
if (hasBodyOrPayload()) {
curl_easy_setopt(curl_->handle, CURLOPT_CUSTOMREQUEST, nullptr);
} else {
curl_easy_setopt(curl_->handle, CURLOPT_POSTFIELDS,
cbs_->readcb_.callback ? nullptr : "");
curl_easy_setopt(curl_->handle, CURLOPT_CUSTOMREQUEST, "POST");
}
prepareCommon();
}
```
So a quick fix is to pass `HttpMethod` enum to `PrepareSession` and call
respective `session_->PrepareXXX`.
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -280,10 +303,33 @@ Result<std::shared_ptr<Table>> RestCatalog::CreateTable(
}
Result<std::shared_ptr<Table>> RestCatalog::UpdateTable(
- [[maybe_unused]] const TableIdentifier& identifier,
- [[maybe_unused]] const std::vector<std::unique_ptr<TableRequirement>>&
requirements,
- [[maybe_unused]] const std::vector<std::unique_ptr<TableUpdate>>& updates)
{
- return NotImplemented("Not implemented");
+ const TableIdentifier& identifier,
+ const std::vector<std::unique_ptr<TableRequirement>>& requirements,
+ const std::vector<std::unique_ptr<TableUpdate>>& updates) {
+ ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::UpdateTable());
+ ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Table(identifier));
+
+ CommitTableRequest request{.identifier = identifier};
+ request.requirements.reserve(requirements.size());
+ for (const auto& req : requirements) {
+ request.requirements.push_back(req->Clone());
+ }
+ request.updates.reserve(updates.size());
+ for (const auto& update : updates) {
+ request.updates.push_back(update->Clone());
+ }
+
+ ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request)));
+ ICEBERG_ASSIGN_OR_RAISE(
+ const auto response,
+ client_->Post(path, json_request, /*headers=*/{},
*TableErrorHandler::Instance()));
+
+ ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body()));
+ ICEBERG_ASSIGN_OR_RAISE(auto commit_response,
CommitTableResponseFromJson(json));
+
+ return Table::Make(identifier, commit_response.metadata,
Review Comment:
```suggestion
return Table::Make(identifier, std::move(commit_response.metadata),
```
--
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]
