From 384909937d0c375ab84c09a87ec27b731fd2cee3 Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 27 Jun 2026 21:58:41 +0100 Subject: [PATCH] fix(rest): resolve a FileIO without io-impl or warehouse MakeCatalogFileIO and MakeTableFileIO previously returned InvalidArgument when neither "io-impl" nor "warehouse" was configured, preventing catalogs that rely entirely on vended storage credentials (no client-side IO config) from being created or loading tables. Now: - MakeCatalogFileIO falls back to a local FileIO so the catalog can still be created; per-table resolution supplies remote access where needed. - MakeTableFileIO resolves the implementation in priority order: io-impl, warehouse scheme, then the table's metadata-location scheme, falling back to a local FileIO. The metadata location is threaded through TableFileIO so the scheme comes from the table's actual location rather than a storage-credential prefix (which may be scheme-only, e.g. "s3"). Updates the catalog test for the new fallback and adds table-FileIO tests for metadata-location detection (with a scheme-only credential prefix) and the local fallback. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/iceberg/catalog/rest/rest_catalog.cc | 14 ++++--- src/iceberg/catalog/rest/rest_catalog.h | 1 + src/iceberg/catalog/rest/rest_file_io.cc | 31 +++++++++----- src/iceberg/catalog/rest/rest_file_io.h | 5 +++ src/iceberg/test/rest_file_io_test.cc | 53 ++++++++++++++++++++++-- 5 files changed, 85 insertions(+), 19 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 27de0befa..1eab534f2 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -488,9 +488,11 @@ Result> RestCatalog::TableAuthSession( Result> RestCatalog::TableFileIO( const SessionContext& /*context*/, const std::unordered_map& table_config, + std::string_view metadata_location, const std::vector& 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_; @@ -730,8 +732,9 @@ Result> 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))); @@ -846,8 +849,9 @@ Result> RestCatalog::MakeTableFromLoadResult( std::shared_ptr 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))); diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index 97cf42151..5c580f0d5 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -81,6 +81,7 @@ class ICEBERG_REST_EXPORT RestCatalog final Result> TableFileIO( const SessionContext& context, const std::unordered_map& table_config, + std::string_view metadata_location, const std::vector& storage_credentials) const; Result> ListNamespaces(const Namespace& ns, diff --git a/src/iceberg/catalog/rest/rest_file_io.cc b/src/iceberg/catalog/rest/rest_file_io.cc index fe8a2b155..8a29bc7c1 100644 --- a/src/iceberg/catalog/rest/rest_file_io.cc +++ b/src/iceberg/catalog/rest/rest_file_io.cc @@ -84,12 +84,14 @@ Result> 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)) { @@ -110,19 +112,26 @@ Result> MakeCatalogFileIO(const RestCatalogProperties& c Result> MakeTableFileIO( const std::unordered_map& catalog_config, const std::unordered_map& table_config, + std::string_view metadata_location, const std::vector& 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)); + 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)); diff --git a/src/iceberg/catalog/rest/rest_file_io.h b/src/iceberg/catalog/rest/rest_file_io.h index 9e5f7a0a9..96ae82684 100644 --- a/src/iceberg/catalog/rest/rest_file_io.h +++ b/src/iceberg/catalog/rest/rest_file_io.h @@ -48,9 +48,14 @@ ICEBERG_REST_EXPORT Result> 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> MakeTableFileIO( const std::unordered_map& catalog_config, const std::unordered_map& table_config, + std::string_view metadata_location, const std::vector& storage_credentials); } // namespace iceberg::rest diff --git a/src/iceberg/test/rest_file_io_test.cc b/src/iceberg/test/rest_file_io_test.cc index 7f41b0b46..af54dc261 100644 --- a/src/iceberg/test/rest_file_io_test.cc +++ b/src/iceberg/test/rest_file_io_test.cc @@ -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& /*properties*/) + -> Result> { return std::make_unique(); }); + + // 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) { @@ -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()); @@ -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, @@ -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& /*properties*/) + -> Result> { + return std::make_unique(); + }); + + // 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& /*properties*/) + -> Result> { return std::make_unique(); }); + + // 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