felipecrv commented on code in PR #41559:
URL: https://github.com/apache/arrow/pull/41559#discussion_r1768817233
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -876,6 +876,18 @@ if(ARROW_FILESYSTEM)
foreach(ARROW_FILESYSTEM_TARGET ${ARROW_FILESYSTEM_TARGETS})
target_link_libraries(${ARROW_FILESYSTEM_TARGET} PRIVATE
${AWSSDK_LINK_LIBRARIES})
endforeach()
+
+ if(ARROW_S3_MODULE)
+ if(NOT ARROW_BUILD_SHARED)
+ message(FATAL_ERROR "ARROW_S3_MODULE without shared libarrow is not
supported")
+ endif()
+
+ add_library(arrow_s3fs MODULE filesystem/s3fs_module.cc
filesystem/s3fs.cc)
+ target_link_libraries(arrow_s3fs PRIVATE ${AWSSDK_LINK_LIBRARIES}
arrow_shared)
+ set_source_files_properties(filesystem/s3fs.cc filesystem/s3fs_module.cc
+ PROPERTIES SKIP_PRECOMPILE_HEADERS ON
+ SKIP_UNITY_BUILD_INCLUSION ON)
Review Comment:
Do we remove the linking of the main arrow library against
`AWSSDK_LINK_LIBRARIES` when `ARROW_S3_MODULE` is ON?
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -876,6 +876,18 @@ if(ARROW_FILESYSTEM)
foreach(ARROW_FILESYSTEM_TARGET ${ARROW_FILESYSTEM_TARGETS})
target_link_libraries(${ARROW_FILESYSTEM_TARGET} PRIVATE
${AWSSDK_LINK_LIBRARIES})
endforeach()
+
+ if(ARROW_S3_MODULE)
+ if(NOT ARROW_BUILD_SHARED)
+ message(FATAL_ERROR "ARROW_S3_MODULE without shared libarrow is not
supported")
Review Comment:
I find it super helpful when these messages let me copy and paste the option
I need to pass to my CMake line instead of leaving me to figure out the flag
myself
```suggestion
message(FATAL_ERROR "ARROW_S3_MODULE without -DARROW_BUILD_SHARED=ON
is not supported")
```
##########
cpp/src/arrow/filesystem/CMakeLists.txt:
##########
@@ -121,6 +121,24 @@ if(ARROW_S3)
target_link_libraries(arrow-filesystem-s3fs-benchmark PRIVATE
parquet_shared)
endif()
endif()
+
+ if(ARROW_S3_MODULE)
+ add_arrow_test(s3fs_module_test
+ SOURCES
+ s3fs_module_test.cc
+ s3_test_util.cc
+ EXTRA_LABELS
+ filesystem
+ DEFINITIONS
+ ARROW_S3_LIBPATH="$<TARGET_FILE:arrow_s3fs>"
+ EXTRA_LINK_LIBS
+ Boost::filesystem
+ Boost::system)
Review Comment:
@assignUser does the auto-formatter for CMake has the ability to indent
these?
```cmake
SOURCES
s3fs_module_test.cc
s3_test_util.cc
EXTRA_LABELS
filesystem
DEFINITIONS
ARROW_S3_LIBPATH="$<TARGET_FILE:arrow_s3fs>"
EXTRA_LINK_LIBS
Boost::filesystem
Boost::system)
```
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -3031,6 +3034,30 @@ Result<std::string> S3FileSystem::PathFromUri(const
std::string& uri_string) con
internal::AuthorityHandlingBehavior::kPrepend);
}
+Result<std::string> S3FileSystem::MakeUri(std::string path) const {
+ if (path.length() <= 1 || path[0] != '/') {
+ return Status::Invalid("MakeUri requires an absolute, non-root path, got
", path);
+ }
+ ARROW_ASSIGN_OR_RAISE(auto uri, util::UriFromAbsolutePath(path));
+ if (!options().GetAccessKey().empty()) {
+ uri = "s3://" + options().GetAccessKey() + ":" + options().GetSecretKey()
+ "@" +
+ uri.substr("file:///"s.size());
+ } else {
+ uri = "s3" + uri.substr("file"s.size());
+ }
+ uri += "?";
+ uri += "region=" + util::UriEscape(options().region);
+ uri += "&";
+ uri += "scheme=" + options().scheme;
+ uri += "&";
+ uri += "endpoint_override=" + util::UriEscape(options().endpoint_override);
+ uri += "&";
+ uri += "allow_bucket_creation="s + (options().allow_bucket_creation ? "1" :
"0");
+ uri += "&";
+ uri += "allow_bucket_deletion="s + (options().allow_bucket_deletion ? "1" :
"0");
+ return uri;
+}
Review Comment:
We didn't have this code already anywhere else?
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -3031,6 +3034,30 @@ Result<std::string> S3FileSystem::PathFromUri(const
std::string& uri_string) con
internal::AuthorityHandlingBehavior::kPrepend);
}
+Result<std::string> S3FileSystem::MakeUri(std::string path) const {
+ if (path.length() <= 1 || path[0] != '/') {
+ return Status::Invalid("MakeUri requires an absolute, non-root path, got
", path);
+ }
+ ARROW_ASSIGN_OR_RAISE(auto uri, util::UriFromAbsolutePath(path));
+ if (!options().GetAccessKey().empty()) {
+ uri = "s3://" + options().GetAccessKey() + ":" + options().GetSecretKey()
+ "@" +
+ uri.substr("file:///"s.size());
Review Comment:
Should these be in URIs? Has this always been the case?
--
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]
