Skip to content

fix(rest): resolve a FileIO without io-impl or warehouse configured #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smaheshwar-pltr wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(rest): resolve a FileIO without io-impl or warehouse configured #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/iceberg/catalog/rest/rest_catalog.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,9 +488,11 @@ Result<std::shared_ptr<auth::AuthSession>> RestCatalog::TableAuthSession(
Result<std::shared_ptr<FileIO>> RestCatalog::TableFileIO(
const SessionContext& /*context*/,
const std::unordered_map<std::string, std::string>& table_config,
std::string_view metadata_location,

@smaheshwar-pltr smaheshwar-pltr Jun 27, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we even need this change for this PR? Doesn't the change of MakeCatalogFileIO not throwing solve our use case alone and it'd keep the PR smaller? Also, look for comments on the original upstream PR to see if there are any notes about our scenario at all

const std::vector<StorageCredential>& storage_credentials) const {
if (!table_config.empty() || !storage_credentials.empty()) {
return MakeTableFileIO(config_.configs(), table_config, storage_credentials);
return MakeTableFileIO(config_.configs(), table_config, metadata_location,
storage_credentials);
}

return file_io_;
Expand Down Expand Up @@ -730,8 +732,9 @@ Result<std::shared_ptr<Transaction>> RestCatalog::StageCreateTable(
/*stage_create=*/true, *contextual_session));
auto table_config = std::move(result.config);
auto storage_credentials = std::move(result.storage_credentials);
ICEBERG_ASSIGN_OR_RAISE(auto table_io,
TableFileIO(context, table_config, storage_credentials));
ICEBERG_ASSIGN_OR_RAISE(
auto table_io,
TableFileIO(context, table_config, result.metadata_location, storage_credentials));
ICEBERG_ASSIGN_OR_RAISE(
auto table_session,
TableAuthSession(identifier, table_config, std::move(contextual_session)));
Expand Down Expand Up @@ -846,8 +849,9 @@ Result<std::shared_ptr<Table>> RestCatalog::MakeTableFromLoadResult(
std::shared_ptr<auth::AuthSession> contextual_session) {
auto table_config = std::move(result.config);
auto storage_credentials = std::move(result.storage_credentials);
ICEBERG_ASSIGN_OR_RAISE(auto table_io,
TableFileIO(context, table_config, storage_credentials));
ICEBERG_ASSIGN_OR_RAISE(
auto table_io,
TableFileIO(context, table_config, result.metadata_location, storage_credentials));
ICEBERG_ASSIGN_OR_RAISE(
auto table_session,
TableAuthSession(identifier, table_config, std::move(contextual_session)));
Expand Down
1 change: 1 addition & 0 deletions src/iceberg/catalog/rest/rest_catalog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ class ICEBERG_REST_EXPORT RestCatalog final
Result<std::shared_ptr<FileIO>> TableFileIO(
const SessionContext& context,
const std::unordered_map<std::string, std::string>& table_config,
std::string_view metadata_location,
const std::vector<StorageCredential>& storage_credentials) const;

Result<std::vector<Namespace>> ListNamespaces(const Namespace& ns,
Expand Down
31 changes: 20 additions & 11 deletions src/iceberg/catalog/rest/rest_file_io.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,14 @@ Result<std::unique_ptr<FileIO>> MakeCatalogFileIO(const RestCatalogProperties& c

if (io_impl.empty()) {
if (warehouse.empty()) {
return InvalidArgument(R"("{}" or "{}" property is required to create FileIO)",
RestCatalogProperties::kIOImpl.key(),
RestCatalogProperties::kWarehouse.key());
// No io-impl or warehouse configured. Fall back to a local FileIO so the
// catalog can still be created; remote access for individual tables is
// resolved per table (e.g. from vended storage credentials).
io_impl = std::string(FileIORegistry::kArrowLocalFileIO);
} else {
ICEBERG_ASSIGN_OR_RAISE(const auto detected_kind, DetectBuiltinFileIO(warehouse));
io_impl = std::string(BuiltinFileIOName(detected_kind));
}
ICEBERG_ASSIGN_OR_RAISE(const auto detected_kind, DetectBuiltinFileIO(warehouse));
io_impl = std::string(BuiltinFileIOName(detected_kind));
}

if (!warehouse.empty() && IsBuiltinImpl(io_impl)) {
Expand All @@ -110,19 +112,26 @@ Result<std::unique_ptr<FileIO>> MakeCatalogFileIO(const RestCatalogProperties& c
Result<std::unique_ptr<FileIO>> MakeTableFileIO(
const std::unordered_map<std::string, std::string>& catalog_config,
const std::unordered_map<std::string, std::string>& table_config,
std::string_view metadata_location,
const std::vector<StorageCredential>& storage_credentials) {
const auto default_properties = MergeFileIOProperties(catalog_config, table_config);
const auto properties = RestCatalogProperties::FromMap(default_properties);
auto io_impl = properties.Get(RestCatalogProperties::kIOImpl);
if (io_impl.empty()) {
const auto warehouse = properties.Get(RestCatalogProperties::kWarehouse);
if (warehouse.empty()) {
return InvalidArgument(R"("{}" or "{}" property is required to create FileIO)",
RestCatalogProperties::kIOImpl.key(),
RestCatalogProperties::kWarehouse.key());
if (!warehouse.empty()) {
ICEBERG_ASSIGN_OR_RAISE(const auto detected_kind, DetectBuiltinFileIO(warehouse));
io_impl = std::string(BuiltinFileIOName(detected_kind));
} else if (!metadata_location.empty()) {
// Neither io-impl nor warehouse is configured. Infer the implementation
// from the table's metadata location so catalogs that rely solely on
// vended credentials can resolve a table FileIO.
ICEBERG_ASSIGN_OR_RAISE(const auto detected_kind,
DetectBuiltinFileIO(metadata_location));

@smaheshwar-pltr smaheshwar-pltr Jun 27, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving the table FileIO from the location scheme (rather than requiring a client warehouse/io-impl) mirrors Java and PyIceberg:

io_impl = std::string(BuiltinFileIOName(detected_kind));
} else {
io_impl = std::string(FileIORegistry::kArrowLocalFileIO);
}
ICEBERG_ASSIGN_OR_RAISE(const auto detected_kind, DetectBuiltinFileIO(warehouse));
io_impl = std::string(BuiltinFileIOName(detected_kind));
}
ICEBERG_ASSIGN_OR_RAISE(auto io, FileIORegistry::Load(io_impl, default_properties));

Expand Down
5 changes: 5 additions & 0 deletions src/iceberg/catalog/rest/rest_file_io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,14 @@ ICEBERG_REST_EXPORT Result<std::unique_ptr<FileIO>> MakeCatalogFileIO(
const RestCatalogProperties& config);

/// \brief Build the configured table FileIO and apply storage credentials if present.
///
/// The implementation is resolved in priority order from the merged catalog and
/// table config "io-impl", the "warehouse" scheme, then the \p metadata_location
/// scheme, falling back to a local FileIO when none of these is available.
ICEBERG_REST_EXPORT Result<std::unique_ptr<FileIO>> MakeTableFileIO(
const std::unordered_map<std::string, std::string>& catalog_config,
const std::unordered_map<std::string, std::string>& table_config,
std::string_view metadata_location,
const std::vector<StorageCredential>& storage_credentials);

} // namespace iceberg::rest
53 changes: 50 additions & 3 deletions src/iceberg/test/rest_file_io_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,16 @@ TEST(RestFileIOTest, DetectBuiltinKindRejectsUnsupportedScheme) {
EXPECT_THAT(result, HasErrorMessage("not supported for automatic FileIO resolution"));
}

TEST(RestFileIOTest, MakeCatalogFileIOMissingImplAndWarehouse) {
TEST(RestFileIOTest, MakeCatalogFileIOFallsBackToLocalWhenUnconfigured) {
FileIORegistry::Register(
std::string(FileIORegistry::kArrowLocalFileIO),
[](const std::unordered_map<std::string, std::string>& /*properties*/)
-> Result<std::unique_ptr<FileIO>> { return std::make_unique<MockFileIO>(); });

// With neither io-impl nor warehouse configured, the catalog still gets a
// (local) FileIO rather than failing, so per-table credential vending works.
auto result = MakeCatalogFileIO(RestCatalogProperties::default_properties());
EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument));
ASSERT_THAT(result, IsOk());
}

TEST(RestFileIOTest, MakeCatalogFileIORejectsIncompatibleWarehouse) {
Expand Down Expand Up @@ -187,6 +194,7 @@ TEST(RestFileIOTest, TableFileIOMergesConfigAndCredentials) {
{"catalog-only", "catalog"},
{"shared", "catalog"}},
{{"io-impl", custom_impl}, {"table-only", "table"}, {"shared", "table"}},
/*metadata_location=*/"s3://bucket/table/metadata/v1.metadata.json",
{{.prefix = "s3://bucket/table",
.config = {{"shared", "credential"}, {"credential-only", "value"}}}});
ASSERT_THAT(result, IsOk());
Expand Down Expand Up @@ -221,7 +229,7 @@ TEST(RestFileIOTest, TableImplOverridesWarehouseScheme) {
auto result =
MakeTableFileIO({{"warehouse", "/tmp/catalog-warehouse"}},
{{"io-impl", std::string(FileIORegistry::kArrowS3FileIO)}},
/*storage_credentials=*/{});
/*metadata_location=*/"", /*storage_credentials=*/{});
ASSERT_THAT(result, IsOk());
EXPECT_THAT(
captured_file_io_properties,
Expand All @@ -239,9 +247,48 @@ TEST(RestFileIOTest, TableFileIORejectsCredentials) {

auto result = MakeTableFileIO(
{{"warehouse", "s3://catalog/warehouse"}}, {{"io-impl", custom_impl}},
/*metadata_location=*/"s3://bucket/table",
{{.prefix = "s3://bucket/table", .config = {{"k", "v"}}}});
EXPECT_THAT(result, IsError(ErrorKind::kNotSupported));
EXPECT_THAT(result, HasErrorMessage("does not support vended storage credentials"));
}

TEST(RestFileIOTest, TableFileIODetectsImplFromMetadataLocation) {
captured_storage_credentials.clear();
FileIORegistry::Register(
std::string(FileIORegistry::kArrowS3FileIO),
[](const std::unordered_map<std::string, std::string>& /*properties*/)
-> Result<std::unique_ptr<FileIO>> {
return std::make_unique<MockCredentialedFileIO>();
});

// No io-impl or warehouse anywhere; the implementation is inferred from the
// table's metadata location scheme. The credential carries a scheme-only "s3"
// prefix (which is not itself a resolvable URI), so detection must not rely on
// it; the vended credentials are still applied.
auto result = MakeTableFileIO(
/*catalog_config=*/{}, /*table_config=*/{},
/*metadata_location=*/"s3://bucket/table/metadata/v1.metadata.json",
{{.prefix = "s3", .config = {{"k", "v"}}}});
ASSERT_THAT(result, IsOk());
ASSERT_NE(result.value()->AsSupportsStorageCredentials(), nullptr);
ASSERT_EQ(captured_storage_credentials.size(), 1);
EXPECT_EQ(captured_storage_credentials[0].prefix, "s3");
}

TEST(RestFileIOTest, TableFileIOFallsBackToLocalWhenUnconfigured) {
FileIORegistry::Register(
std::string(FileIORegistry::kArrowLocalFileIO),
[](const std::unordered_map<std::string, std::string>& /*properties*/)
-> Result<std::unique_ptr<FileIO>> { return std::make_unique<MockFileIO>(); });

// table_config exercises MakeTableFileIO, but there is no io-impl, warehouse,
// or metadata location to infer from, so it defaults to a local FileIO.
auto result = MakeTableFileIO(/*catalog_config=*/{},
/*table_config=*/{{"some-key", "value"}},
/*metadata_location=*/"",
/*storage_credentials=*/{});
ASSERT_THAT(result, IsOk());
}

} // namespace iceberg::rest
Loading