You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2026/08/16 15:59:20 UTC
[PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
wgtmac opened a new pull request, #889:
URL: https://github.com/apache/iceberg-cpp/pull/889
## Problem
The existing FileIO design embeds scheme ownership in `ResolvingFileIO` and
registers the resolver itself as a special implementation. This duplicates
backend-specific scheme knowledge, prevents custom FileIOs from declaring the
schemes they support, and couples automatic routing to a special registry
entry.
Cached delegates are also returned as raw pointers while credential refresh
can invalidate the cache. In addition, treating `oss://` as an S3 alias is not
safe without provider-specific endpoint and compatibility validation.
## Changes
- Make `FileIORegistry::Factory` contain a required `create` callback and an
optional `accepts` callback.
- Resolve normalized schemes from registered factories, with later
registrations overriding earlier ones.
- Construct the default `ResolvingFileIO` directly while preserving explicit
`io-impl` precedence.
- Use Java-compatible first-colon scheme parsing and remove hard-coded scheme
mapping from the resolver.
- Keep cached delegates alive with `shared_ptr` across credential refreshes.
- Forward complete credential lists to delegates that support them and rebuild
delegates after refresh.
- Limit Arrow S3 routing and credential prefix handling to `s3`, `s3a`, and
`s3n`; defer OSS/COS support to a separate change.
- Add focused registry, resolver, REST, and S3 coverage.
- Document built-in and custom FileIO usage.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3792173719
##########
src/iceberg/arrow/s3/s3_properties.h:
##########
@@ -22,6 +22,8 @@
/// \file iceberg/arrow/s3/s3_properties.h
/// \brief Define S3 configuration property keys.
+#include <algorithm>
+#include <array>
#include <string_view>
Review Comment:
`IsS3Scheme` uses `std::ranges::contains`, but this header doesn't include `<ranges>`, which can break builds depending on transitive includes.
##########
src/iceberg/file_io_registry.cc:
##########
@@ -19,56 +19,98 @@
#include "iceberg/file_io_registry.h"
+#include <algorithm>
#include <mutex>
+#include <ranges>
+#include <string>
#include <utility>
+#include <vector>
-#include "iceberg/resolving_file_io.h"
+#include "iceberg/util/macros.h"
+#include "iceberg/util/string_util.h"
namespace iceberg {
namespace {
struct RegistryState {
- std::mutex mutex;
- std::unordered_map<std::string, FileIORegistry::Factory> registry;
+ struct Entry {
+ std::string name;
+ FileIORegistry::Factory factory;
+ };
- RegistryState() {
- // Always available: the scheme-resolving FileIO lives in the core library.
- registry[std::string(FileIORegistry::kResolvingFileIO)] =
- [](const std::unordered_map<std::string, std::string>& properties)
- -> Result<std::unique_ptr<FileIO>> {
- return std::make_unique<ResolvingFileIO>(properties);
- };
- }
+ std::mutex mutex;
+ std::vector<Entry> registrations;
};
RegistryState& State() {
static RegistryState state;
return state;
}
+// Copy entries so user callbacks run outside the registry lock.
+std::vector<RegistryState::Entry> SnapshotEntries() {
+ auto& state = State();
+ std::lock_guard lock(state.mutex);
+ return state.registrations;
+}
+
+std::string FormatNames(const std::vector<RegistryState::Entry>& entries) {
+ std::string result;
+ for (const auto& entry : entries) {
+ if (!result.empty()) {
+ result += ", ";
+ }
+ result += entry.name;
+ }
+ return result.empty() ? "(none registered)" : result;
+}
+
} // namespace
-void FileIORegistry::Register(const std::string& name, Factory factory) {
+void FileIORegistry::Register(std::string name, Factory factory) {
auto& state = State();
std::lock_guard lock(state.mutex);
- state.registry[name] = std::move(factory);
+ std::erase_if(state.registrations, [&name](const RegistryState::Entry& entry) {
+ return entry.name == name;
+ });
+ state.registrations.emplace_back(std::move(name), std::move(factory));
}
Review Comment:
`RegistryState::Entry` is an aggregate (no constructor), so `registrations.emplace_back(std::move(name), std::move(factory))` will not compile. Use aggregate initialization (braces) or add an explicit `Entry` constructor.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#issuecomment-5312594504
> I've made some refactoring to the FileIO resolution. Let me know what you think. @plusplusjiajia
@wgtmac Thanks for looping me in — registry-driven resolution is right, and `shared_ptr` delegates close a real use-after-free window.
credential prefixes — agree.** `s3`/`s3a`/`s3n` matches Java's `S3FileIO`.
routing — I'd keep `oss`.** This is stricter than Java: `S3URI` applies no scheme whitelist, and its javadoc says it "supports any valid URI schemes ... allows users to use S3FileIO with other S3-compatible object storage services". PyIceberg routes `oss` the same way ([map](https://github.com/apache/iceberg-python/blob/03ad0d6c/pyiceberg/io/__init__.py#L322), [init](https://github.com/apache/iceberg-python/blob/03ad0d6c/pyiceberg/io/pyarrow.py#L450)). `ArrowS3FileIO` is equally capable — `ResolvePath` already handles foreign schemes — so `kS3Schemes` is a new gate, and an unmatched scheme is a hard `NotSupported`, which makes such stores unusable.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#issuecomment-5311275698
I've made some refactoring to the FileIO resolution. Let me know what you think. @plusplusjiajia
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "MisterRaindrop (via GitHub)" <gi...@apache.org>.
MisterRaindrop commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3794263203
##########
src/iceberg/arrow/s3/arrow_s3_file_io.cc:
##########
@@ -91,11 +91,10 @@ std::string SplitEndpointScheme(std::string_view endpoint,
return std::string(endpoint);
}
-// Location prefixes this FileIO can serve: must cover every scheme
-// ResolveFileIOName routes here, or such a credential would be dropped.
+// Location prefixes this FileIO can serve.
bool IsS3FileIOCredentialPrefix(std::string_view prefix) {
return prefix == "s3" || prefix.starts_with("s3://") || prefix.starts_with("s3a://") ||
- prefix.starts_with("s3n://") || prefix.starts_with("oss://");
+ prefix.starts_with("s3n://");
Review Comment:
We’d be better off using a macro for this？
##########
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:
ResolvingFileIO does not have a bound stable registry snapshot; therefore, the parsing results returned by FileIO may be affected by the process-wide mutable registry ?
example:
Step 1: A(s3) Registration
Step 2: Resolver resolves s3 -> A
Time 3: A is replaced by a record with the same name.
Result: Resolver still hit an old cache entry named "A".
I just feel maybe there is this kind of problem.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3811125628
##########
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:
I think the contract is that FileIO registration should be done before anything. A created ResolvingFileIO should just use its cached instances to serve followup calls.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#issuecomment-5312082596
cc @MisterRaindrop
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "MisterRaindrop (via GitHub)" <gi...@apache.org>.
MisterRaindrop commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3794340505
##########
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:
ResolvingFileIO does not have a bound stable registry snapshot therefore, the parsing results returned by FileIO may be affected by the process-wide mutable registry ?
example:
Step 1: A(s3) Registration
Step 2: Resolver resolves s3 -> A
Time 3: A is replaced by a record with the same name.
Result: Resolver still hit an old cache entry named "A".
I just feel maybe there is this kind of problem.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3794216189
##########
src/iceberg/util/location_util.cc:
##########
@@ -17,20 +17,16 @@
* under the License.
*/
-#pragma once
-
-/// \file iceberg/resolving_file_io_internal.h
-/// \brief Internal helpers for ResolvingFileIO. Not part of the public API.
-
-#include <string_view>
-
-#include "iceberg/iceberg_export.h"
-#include "iceberg/result.h"
+#include "iceberg/util/location_util.h"
namespace iceberg {
-/// \brief The FileIORegistry name of the implementation serving `location`,
-/// chosen by its URI scheme. Exported so tests can link it in shared builds.
-ICEBERG_EXPORT Result<std::string_view> ResolveFileIOName(std::string_view location);
+std::string_view LocationUtil::ParseScheme(std::string_view location) {
+ const auto colon = location.find(':');
Review Comment:
Fair enough.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#issuecomment-5338604024
@plusplusjiajia I‘d prefer postponing the support of `oss` scheme to a separate PR. Correctly we have no evidence that it works. Perhaps we can provide a cli for FileIO to verify its connection.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#issuecomment-5340567924
Thanks everyone!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3794188218
##########
src/iceberg/util/location_util.cc:
##########
@@ -17,20 +17,16 @@
* under the License.
*/
-#pragma once
-
-/// \file iceberg/resolving_file_io_internal.h
-/// \brief Internal helpers for ResolvingFileIO. Not part of the public API.
-
-#include <string_view>
-
-#include "iceberg/iceberg_export.h"
-#include "iceberg/result.h"
+#include "iceberg/util/location_util.h"
namespace iceberg {
-/// \brief The FileIORegistry name of the implementation serving `location`,
-/// chosen by its URI scheme. Exported so tests can link it in shared builds.
-ICEBERG_EXPORT Result<std::string_view> ResolveFileIOName(std::string_view location);
+std::string_view LocationUtil::ParseScheme(std::string_view location) {
+ const auto colon = location.find(':');
Review Comment:
Indeed I have tried this which ended up with messy code here. I deliberately align this logic with Java with the assumption that local file is not supposed to be used in production so we should not bother with them just for testing purposes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3793435071
##########
src/iceberg/arrow/s3/s3_properties.h:
##########
@@ -22,6 +22,8 @@
/// \file iceberg/arrow/s3/s3_properties.h
/// \brief Define S3 configuration property keys.
+#include <algorithm>
+#include <array>
#include <string_view>
Review Comment:
std::ranges::contains is declared in `<algorithm>`, which this header already includes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3810652624
##########
src/iceberg/arrow/s3/arrow_s3_file_io.cc:
##########
@@ -91,11 +91,10 @@ std::string SplitEndpointScheme(std::string_view endpoint,
return std::string(endpoint);
}
-// Location prefixes this FileIO can serve: must cover every scheme
-// ResolveFileIOName routes here, or such a credential would be dropped.
+// Location prefixes this FileIO can serve.
bool IsS3FileIOCredentialPrefix(std::string_view prefix) {
return prefix == "s3" || prefix.starts_with("s3://") || prefix.starts_with("s3a://") ||
- prefix.starts_with("s3n://") || prefix.starts_with("oss://");
+ prefix.starts_with("s3n://");
Review Comment:
I'm not sure if I understand this. Could you give an example?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "MisterRaindrop (via GitHub)" <gi...@apache.org>.
MisterRaindrop commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3810802352
##########
src/iceberg/arrow/s3/arrow_s3_file_io.cc:
##########
@@ -91,11 +91,10 @@ std::string SplitEndpointScheme(std::string_view endpoint,
return std::string(endpoint);
}
-// Location prefixes this FileIO can serve: must cover every scheme
-// ResolveFileIOName routes here, or such a credential would be dropped.
+// Location prefixes this FileIO can serve.
bool IsS3FileIOCredentialPrefix(std::string_view prefix) {
return prefix == "s3" || prefix.starts_with("s3://") || prefix.starts_with("s3a://") ||
- prefix.starts_with("s3n://") || prefix.starts_with("oss://");
+ prefix.starts_with("s3n://");
Review Comment:
"macro" was not the right term. I feel moving this logic to kS3Schemes in s3_properties.h looks better
like this
```
inline bool IsS3CredentialPrefix(std::string_view prefix) {
if (prefix == S3Properties::kS3Schema) {
return true;
}
const auto delimiter = prefix.find("://");
return delimiter != std::string_view::npos &&
IsS3Scheme(prefix.substr(0, delimiter));
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3793435071
##########
src/iceberg/arrow/s3/s3_properties.h:
##########
@@ -22,6 +22,8 @@
/// \file iceberg/arrow/s3/s3_properties.h
/// \brief Define S3 configuration property keys.
+#include <algorithm>
+#include <array>
#include <string_view>
Review Comment:
std::ranges::contains is declared in <algorithm>, which this header already includes.
##########
src/iceberg/file_io_registry.cc:
##########
@@ -19,56 +19,98 @@
#include "iceberg/file_io_registry.h"
+#include <algorithm>
#include <mutex>
+#include <ranges>
+#include <string>
#include <utility>
+#include <vector>
-#include "iceberg/resolving_file_io.h"
+#include "iceberg/util/macros.h"
+#include "iceberg/util/string_util.h"
namespace iceberg {
namespace {
struct RegistryState {
- std::mutex mutex;
- std::unordered_map<std::string, FileIORegistry::Factory> registry;
+ struct Entry {
+ std::string name;
+ FileIORegistry::Factory factory;
+ };
- RegistryState() {
- // Always available: the scheme-resolving FileIO lives in the core library.
- registry[std::string(FileIORegistry::kResolvingFileIO)] =
- [](const std::unordered_map<std::string, std::string>& properties)
- -> Result<std::unique_ptr<FileIO>> {
- return std::make_unique<ResolvingFileIO>(properties);
- };
- }
+ std::mutex mutex;
+ std::vector<Entry> registrations;
};
RegistryState& State() {
static RegistryState state;
return state;
}
+// Copy entries so user callbacks run outside the registry lock.
+std::vector<RegistryState::Entry> SnapshotEntries() {
+ auto& state = State();
+ std::lock_guard lock(state.mutex);
+ return state.registrations;
+}
+
+std::string FormatNames(const std::vector<RegistryState::Entry>& entries) {
+ std::string result;
+ for (const auto& entry : entries) {
+ if (!result.empty()) {
+ result += ", ";
+ }
+ result += entry.name;
+ }
+ return result.empty() ? "(none registered)" : result;
+}
+
} // namespace
-void FileIORegistry::Register(const std::string& name, Factory factory) {
+void FileIORegistry::Register(std::string name, Factory factory) {
auto& state = State();
std::lock_guard lock(state.mutex);
- state.registry[name] = std::move(factory);
+ std::erase_if(state.registrations, [&name](const RegistryState::Entry& entry) {
+ return entry.name == name;
+ });
+ state.registrations.emplace_back(std::move(name), std::move(factory));
}
Review Comment:
This is valid in C++23; parenthesized aggregate initialization has been supported since C++20, and all CI compilers build it successfully.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3794145120
##########
src/iceberg/util/location_util.cc:
##########
@@ -17,20 +17,16 @@
* under the License.
*/
-#pragma once
-
-/// \file iceberg/resolving_file_io_internal.h
-/// \brief Internal helpers for ResolvingFileIO. Not part of the public API.
-
-#include <string_view>
-
-#include "iceberg/iceberg_export.h"
-#include "iceberg/result.h"
+#include "iceberg/util/location_util.h"
namespace iceberg {
-/// \brief The FileIORegistry name of the implementation serving `location`,
-/// chosen by its URI scheme. Exported so tests can link it in shared builds.
-ICEBERG_EXPORT Result<std::string_view> ResolveFileIOName(std::string_view location);
+std::string_view LocationUtil::ParseScheme(std::string_view location) {
+ const auto colon = location.find(':');
Review Comment:
Should we find `://` here? Since Windows local path can contain `:`, not sure if we should care much about Windows platform though :-)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#discussion_r3811433195
##########
src/iceberg/arrow/s3/arrow_s3_file_io.cc:
##########
@@ -91,11 +91,10 @@ std::string SplitEndpointScheme(std::string_view endpoint,
return std::string(endpoint);
}
-// Location prefixes this FileIO can serve: must cover every scheme
-// ResolveFileIOName routes here, or such a credential would be dropped.
+// Location prefixes this FileIO can serve.
bool IsS3FileIOCredentialPrefix(std::string_view prefix) {
return prefix == "s3" || prefix.starts_with("s3://") || prefix.starts_with("s3a://") ||
- prefix.starts_with("s3n://") || prefix.starts_with("oss://");
+ prefix.starts_with("s3n://");
Review Comment:
Addressed. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889#issuecomment-5340546007
> @plusplusjiajia I‘d prefer postponing the support of `oss` scheme to a separate PR. Correctly we have no evidence that it works. Perhaps we can provide a cli for FileIO to verify its connection.
Agreed — no objection to this going in as is. I'll bring `oss` back in #893 with the test and a `fileio.md`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(io): make FileIO resolution registry-driven [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #889:
URL: https://github.com/apache/iceberg-cpp/pull/889
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org