From 041dac87d075a897861a9c47440859f98c855d00 Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 05:44:12 +0100 Subject: [PATCH 1/9] Bump iceberg-cpp to upstream main Pin a new fork branch, duckdb-proto-iceberg: upstream main plus the two open upstream pull requests the extension needs (FileIO properties and the bundled S3 link dependencies) and a missing include that breaks MinGW builds. Upstream now covers the fork's other changes, credential vending and the public SnapshotUtil header. Adapt to upstream's API: RestCatalog is used through its Catalog view, and the S3 region property is client.region. Co-Authored-By: Claude Opus 5.5 (1M context) --- .gitmodules | 2 +- src/include/proto_iceberg_catalog.hpp | 10 +++++----- src/proto_iceberg_catalog.cpp | 3 +-- src/proto_iceberg_catalog_attach.cpp | 7 +++++-- src/s3_conversion.cpp | 2 +- test/test_s3_conversion.cpp | 6 +++--- third_party/iceberg-cpp | 2 +- 7 files changed, 17 insertions(+), 15 deletions(-) diff --git a/.gitmodules b/.gitmodules index e70425f..fbc55fe 100644 --- a/.gitmodules +++ b/.gitmodules @@ -10,4 +10,4 @@ [submodule "third_party/iceberg-cpp"] path = third_party/iceberg-cpp url = https://github.com/smaheshwar-pltr/iceberg-cpp.git - branch = duckdb-iceberg + branch = duckdb-proto-iceberg diff --git a/src/include/proto_iceberg_catalog.hpp b/src/include/proto_iceberg_catalog.hpp index f352eac..8cd4988 100644 --- a/src/include/proto_iceberg_catalog.hpp +++ b/src/include/proto_iceberg_catalog.hpp @@ -7,7 +7,7 @@ #include "duckdb/parser/parsed_data/attach_info.hpp" #include "duckdb/storage/storage_extension.hpp" -#include "iceberg/catalog/rest/rest_catalog.h" +#include "iceberg/catalog.h" namespace duckdb { @@ -15,8 +15,8 @@ class ProtoIcebergSchemaEntry; class ProtoIcebergCatalog : public Catalog { public: - ProtoIcebergCatalog(AttachedDatabase &db_p, string catalog_uri, - std::shared_ptr rest_catalog, string default_schema); + ProtoIcebergCatalog(AttachedDatabase &db_p, string catalog_uri, std::shared_ptr rest_catalog, + string default_schema); static unique_ptr Attach(optional_ptr storage_info, ClientContext &context, AttachedDatabase &db, const string &name, AttachInfo &info, @@ -32,7 +32,7 @@ class ProtoIcebergCatalog : public Catalog { /// Acquires exclusive use of the REST catalog. Every call into it, including through a loaded iceberg::Table (e.g. /// Table::Refresh()), must hold the returned guard, and only for the duration of that call. - Mutex>::Guard LockRestCatalog() { + Mutex>::Guard LockRestCatalog() { return rest_catalog_.Lock(); } @@ -93,7 +93,7 @@ class ProtoIcebergCatalog : public Catalog { string default_schema_; // TODO: Drop the lock once iceberg-cpp documents RestCatalog as thread-safe. It currently isn't: its HttpClient // shares one libcurl connection cache across all requests. - Mutex> rest_catalog_; + Mutex> rest_catalog_; }; } // namespace duckdb diff --git a/src/proto_iceberg_catalog.cpp b/src/proto_iceberg_catalog.cpp index 2a4dbe1..1a6b630 100644 --- a/src/proto_iceberg_catalog.cpp +++ b/src/proto_iceberg_catalog.cpp @@ -11,8 +11,7 @@ namespace duckdb { ProtoIcebergCatalog::ProtoIcebergCatalog(AttachedDatabase &db_p, string catalog_uri, - std::shared_ptr rest_catalog, - string default_schema) + std::shared_ptr rest_catalog, string default_schema) : Catalog(db_p), catalog_uri_(std::move(catalog_uri)), default_schema_(std::move(default_schema)), rest_catalog_(std::in_place, std::move(rest_catalog)) { } diff --git a/src/proto_iceberg_catalog_attach.cpp b/src/proto_iceberg_catalog_attach.cpp index c9d9aa7..1d602d1 100644 --- a/src/proto_iceberg_catalog_attach.cpp +++ b/src/proto_iceberg_catalog_attach.cpp @@ -9,6 +9,7 @@ #include "duckdb/main/secret/secret.hpp" #include "iceberg/catalog/rest/catalog_properties.h" +#include "iceberg/catalog/rest/rest_catalog.h" #include @@ -136,8 +137,10 @@ unique_ptr ProtoIcebergCatalog::Attach(optional_ptrAsCatalog(), "Failed to create Iceberg REST catalog at %s", params.uri); auto catalog = make_uniq(db, std::move(params.uri), std::move(rest_catalog), std::move(params.default_schema)); diff --git a/src/s3_conversion.cpp b/src/s3_conversion.cpp index c9dab18..751431c 100644 --- a/src/s3_conversion.cpp +++ b/src/s3_conversion.cpp @@ -31,7 +31,7 @@ constexpr std::array kPlainPropertyMappings = {{ {"key_id", S3Properties::kAccessKeyId}, {"secret", S3Properties::kSecretAccessKey}, {"session_token", S3Properties::kSessionToken}, - {"region", S3Properties::kRegion}, + {"region", S3Properties::kClientRegion}, }}; /// Parses an iceberg-cpp boolean property, which is exactly "true" or "false". diff --git a/test/test_s3_conversion.cpp b/test/test_s3_conversion.cpp index 97b2ab2..eef4f58 100644 --- a/test/test_s3_conversion.cpp +++ b/test/test_s3_conversion.cpp @@ -48,7 +48,7 @@ TEST_CASE("secret to properties: plain keys pass through", "[s3_conversion]") { {"secret", Value("shh")}, {"session_token", Value("tok")}, {"region", Value("eu-west-1")}})) == - "s3.access-key-id=AKIA, s3.region=eu-west-1, s3.secret-access-key=shh, s3.session-token=tok"); + "client.region=eu-west-1, s3.access-key-id=AKIA, s3.secret-access-key=shh, s3.session-token=tok"); } TEST_CASE("secret to properties: empty, null and unrelated keys are omitted", "[s3_conversion]") { @@ -89,7 +89,7 @@ TEST_CASE("properties to secret: plain keys pass through", "[s3_conversion]") { REQUIRE(Render(ConvertIcebergPropertiesToS3Secret({{"s3.access-key-id", "AKIA"}, {"s3.secret-access-key", "shh"}, {"s3.session-token", "tok"}, - {"s3.region", "eu-west-1"}})) == + {"client.region", "eu-west-1"}})) == "key_id='AKIA', region='eu-west-1', secret='shh', session_token='tok'"); } @@ -97,7 +97,7 @@ TEST_CASE("properties to secret: empty and unrelated properties are omitted", "[ REQUIRE(Render(ConvertIcebergPropertiesToS3Secret({{"s3.access-key-id", ""}, {"s3.endpoint", ""}, {"s3.connect-timeout-ms", "1000"}, - {"client.region", "us-east-1"}})) == ""); + {"s3.region", "us-east-1"}})) == ""); } TEST_CASE("properties to secret: non-boolean path-style access and SSL enabled are ignored", "[s3_conversion]") { diff --git a/third_party/iceberg-cpp b/third_party/iceberg-cpp index 5cbe377..d307d0c 160000 --- a/third_party/iceberg-cpp +++ b/third_party/iceberg-cpp @@ -1 +1 @@ -Subproject commit 5cbe377ba26e046c7970abae386fb310be2f8c56 +Subproject commit d307d0ced0bf0376da7ca6e1658cf1f3a0308e7c From 2598ee42e7bd130f48b82fbe155037468bd48695 Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 05:44:19 +0100 Subject: [PATCH 2/9] Apply vended storage credentials to scoped S3 secrets REST catalogs can vend credentials as storage credentials scoped to location prefixes rather than in the table config. The old fork merged the best match into the FileIO properties; upstream keeps them separate. Overlay the credential with the longest prefix matching the table location, which resolves the TODO for the storage credentials field. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/include/s3_conversion.hpp | 11 +++++++++++ src/proto_iceberg_table_entry.cpp | 8 ++++++-- src/s3_conversion.cpp | 17 +++++++++++++++++ test/test_s3_conversion.cpp | 25 +++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/include/s3_conversion.hpp b/src/include/s3_conversion.hpp index 78b18b4..afee14d 100644 --- a/src/include/s3_conversion.hpp +++ b/src/include/s3_conversion.hpp @@ -4,8 +4,11 @@ #include "duckdb/common/case_insensitive_map.hpp" #include "duckdb/common/types/value.hpp" +#include "iceberg/storage_credential.h" +#include #include +#include #include namespace duckdb::conversion { @@ -21,4 +24,12 @@ ConvertS3SecretToIcebergProperties(const case_insensitive_tree_t &secret) [[nodiscard]] case_insensitive_map_t ConvertIcebergPropertiesToS3Secret(const std::unordered_map &properties); +/// Overlays the vended storage credential whose prefix is the longest match for location onto FileIO properties. +/// +/// REST catalogs vend credentials either in the table config, which iceberg-cpp merges into the FileIO properties, +/// or as storage credentials scoped to location prefixes, which it keeps separately. +[[nodiscard]] std::unordered_map +MergeStorageCredential(std::unordered_map properties, + std::span credentials, std::string_view location); + } // namespace duckdb::conversion diff --git a/src/proto_iceberg_table_entry.cpp b/src/proto_iceberg_table_entry.cpp index 0979a4b..7cd24d0 100644 --- a/src/proto_iceberg_table_entry.cpp +++ b/src/proto_iceberg_table_entry.cpp @@ -74,8 +74,12 @@ CreateSecretInput BuildScopedS3Secret(const string &catalog_name, const iceberg: input.scope.push_back(scope_with_slash(std::move(write_data_path))); } - // TODO: Respect storage credentials REST field, not just the credentials in IO properties - input.options = conversion::ConvertIcebergPropertiesToS3Secret(io->properties()); + std::span credentials; + if (const auto *credentialed = io->AsSupportsStorageCredentials()) { + credentials = credentialed->credentials(); + } + input.options = conversion::ConvertIcebergPropertiesToS3Secret( + conversion::MergeStorageCredential(io->properties(), credentials, table.location())); return input; } diff --git a/src/s3_conversion.cpp b/src/s3_conversion.cpp index 751431c..798fe99 100644 --- a/src/s3_conversion.cpp +++ b/src/s3_conversion.cpp @@ -138,4 +138,21 @@ ConvertIcebergPropertiesToS3Secret(const std::unordered_map +MergeStorageCredential(std::unordered_map properties, + std::span credentials, std::string_view location) { + const iceberg::StorageCredential *best = nullptr; + for (const auto &credential : credentials) { + if (location.starts_with(credential.prefix) && (!best || credential.prefix.size() > best->prefix.size())) { + best = &credential; + } + } + if (best) { + for (const auto &[key, value] : best->config) { + properties.insert_or_assign(key, value); + } + } + return properties; +} + } // namespace duckdb::conversion diff --git a/test/test_s3_conversion.cpp b/test/test_s3_conversion.cpp index eef4f58..0ad38a4 100644 --- a/test/test_s3_conversion.cpp +++ b/test/test_s3_conversion.cpp @@ -4,6 +4,7 @@ #include #include #include +#include using namespace duckdb; using namespace duckdb::conversion; @@ -170,3 +171,27 @@ TEST_CASE("secret round-trips through iceberg-cpp properties", "[s3_conversion]" {"use_ssl", Value::BOOLEAN(false)}}; REQUIRE(Render(ConvertIcebergPropertiesToS3Secret(ConvertS3SecretToIcebergProperties(secret))) == Render(secret)); } + +TEST_CASE("storage credential: without credentials the properties are unchanged", "[s3_conversion]") { + REQUIRE(Render(MergeStorageCredential({{"s3.access-key-id", "AKIA"}}, {}, "s3://bucket/table")) == + "s3.access-key-id=AKIA"); +} + +TEST_CASE("storage credential: the longest matching prefix overrides the properties", "[s3_conversion]") { + std::vector credentials = { + {.prefix = "s3://bucket/", .config = {{"s3.access-key-id", "BUCKET"}, {"s3.session-token", "bucket-token"}}}, + {.prefix = "s3://bucket/table", .config = {{"s3.access-key-id", "TABLE"}}}, + {.prefix = "s3://other/", .config = {{"s3.access-key-id", "OTHER"}}}, + }; + REQUIRE(Render(MergeStorageCredential({{"s3.access-key-id", "CATALOG"}, {"client.region", "eu-west-1"}}, + credentials, "s3://bucket/table")) == + "client.region=eu-west-1, s3.access-key-id=TABLE"); +} + +TEST_CASE("storage credential: credentials for other locations are ignored", "[s3_conversion]") { + std::vector credentials = { + {.prefix = "s3://other/", .config = {{"s3.access-key-id", "OTHER"}}}, + }; + REQUIRE(Render(MergeStorageCredential({{"s3.access-key-id", "CATALOG"}}, credentials, "s3://bucket/table")) == + "s3.access-key-id=CATALOG"); +} From 8b447f5d89e90bd377373052d297dc4212d1dc9a Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 05:54:48 +0100 Subject: [PATCH 3/9] Request vended credentials and turn off REST metrics reports The old fork sent X-Iceberg-Access-Delegation: vended-credentials with every request; upstream does not, and some catalogs only vend credentials when asked. Upstream also reports scan metrics to the catalog by default, which would add a request per scan outside the catalog lock. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/proto_iceberg_catalog_attach.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/proto_iceberg_catalog_attach.cpp b/src/proto_iceberg_catalog_attach.cpp index 1d602d1..e2d8ef8 100644 --- a/src/proto_iceberg_catalog_attach.cpp +++ b/src/proto_iceberg_catalog_attach.cpp @@ -28,6 +28,9 @@ using constants::kWarehouse; const string kUri = "uri"; const string kHeaderAuthorization = "header.Authorization"; +/// Asks the catalog to vend storage credentials with each loaded table. Some catalogs only vend when asked. +const string kHeaderAccessDelegation = "header.X-Iceberg-Access-Delegation"; +const string kVendedCredentials = "vended-credentials"; /// Path to look up the user's S3 secret at: only secrets scoped to all S3 paths match it, not per-bucket ones or the /// per-table secrets this extension creates. const string kS3RootPath = "s3://"; @@ -135,12 +138,15 @@ unique_ptr ProtoIcebergCatalog::Attach(optional_ptrAsCatalog(), "Failed to create Iceberg REST catalog at %s", params.uri); + UnwrapOrThrow(session_catalog->AsCatalog(), "Failed to open Iceberg REST catalog at %s", params.uri); auto catalog = make_uniq(db, std::move(params.uri), std::move(rest_catalog), std::move(params.default_schema)); From 070922f8284ce5ede951f193c3413593a8d471a6 Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 05:54:48 +0100 Subject: [PATCH 4/9] Match iceberg-cpp's S3 property and credential handling Upstream reads a boolean as true only for a case-insensitive "true", lets s3.ssl.enabled override the endpoint scheme, and matches storage credentials by canonical S3 prefix against full paths. Do the same, and match against the slash-terminated table location, so that DuckDB reads data files with the same settings and credentials iceberg-cpp uses for metadata. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/include/s3_conversion.hpp | 5 ++-- src/proto_iceberg_table_entry.cpp | 3 ++- src/s3_conversion.cpp | 43 +++++++++++++++++++------------ test/test_s3_conversion.cpp | 40 ++++++++++++++++++++++------ 4 files changed, 64 insertions(+), 27 deletions(-) diff --git a/src/include/s3_conversion.hpp b/src/include/s3_conversion.hpp index afee14d..6f58262 100644 --- a/src/include/s3_conversion.hpp +++ b/src/include/s3_conversion.hpp @@ -24,10 +24,11 @@ ConvertS3SecretToIcebergProperties(const case_insensitive_tree_t &secret) [[nodiscard]] case_insensitive_map_t ConvertIcebergPropertiesToS3Secret(const std::unordered_map &properties); -/// Overlays the vended storage credential whose prefix is the longest match for location onto FileIO properties. +/// Overlays the vended S3 storage credential whose prefix is the longest match for location onto FileIO properties. /// /// REST catalogs vend credentials either in the table config, which iceberg-cpp merges into the FileIO properties, -/// or as storage credentials scoped to location prefixes, which it keeps separately. +/// or as storage credentials scoped to location prefixes, which it keeps separately. Matching follows iceberg-cpp: +/// non-S3 prefixes are skipped and S3 scheme aliases such as s3a:// compare equal to s3://. [[nodiscard]] std::unordered_map MergeStorageCredential(std::unordered_map properties, std::span credentials, std::string_view location); diff --git a/src/proto_iceberg_table_entry.cpp b/src/proto_iceberg_table_entry.cpp index 7cd24d0..5085293 100644 --- a/src/proto_iceberg_table_entry.cpp +++ b/src/proto_iceberg_table_entry.cpp @@ -78,8 +78,9 @@ CreateSecretInput BuildScopedS3Secret(const string &catalog_name, const iceberg: if (const auto *credentialed = io->AsSupportsStorageCredentials()) { credentials = credentialed->credentials(); } + // Match on the slash-terminated table location, so that a credential for "/" applies. input.options = conversion::ConvertIcebergPropertiesToS3Secret( - conversion::MergeStorageCredential(io->properties(), credentials, table.location())); + conversion::MergeStorageCredential(io->properties(), credentials, input.scope.front())); return input; } diff --git a/src/s3_conversion.cpp b/src/s3_conversion.cpp index 798fe99..47423f2 100644 --- a/src/s3_conversion.cpp +++ b/src/s3_conversion.cpp @@ -1,6 +1,7 @@ #include "s3_conversion.hpp" #include "iceberg/arrow/s3/s3_properties.h" +#include "iceberg/util/property_util.h" #include #include @@ -34,17 +35,6 @@ constexpr std::array kPlainPropertyMappings = {{ {"region", S3Properties::kClientRegion}, }}; -/// Parses an iceberg-cpp boolean property, which is exactly "true" or "false". -std::optional ParseBool(std::string_view value) { - if (value == "true") { - return true; - } - if (value == "false") { - return false; - } - return std::nullopt; -} - /// Returns whether an Iceberg S3 endpoint's http(s) scheme implies SSL, or nullopt if it has no such scheme. std::optional SchemeUsesSsl(std::string_view endpoint) { if (endpoint.starts_with(kHttpsScheme)) { @@ -56,6 +46,16 @@ std::optional SchemeUsesSsl(std::string_view endpoint) { return std::nullopt; } +/// Rewrites an S3 scheme alias such as s3a:// or S3:// to s3://, as iceberg-cpp does before matching credential +/// prefixes. +std::string CanonicalizeS3Scheme(std::string_view location) { + if (auto separator = location.find("://"); + separator != std::string_view::npos && iceberg::arrow::IsS3Scheme(location.substr(0, separator))) { + return "s3://" + std::string(location.substr(separator + 3)); + } + return std::string(location); +} + /// Returns DuckDB's scheme-less form of an Iceberg S3 endpoint, e.g. host:9000/path for http://host:9000/path/. std::string_view ToDuckDBEndpoint(std::string_view endpoint) { if (endpoint.starts_with(kHttpsScheme)) { @@ -123,16 +123,20 @@ ConvertIcebergPropertiesToS3Secret(const std::unordered_map MergeStorageCredential(std::unordered_map properties, std::span credentials, std::string_view location) { + const auto canonical_location = CanonicalizeS3Scheme(location); const iceberg::StorageCredential *best = nullptr; + size_t best_length = 0; for (const auto &credential : credentials) { - if (location.starts_with(credential.prefix) && (!best || credential.prefix.size() > best->prefix.size())) { + if (!iceberg::arrow::IsS3CredentialPrefix(credential.prefix)) { + continue; + } + auto prefix = CanonicalizeS3Scheme(credential.prefix); + if (prefix.size() > best_length && canonical_location.starts_with(prefix)) { best = &credential; + best_length = prefix.size(); } } if (best) { diff --git a/test/test_s3_conversion.cpp b/test/test_s3_conversion.cpp index 0ad38a4..0827f7e 100644 --- a/test/test_s3_conversion.cpp +++ b/test/test_s3_conversion.cpp @@ -101,9 +101,11 @@ TEST_CASE("properties to secret: empty and unrelated properties are omitted", "[ {"s3.region", "us-east-1"}})) == ""); } -TEST_CASE("properties to secret: non-boolean path-style access and SSL enabled are ignored", "[s3_conversion]") { +TEST_CASE("properties to secret: booleans are true only for true in any case", "[s3_conversion]") { REQUIRE(Render(ConvertIcebergPropertiesToS3Secret( - {{"s3.path-style-access", "True"}, {"s3.ssl.enabled", "FALSE"}})) == ""); + {{"s3.path-style-access", "True"}, {"s3.ssl.enabled", "TRUE"}})) == "url_style='path', use_ssl=true"); + REQUIRE(Render(ConvertIcebergPropertiesToS3Secret({{"s3.path-style-access", "yes"}, {"s3.ssl.enabled", ""}})) == + "url_style='vhost', use_ssl=false"); } TEST_CASE("properties to secret: path-style access maps to URL style", "[s3_conversion]") { @@ -126,13 +128,13 @@ TEST_CASE("properties to secret: https endpoint drops the scheme and enables SSL "endpoint='s3.eu-west-1.amazonaws.com', use_ssl=true"); } -TEST_CASE("properties to secret: endpoint scheme takes precedence over SSL enabled", "[s3_conversion]") { +TEST_CASE("properties to secret: SSL enabled takes precedence over the endpoint scheme", "[s3_conversion]") { REQUIRE(Render(ConvertIcebergPropertiesToS3Secret( {{"s3.endpoint", "http://minio:9000"}, {"s3.ssl.enabled", "true"}})) == - "endpoint='minio:9000', use_ssl=false"); + "endpoint='minio:9000', use_ssl=true"); REQUIRE( Render(ConvertIcebergPropertiesToS3Secret({{"s3.endpoint", "https://storage"}, {"s3.ssl.enabled", "false"}})) == - "endpoint='storage', use_ssl=true"); + "endpoint='storage', use_ssl=false"); } TEST_CASE("properties to secret: scheme-less endpoint takes SSL from SSL enabled", "[s3_conversion]") { @@ -173,7 +175,7 @@ TEST_CASE("secret round-trips through iceberg-cpp properties", "[s3_conversion]" } TEST_CASE("storage credential: without credentials the properties are unchanged", "[s3_conversion]") { - REQUIRE(Render(MergeStorageCredential({{"s3.access-key-id", "AKIA"}}, {}, "s3://bucket/table")) == + REQUIRE(Render(MergeStorageCredential({{"s3.access-key-id", "AKIA"}}, {}, "s3://bucket/table/")) == "s3.access-key-id=AKIA"); } @@ -184,7 +186,7 @@ TEST_CASE("storage credential: the longest matching prefix overrides the propert {.prefix = "s3://other/", .config = {{"s3.access-key-id", "OTHER"}}}, }; REQUIRE(Render(MergeStorageCredential({{"s3.access-key-id", "CATALOG"}, {"client.region", "eu-west-1"}}, - credentials, "s3://bucket/table")) == + credentials, "s3://bucket/table/")) == "client.region=eu-west-1, s3.access-key-id=TABLE"); } @@ -192,6 +194,28 @@ TEST_CASE("storage credential: credentials for other locations are ignored", "[s std::vector credentials = { {.prefix = "s3://other/", .config = {{"s3.access-key-id", "OTHER"}}}, }; - REQUIRE(Render(MergeStorageCredential({{"s3.access-key-id", "CATALOG"}}, credentials, "s3://bucket/table")) == + REQUIRE(Render(MergeStorageCredential({{"s3.access-key-id", "CATALOG"}}, credentials, "s3://bucket/table/")) == "s3.access-key-id=CATALOG"); } + +TEST_CASE("storage credential: a prefix ending in a slash matches the table location", "[s3_conversion]") { + std::vector credentials = { + {.prefix = "s3://bucket/table/", .config = {{"s3.access-key-id", "TABLE"}}}, + }; + REQUIRE(Render(MergeStorageCredential({}, credentials, "s3://bucket/table/")) == "s3.access-key-id=TABLE"); +} + +TEST_CASE("storage credential: S3 scheme aliases compare equal", "[s3_conversion]") { + std::vector credentials = { + {.prefix = "s3a://bucket/", .config = {{"s3.access-key-id", "BUCKET"}}}, + }; + REQUIRE(Render(MergeStorageCredential({}, credentials, "S3://bucket/table/")) == "s3.access-key-id=BUCKET"); +} + +TEST_CASE("storage credential: non-S3 prefixes are skipped", "[s3_conversion]") { + std::vector credentials = { + {.prefix = "gs://bucket/", .config = {{"s3.access-key-id", "GCS"}}}, + {.prefix = "s3", .config = {{"s3.access-key-id", "S3"}}}, + }; + REQUIRE(Render(MergeStorageCredential({}, credentials, "s3://bucket/table/")) == "s3.access-key-id=S3"); +} From 0d59b63badb3275b5dde25cdfcbe642bafdfbc27 Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 06:00:12 +0100 Subject: [PATCH 5/9] Name the slash-terminated table location used for credential matching Co-Authored-By: Claude Opus 5.5 (1M context) --- src/proto_iceberg_table_entry.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/proto_iceberg_table_entry.cpp b/src/proto_iceberg_table_entry.cpp index 5085293..a1fe7ae 100644 --- a/src/proto_iceberg_table_entry.cpp +++ b/src/proto_iceberg_table_entry.cpp @@ -67,7 +67,7 @@ CreateSecretInput BuildScopedS3Secret(const string &catalog_name, const iceberg: auto input = MakeBaseS3SecretInput(); input.name = GenerateScopedSecretName(catalog_name, table.name(), txn_id); - input.scope.push_back(std::move(scope_prefix)); + input.scope.push_back(scope_prefix); // Scope to write.data.path too, that may live outside the table's location if (string write_data_path {table.properties().Get(iceberg::TableProperties::kWriteDataLocation)}; !write_data_path.empty()) { @@ -80,7 +80,7 @@ CreateSecretInput BuildScopedS3Secret(const string &catalog_name, const iceberg: } // Match on the slash-terminated table location, so that a credential for "/" applies. input.options = conversion::ConvertIcebergPropertiesToS3Secret( - conversion::MergeStorageCredential(io->properties(), credentials, input.scope.front())); + conversion::MergeStorageCredential(io->properties(), credentials, scope_prefix)); return input; } From b21b0adb2882d57724042787cac68a10c770ff1e Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 09:28:30 +0100 Subject: [PATCH 6/9] Turn off AWS SDK logging to avoid a crash at exit With the Arrow 25 and AWS SDK versions iceberg-cpp now bundles, finalizing Arrow's S3 support at exit sometimes crashed: an AWS CRT event loop still shutting down on another thread logged through the SDK's logger after the SDK had torn it down. With logging off the SDK installs no logger. The extension defaults ARROW_S3_LOG_LEVEL to off before S3 is initialized, keeping a value the user set. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/proto_iceberg_extension.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/proto_iceberg_extension.cpp b/src/proto_iceberg_extension.cpp index 7e74db7..bff2f10 100644 --- a/src/proto_iceberg_extension.cpp +++ b/src/proto_iceberg_extension.cpp @@ -13,6 +13,8 @@ #include "iceberg/arrow/arrow_register.h" #include "iceberg/avro/avro_register.h" +#include + namespace duckdb { namespace { @@ -53,6 +55,22 @@ void RegisterIcebergFileIO() { iceberg::avro::RegisterAll(); } +/// Turns off the AWS SDK's logging unless the user configured it, before iceberg-cpp initializes Arrow's S3 support. +/// +/// When Arrow finalizes S3 at exit, the AWS CRT can still be shutting down an event loop on another thread, which +/// logs through the SDK's logger after the SDK has torn it down and crashes. With logging off the SDK installs no +/// logger. Arrow reads ARROW_S3_LOG_LEVEL when S3 is first initialized. +void DisableAwsSdkLogging() { + static constexpr const char *kArrowS3LogLevel = "ARROW_S3_LOG_LEVEL"; +#ifdef _WIN32 + if (!std::getenv(kArrowS3LogLevel)) { + _putenv_s(kArrowS3LogLevel, "off"); + } +#else + setenv(kArrowS3LogLevel, "off", /*overwrite=*/0); +#endif +} + void RegisterIcebergSecretType(ExtensionLoader &loader) { SecretType iceberg_secret_type; iceberg_secret_type.name = kIcebergSecretType; @@ -74,6 +92,7 @@ void RegisterIcebergSecretFunction(ExtensionLoader &loader) { void LoadInternal(ExtensionLoader &loader) { auto &instance = loader.GetDatabaseInstance(); + DisableAwsSdkLogging(); RegisterIcebergFileIO(); RegisterIcebergSecretType(loader); From 90b69fded17fe78bba97cd2d21104ea130aacdb5 Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 09:34:30 +0100 Subject: [PATCH 7/9] Note the process-wide effect of the AWS SDK logging default Co-Authored-By: Claude Opus 5.5 (1M context) --- src/proto_iceberg_extension.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/proto_iceberg_extension.cpp b/src/proto_iceberg_extension.cpp index bff2f10..1facae3 100644 --- a/src/proto_iceberg_extension.cpp +++ b/src/proto_iceberg_extension.cpp @@ -59,7 +59,8 @@ void RegisterIcebergFileIO() { /// /// When Arrow finalizes S3 at exit, the AWS CRT can still be shutting down an event loop on another thread, which /// logs through the SDK's logger after the SDK has torn it down and crashes. With logging off the SDK installs no -/// logger. Arrow reads ARROW_S3_LOG_LEVEL when S3 is first initialized. +/// logger. Arrow reads ARROW_S3_LOG_LEVEL when S3 is first initialized. The variable is process-wide, so it also +/// applies to other Arrow S3 users in the process and to child processes. void DisableAwsSdkLogging() { static constexpr const char *kArrowS3LogLevel = "ARROW_S3_LOG_LEVEL"; #ifdef _WIN32 From fc9340ad99944cdb0e36ecf4c3641cbe41a2a505 Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 10:02:04 +0100 Subject: [PATCH 8/9] Bump DuckDB to v1.5.5 Move the duckdb and extension-ci-tools submodules, the workflows and the httpfs pin to v1.5.5. The macOS C++23 patch still applies unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/MainDistributionPipeline.yml | 12 ++++++------ .gitmodules | 2 +- cmake/duckdb_cxx23_patch.cmake | 4 ++-- docs/UPDATING.md | 2 +- duckdb | 2 +- extension-ci-tools | 2 +- extension_config.cmake | 4 ++-- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/.github/workflows/MainDistributionPipeline.yml b/.github/workflows/MainDistributionPipeline.yml index ed1de59..b4eb158 100644 --- a/.github/workflows/MainDistributionPipeline.yml +++ b/.github/workflows/MainDistributionPipeline.yml @@ -19,20 +19,20 @@ concurrency: jobs: duckdb-stable-build: name: Build extension binaries - uses: duckdb/extension-ci-tools/.github/workflows/_extension_distribution.yml@v1.5.1 + uses: duckdb/extension-ci-tools/.github/workflows/_extension_distribution.yml@v1.5.5 with: - duckdb_version: v1.5.1 - ci_tools_version: v1.5.1 + duckdb_version: v1.5.5 + ci_tools_version: v1.5.5 extension_name: proto_iceberg # iceberg-cpp does not build for Windows or WebAssembly yet. exclude_archs: 'windows_amd64;windows_amd64_mingw;wasm_mvp;wasm_eh;wasm_threads' code-quality-check: name: Code Quality Check - uses: duckdb/extension-ci-tools/.github/workflows/_extension_code_quality.yml@v1.5.1 + uses: duckdb/extension-ci-tools/.github/workflows/_extension_code_quality.yml@v1.5.5 with: - duckdb_version: v1.5.1 - ci_tools_version: v1.5.1 + duckdb_version: v1.5.5 + ci_tools_version: v1.5.5 extension_name: proto_iceberg # FIXME: Disable tidy to prevent full compilation format_checks: 'format' diff --git a/.gitmodules b/.gitmodules index fbc55fe..28c9b8c 100644 --- a/.gitmodules +++ b/.gitmodules @@ -5,7 +5,7 @@ [submodule "extension-ci-tools"] path = extension-ci-tools url = https://github.com/duckdb/extension-ci-tools - branch = v1.5.1 + branch = v1.5.5 # TODO: Switch to upstream apache/iceberg-cpp once requirements merged [submodule "third_party/iceberg-cpp"] path = third_party/iceberg-cpp diff --git a/cmake/duckdb_cxx23_patch.cmake b/cmake/duckdb_cxx23_patch.cmake index ff32c8d..79a1b08 100644 --- a/cmake/duckdb_cxx23_patch.cmake +++ b/cmake/duckdb_cxx23_patch.cmake @@ -1,4 +1,4 @@ -# DuckDB v1.5.1's profiling_utils.hpp does not compile in a C++23 translation unit under libc++ +# DuckDB v1.5's profiling_utils.hpp does not compile in a C++23 translation unit under libc++ # (Apple's standard library): QueryMetrics resets a unique_ptr in an inline member # function before ActiveTimer is complete, and libc++ instantiates the constexpr unique_ptr members # eagerly in C++23. The patch moves those member functions below ActiveTimer's definition. It is @@ -17,7 +17,7 @@ if(APPLE) RESULT_VARIABLE result) if(NOT result EQUAL 0) message(FATAL_ERROR "[proto_iceberg] Cannot apply ${patch} to the DuckDB tree at " - "${CMAKE_SOURCE_DIR}. Is it still DuckDB v1.5.1?") + "${CMAKE_SOURCE_DIR}. Is it still DuckDB v1.5?") endif() message(STATUS "[proto_iceberg] Applied ${patch}") endif() diff --git a/docs/UPDATING.md b/docs/UPDATING.md index 7a5f9b8..c006433 100644 --- a/docs/UPDATING.md +++ b/docs/UPDATING.md @@ -5,7 +5,7 @@ The extension targets a DuckDB release, which is pinned in several places that must move together. Check out submodule commits explicitly rather than using `make update` or `make pull`, which move every submodule to the tip of its tracked branch. - The `duckdb` submodule: check out the release tag, and set its `branch` in `.gitmodules` to the release branch. -- The `extension-ci-tools` submodule: check out the branch named after the release (e.g. `v1.5.1`), and set its `branch` in `.gitmodules` to match. +- The `extension-ci-tools` submodule: check out the branch named after the release (e.g. `v1.5.5`), and set its `branch` in `.gitmodules` to match. - `.github/workflows/MainDistributionPipeline.yml`: the reusable workflow refs and the `duckdb_version` and `ci_tools_version` inputs. - `extension_config.cmake`: the httpfs `GIT_TAG`, which should match `duckdb/.github/config/extensions/httpfs.cmake`. - `cmake/duckdb_cxx23.patch`: check whether it is still needed. On macOS, configuring fails if it no longer applies. diff --git a/duckdb b/duckdb index 7dbb2e6..d8cdaa3 160000 --- a/duckdb +++ b/duckdb @@ -1 +1 @@ -Subproject commit 7dbb2e646fea939a89f10a55aa98c474cbb0c098 +Subproject commit d8cdaa33fda8df955cc76ef58a280f68f4cd43fa diff --git a/extension-ci-tools b/extension-ci-tools index ef15a2a..72e76e9 160000 --- a/extension-ci-tools +++ b/extension-ci-tools @@ -1 +1 @@ -Subproject commit ef15a2a7453db5b4f85b7c668a545ae2f1193ff6 +Subproject commit 72e76e99cd7fee45a99739cd118ec2db64e034ec diff --git a/extension_config.cmake b/extension_config.cmake index f5faf5d..25436b3 100644 --- a/extension_config.cmake +++ b/extension_config.cmake @@ -1,5 +1,5 @@ # Extensions built by `make`: this one, plus httpfs, which the SQL tests use to read table data from S3. -# httpfs is pinned to the commit DuckDB v1.5.1 builds against. +# httpfs is pinned to the commit DuckDB v1.5.5 builds against. duckdb_extension_load(proto_iceberg SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR} @@ -8,5 +8,5 @@ duckdb_extension_load(proto_iceberg duckdb_extension_load(httpfs GIT_URL https://github.com/duckdb/duckdb-httpfs - GIT_TAG 7e86e7a5e5a1f01f458361bebdfa9b0a9a73a619 + GIT_TAG 827222fb45a043a7a852d1f7aae46901492a3cda ) From 855fca7134aaa829154004cc4f14dd8d33e33eac Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Sat, 26 Sep 2026 10:02:04 +0100 Subject: [PATCH 9/9] Build Windows binaries with MSVC DuckDB links the static MSVC runtime, so build iceberg-cpp and the libraries it bundles the same way: forward CMAKE_MSVC_RUNTIME_LIBRARY, let it reach dependencies that predate CMake 3.15, and set the option the AWS libraries use. windows_amd64_mingw stays excluded because rtools42's GCC predates C++23. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/MainDistributionPipeline.yml | 4 ++-- README.md | 2 +- cmake/iceberg_cpp.cmake | 9 +++++++++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/workflows/MainDistributionPipeline.yml b/.github/workflows/MainDistributionPipeline.yml index b4eb158..d455a81 100644 --- a/.github/workflows/MainDistributionPipeline.yml +++ b/.github/workflows/MainDistributionPipeline.yml @@ -24,8 +24,8 @@ jobs: duckdb_version: v1.5.5 ci_tools_version: v1.5.5 extension_name: proto_iceberg - # iceberg-cpp does not build for Windows or WebAssembly yet. - exclude_archs: 'windows_amd64;windows_amd64_mingw;wasm_mvp;wasm_eh;wasm_threads' + # windows_amd64_mingw builds with rtools42's GCC, which predates C++23; iceberg-cpp does not build for WebAssembly. + exclude_archs: 'windows_amd64_mingw;wasm_mvp;wasm_eh;wasm_threads' code-quality-check: name: Code Quality Check diff --git a/README.md b/README.md index d928564..62eb4f2 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ GEN=ninja make release # or: GEN=ninja make debug The first build takes longer: CMake downloads and builds iceberg-cpp and its vendored dependencies (Arrow, the AWS SDK and others) while configuring. Later builds skip this unless the submodule or its build settings change. Set `CMAKE_BUILD_PARALLEL_LEVEL` to limit build parallelism on machines with little memory. -CI also builds loadable binaries for Linux (amd64, arm64) and macOS 13.3+ (amd64, arm64) with DuckDB's [extension-ci-tools](https://github.com/duckdb/extension-ci-tools); each run of the Main Extension Distribution Pipeline workflow attaches them as artifacts. +CI also builds loadable binaries for Linux (amd64, arm64), macOS 13.3+ (amd64, arm64) and Windows (amd64) with DuckDB's [extension-ci-tools](https://github.com/duckdb/extension-ci-tools); each run of the Main Extension Distribution Pipeline workflow attaches them as artifacts. See [docs/UPDATING.md](docs/UPDATING.md) for updating DuckDB or iceberg-cpp. diff --git a/cmake/iceberg_cpp.cmake b/cmake/iceberg_cpp.cmake index 4c51ba1..f4994da 100644 --- a/cmake/iceberg_cpp.cmake +++ b/cmake/iceberg_cpp.cmake @@ -18,6 +18,8 @@ block() set(init_cache [==[ set(CMAKE_POSITION_INDEPENDENT_CODE ON CACHE BOOL "") set(CMAKE_INSTALL_LIBDIR lib CACHE PATH "") +# Lets the forwarded CMAKE_MSVC_RUNTIME_LIBRARY reach bundled dependencies that predate CMake 3.15. +set(CMAKE_POLICY_DEFAULT_CMP0091 NEW CACHE STRING "") # Build every dependency from source rather than picking up system packages. set(FETCHCONTENT_TRY_FIND_PACKAGE_MODE NEVER CACHE STRING "") set(ICEBERG_BUILD_STATIC ON CACHE BOOL "") @@ -40,6 +42,8 @@ set(ICEBERG_S3 ON CACHE BOOL "") CMAKE_OSX_ARCHITECTURES CMAKE_OSX_DEPLOYMENT_TARGET CMAKE_OSX_SYSROOT + # DuckDB links the static MSVC runtime, and all objects in the extension must agree. + CMAKE_MSVC_RUNTIME_LIBRARY # Set by the vcpkg toolchain, so that the sub-build uses the same installed packages. VCPKG_TARGET_TRIPLET VCPKG_HOST_TRIPLET @@ -49,6 +53,11 @@ set(ICEBERG_S3 ON CACHE BOOL "") endif() endforeach() + # The AWS libraries Arrow bundles choose their MSVC runtime from their own option instead. + if(MSVC AND CMAKE_MSVC_RUNTIME_LIBRARY AND NOT CMAKE_MSVC_RUNTIME_LIBRARY MATCHES "DLL") + string(APPEND init_cache "set(AWS_STATIC_MSVC_RUNTIME_LIBRARY ON CACHE BOOL \"\")\n") + endif() + execute_process(COMMAND git rev-parse HEAD WORKING_DIRECTORY "${source_dir}" OUTPUT_VARIABLE revision