Copilot commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3811562346
##########
src/iceberg/resolving_file_io.cc:
##########
@@ -32,33 +35,22 @@
ResolvingFileIO::ResolvingFileIO(std::unordered_map<std::string, std::string> pr
ResolvingFileIO::~ResolvingFileIO() = default;
-Result<std::string_view> ResolveFileIOName(std::string_view location) {
- const auto pos = location.find("://");
- if (pos == std::string_view::npos) {
- return FileIORegistry::kArrowLocalFileIO;
- }
+Result<std::shared_ptr<FileIO>> ResolvingFileIO::FileIOForPath(
+ std::string_view location) {
+ const auto scheme =
StringUtils::ToLower(LocationUtil::ParseScheme(location));
+ ICEBERG_ASSIGN_OR_RAISE(const auto name, FileIORegistry::Resolve(scheme));
Review Comment:
First-colon scheme parsing will misinterpret Windows paths like
`C:\\dir\\file` as scheme `c`, causing `FileIORegistry::Resolve()` to fail and
routing local paths to break on Windows/MSVC builds. Consider treating
drive-letter paths as 'no scheme' (e.g., if `colon == 1` and the next char is
`\\` or `/`), and add a unit test case for that behavior.
##########
src/iceberg/arrow/s3/s3_properties.h:
##########
@@ -54,4 +56,26 @@ struct S3Properties {
static constexpr std::string_view kSocketTimeoutMs = "s3.socket-timeout-ms";
};
+/// \brief URI schemes served by the Arrow S3 FileIO, lower-case.
+///
+/// Single source of truth: both the registry registration and IsS3Scheme
derive
+/// from this list, so a new alias only has to be added here.
+inline constexpr std::array<std::string_view, 3> kS3Schemes = {"s3", "s3a",
"s3n"};
+
+/// \brief Return whether a normalized URI scheme is S3-compatible.
+inline constexpr bool IsS3Scheme(std::string_view scheme) {
+ return std::ranges::contains(kS3Schemes, scheme);
Review Comment:
`std::ranges::contains` is a C++23 addition; if this project is compiled as
C++20 (which is consistent with other current `std::ranges` usage), this will
fail to compile. Use a C++20-compatible check (e.g., `std::ranges::find(...) !=
end`) to keep the helper portable.
--
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]
