refactor(io): make FileIO resolution registry-driven - #889
Open
wgtmac wants to merge 1 commit into
Open
Conversation
Add scheme-aware FileIO factories with deterministic registration precedence, route ResolvingFileIO by location scheme, and forward vended storage credentials through registered delegates.
There was a problem hiding this comment.
Pull request overview
Refactors FileIO resolution so ResolvingFileIO routes locations via FileIORegistry factories (registry-driven scheme ownership), improves delegate caching across credential refreshes, narrows S3 routing to s3/s3a/s3n, and adds/updates targeted coverage plus end-user documentation.
Changes:
- Introduces
FileIORegistry::Factory{create, accepts}and scheme-basedFileIORegistry::Resolve()with “latest registration wins” semantics. - Updates
ResolvingFileIOto resolve schemes via the registry (Java-style first-colon parsing) and to cache delegates usingshared_ptrwith refreshed-credential rebuild behavior. - Updates tests, build files, and docs to reflect the new registry-driven routing model and supported S3 schemes.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/iceberg/util/location_util.h | Adds LocationUtil::ParseScheme declaration used for scheme routing. |
| src/iceberg/util/location_util.cc | Implements ParseScheme for Java-style first-colon scheme parsing. |
| src/iceberg/file_io_registry.h | Refactors registry API to struct-based factories and adds Resolve(scheme). |
| src/iceberg/file_io_registry.cc | Implements ordered registrations, explicit load, and scheme resolution. |
| src/iceberg/resolving_file_io.h | Switches caching to shared_ptr and updates locking to shared_mutex. |
| src/iceberg/resolving_file_io.cc | Routes per-location via FileIORegistry::Resolve, caches delegates, rebuilds on credential refresh, and groups bulk deletes by delegate. |
| src/iceberg/catalog/rest/rest_file_io.cc | Defaults REST catalog FileIO to directly constructed ResolvingFileIO when io-impl is absent. |
| src/iceberg/arrow/arrow_register.cc | Registers built-in local/S3 FileIOs with create + accepts callbacks. |
| src/iceberg/arrow/s3/s3_properties.h | Centralizes S3 scheme list and helpers for scheme acceptance. |
| src/iceberg/arrow/s3/arrow_s3_file_io.cc | Removes OSS alias handling from S3 credential prefix logic and scheme canonicalization. |
| src/iceberg/util/location_util.cc | (Also) relocates scheme parsing logic into a shared util. |
| src/iceberg/test/rest_file_io_test.cc | Adjusts default REST FileIO expectation; adds a registry-delegation test; updates registry registration callsites. |
| src/iceberg/test/rest_catalog_integration_test.cc | Updates registry registration to new factory struct form. |
| src/iceberg/test/resolving_file_io_test.cc | Updates routing assumptions (no OSS alias), adds batch delete grouping tests, and validates no fallback after selected factory failure. |
| src/iceberg/test/location_util_test.cc | Adds unit test coverage for ParseScheme. |
| src/iceberg/test/arrow_s3_file_io_test.cc | Removes OSS from S3-compatible credential prefixes; updates endpoint scheme test data away from OSS-specific values. |
| src/iceberg/test/arrow_io_test.cc | Adds a registration smoke test ensuring built-in registry routing works via ResolvingFileIO. |
| src/iceberg/test/rest_arrow_file_io_test.cc | Removes an integration test that depended on the prior OSS/S3 routing behavior and bundle linkage. |
| src/iceberg/test/CMakeLists.txt | Removes bundle-only REST Arrow FileIO test wiring and USE_BUNDLE option. |
| src/iceberg/CMakeLists.txt | Adds util/location_util.cc to the CMake build. |
| src/iceberg/meson.build | Adds util/location_util.cc to the Meson build. |
| mkdocs/mkdocs.yml | Adds FileIO docs page to the documentation nav. |
| mkdocs/docs/file-io.md | Documents built-in/custom FileIO registration and selection, plus credential forwarding behavior. |
Suppressed comments (1)
src/iceberg/util/location_util.cc:28
ParseSchemecurrently treats any text before the first ':' as a scheme, even for local paths that may contain ':' (e.g. WindowsC:\\...or a POSIX path segment with ':'). That makesResolvingFileIOattempt registry resolution for what should be a local path and can yield kNotSupported.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+74
to
78
| std::erase_if(state.registrations, [&name](const RegistryState::Entry& entry) { | ||
| return entry.name == name; | ||
| }); | ||
| state.registrations.emplace_back(std::move(name), std::move(factory)); | ||
| } |
Comment on lines
+25
to
28
| #include <algorithm> | ||
| #include <array> | ||
| #include <string_view> | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The existing FileIO design embeds scheme ownership in
ResolvingFileIOandregisters 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 notsafe without provider-specific endpoint and compatibility validation.
Changes
FileIORegistry::Factorycontain a requiredcreatecallback and anoptional
acceptscallback.registrations overriding earlier ones.
ResolvingFileIOdirectly while preserving explicitio-implprecedence.mapping from the resolver.
shared_ptracross credential refreshes.delegates after refresh.
s3,s3a, ands3n; defer OSS/COS support to a separate change.