Added support for s3 tables by subkanthi · Pull Request #1617 · Altinity/ClickHouse

subkanthi · 2026-04-05T18:10:44Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Support for s3 tables(iceberg REST Catalog), closes #ClickHouse#95340

Documentation entry for user-facing changes

Support for s3 tables(Iceberg serverless REST Catalog) using sigv4 authentication.
Can be enabled with allow_experimental_database_s3_tables

CI/CD Options

Exclude tests:

Regression jobs to run:

github-actions · 2026-04-05T18:11:54Z

Workflow [PR], commit [8acaa19]

…s only support single namespace, for every namespace , make call to getTables. Use ThreadPoolCallbackRunnerLocal to perform in thredpool. tryGetTableMetadata , Inject https://s3.{region}.amazonaws.com so that requests dont goto bucket.s3.amazonaws.com. pre-strips the s3://bucket-name/ prefix, leaving just the relative path metadata/00001-....json, so downstream code constructs the correct S3 key. Detects missing/empty credentials and injects the catalog's own IAM credentials from credentials_provider->GetAWSCredentials().

subkanthi · 2026-04-06T22:16:42Z

TESTING
Using Athena to create iceberg tables in s3Tables

CREATE TABLE `nyc`.daily_sales (
sale_date date, 
product_category string, 
sales_amount double)
PARTITIONED BY (month(sale_date))
TBLPROPERTIES ('table_type' = 'iceberg')

INSERT INTO daily_sales
VALUES
(DATE '2024-01-15', 'Laptop', 900.00),
(DATE '2024-01-15', 'Monitor', 250.00),
(DATE '2024-01-16', 'Laptop', 1350.00),
(DATE '2024-02-01', 'Monitor', 300.00),
(DATE '2024-02-01', 'Keyboard', 60.00),
(DATE '2024-02-02', 'Mouse', 25.00),
(DATE '2024-02-02', 'Laptop', 1050.00),
(DATE '2024-02-03', 'Laptop', 1200.00),
(DATE '2024-02-03', 'Monitor', 375.00);

SET allow_experimental_database_s3_tables = 1;

CREATE DATABASE my_s3_tables_sales
ENGINE = DataLakeCatalog('https://s3tables.us-east-2.amazonaws.com/iceberg')
SETTINGS
    catalog_type = 's3tables',
    warehouse = 'arn:aws:s3tables:us-east-2:<account_id>:bucket/nyc-taxis',
    aws_access_key_id = '',
    aws_secret_access_key = '',
    region = 'us-east-2';

use my_s3_tables_sales;

USE my_s3_tables_sales

Query id: 6e72f6fd-568f-4dd2-95fa-b1ff6bddd7df

Ok.

0 rows in set. Elapsed: 0.002 sec. 

Ubuntu-2404-noble-amd64-base :) select * from my_s3_tables_sales.`nyc.daily_sales`;

SELECT *
FROM my_s3_tables_sales.`nyc.daily_sales`

Query id: 1cec644c-8927-4b9c-a2c3-b770375f7734

   ┌──sale_date─┬─product_category─┬─sales_amount─┐
1. │ 2024-01-15 │ Laptop           │          900 │
2. │ 2024-01-15 │ Monitor          │          250 │
3. │ 2024-01-16 │ Laptop           │         1350 │
4. │ 2024-02-01 │ Monitor          │          300 │
5. │ 2024-02-01 │ Keyboard         │           60 │
6. │ 2024-02-02 │ Mouse            │           25 │
7. │ 2024-02-02 │ Laptop           │         1050 │
8. │ 2024-02-03 │ Laptop           │         1200 │
9. │ 2024-02-03 │ Monitor          │          375 │
   └────────────┴──────────────────┴──────────────┘

mkmkme · 2026-04-07T08:53:12Z

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #1617 (Added support for s3 tables / S3 Tables Iceberg REST catalog):

Confirmed defects:

No confirmed defects in reviewed scope.

Coverage summary:

Scope reviewed: PR diff only—settings/enums, `DatabaseDataLake` registration and validation, new `AWSV4Signer` + `S3TablesCatalog`, `TableMetadata::hasDataLakeSpecificProperties`, `RestCatalog` virtual hooks, `S3Credentials::isEmpty`, experimental-settings enablement
Categories failed: none
Categories passed: build/link assumptions (`add_headers_and_sources` for `Databases/DataLake`), SigV4 signer wiring vs AWS SDK `AWSAuthV4Signer` API, guarded catalog factory paths (`USE_AWS_S3`/`USE_SSL` vs TU `USE_AVRO`/`USE_PARQUET`), parallel `getTables` vs existing `RestCatalog` patterns, credential/endpoint/metadata-path overrides
Assumptions/limits: no build or integration tests were run; defects would require runtime/catalog-specific failures not evidenced in the diff alone

arthurpassos · 2026-04-08T17:19:28Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dbcf541b8d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-04-08T17:30:19Z

src/Databases/DataLake/S3TablesCatalog.h

+
+/// Iceberg REST catalog for Amazon S3 Tables (SigV4, signing name `s3tables`).
+/// https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-tables-integrating-open-source.html
+class S3TablesCatalog final : public RestCatalog


Override S3 Tables drop to request purge

S3TablesCatalog inherits RestCatalog::dropTable, which hardcodes ?purgeRequested=False in the DELETE URL; AWS S3 Tables documents that purge=false is rejected with 400 Bad Request, so DROP TABLE against catalog_type='s3tables' will fail consistently. Please override the drop path for this catalog to send a purge-enabled delete request.

Useful? React with 👍 / 👎.

Addressed, added support for dropTable

chatgpt-codex-connector · 2026-04-08T17:30:19Z

src/Databases/DataLake/DatabaseDataLake.cpp

 {
-    if (settings[DatabaseDataLakeSetting::catalog_type].value == DB::DatabaseDataLakeCatalogType::GLUE)
+    if (settings[DatabaseDataLakeSetting::catalog_type].value == DB::DatabaseDataLakeCatalogType::GLUE
+        || settings[DatabaseDataLakeSetting::catalog_type].value == DB::DatabaseDataLakeCatalogType::S3_TABLES)


Enforce non-empty warehouse for s3tables

validateSettings() now groups S3_TABLES with Glue and skips the warehouse check, but this catalog still relies on warehouse for loadConfig() (/v1/config?warehouse=...) and for deriving config.prefix when the server does not return one; allowing an empty warehouse defers this to runtime failures on namespace/table requests instead of rejecting invalid configuration at CREATE DATABASE time.

Useful? React with 👍 / 👎.

addressed added separate check for S3_TABLES

arthurpassos · 2026-04-08T17:04:50Z

src/Databases/DataLake/S3TablesCatalog.cpp

+
+    LOG_DEBUG(log, "Requesting: {}", url.toString());
+
+    return DB::BuilderRWBufferFromHTTP(url)


I see the base class implementation has some sort of retry mechanism, why not have it here as well?

try { return create_buffer(false); } catch (const DB::HTTPException & e) { const auto status = e.getHTTPStatus(); if (update_token_if_expired && (status == Poco::Net::HTTPResponse::HTTPStatus::HTTP_UNAUTHORIZED || status == Poco::Net::HTTPResponse::HTTPStatus::HTTP_FORBIDDEN)) { return create_buffer(true); } throw; }

Added retry.

arthurpassos · 2026-04-08T17:06:36Z

src/Databases/DataLake/S3TablesCatalog.cpp

+
+    const auto & context = getContext();
+
+    Poco::URI url(endpoint, /* enable_url_encoding */ false);


/// enable_url_encoding=false to allow use tables with encoded sequences in names like 'foo%2Fbar'

arthurpassos · 2026-04-08T18:04:12Z

src/Databases/DataLake/AWSV4Signer.cpp

+
+}
+
+void signRequestWithAWSV4(


Nit: Perhaps name it like signHeaders as the output of this method is a list of headers and not a request?

if (!payload.empty()) { auto body_stream = Aws::MakeShared<std::stringstream>("AWSV4Signer"); body_stream->write(payload.data(), static_cast<std::streamsize>(payload.size())); body_stream->seekg(0); request.AddContentBody(body_stream); } static constexpr bool sign_body = true; if (!signer.SignRequest(request, region.c_str(), service.c_str(), sign_body))

The request body is also signed

arthurpassos · 2026-04-08T18:07:43Z

src/Databases/DataLake/AWSV4Signer.cpp

+
+    static constexpr bool sign_body = true;
+    if (!signer.SignRequest(request, region.c_str(), service.c_str(), sign_body))
+        throw DB::Exception(DB::ErrorCodes::LOGICAL_ERROR, "AWS SigV4 signing failed");


LOGICAL_ERROR will crash ClickHouse, it should be something else

changed to S3_ERROR

arthurpassos · 2026-04-08T18:13:29Z

src/Databases/DataLake/S3TablesCatalog.cpp

+        Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Always,
+        /* urlEscapePath = */ false);
+
+    config = loadConfig();


This is kind of dangerous. You've made createReadBuffer virtual, and loadConfig calls it. If S3TablesCatalog::createReadBuffer relies on members that are initialized after loadConfig, it is UB

Looking at the code, there doesn't seem to be an imminent problem, but i would at least add a comment about this?

I don't see a way around this "problem" tbh

Maybe this solves the problem? https://github.com/Altinity/ClickHouse/pull/1617/changes#r3053334453

arthurpassos · 2026-04-08T18:22:31Z

src/Databases/DataLake/S3TablesCatalog.cpp

+    return true;
+}
+
+DB::HTTPHeaderEntries S3TablesCatalog::getAuthHeaders(bool /* update_token */) const


Why not use this method to implement the signing instead of overwriting the createReadBuffer method?

moved logic to getAuthHeaders

subkanthi · 2026-04-13T18:30:38Z

Added support for drop table

drop table my_s3_tables_sales.`nyc.daily_sales_4`;

DROP TABLE my_s3_tables_sales.`nyc.daily_sales_4`

Query id: cd68b468-2eba-4330-8b89-cdc4f76074d6

Ok.

0 rows in set. Elapsed: 0.849 sec.

…r to getAuthHeaders

arthurpassos

I haven't finished reviewing it yet, a few files are still pending

arthurpassos · 2026-04-13T22:35:15Z

src/Databases/DataLake/RestCatalog.h

    Poco::Net::HTTPBasicCredentials credentials{};

-    DB::ReadWriteBufferFromHTTPPtr createReadBuffer(
+    virtual DB::ReadWriteBufferFromHTTPPtr createReadBuffer(


I think you no longer need the virtual modifier here and in sendRequest

arthurpassos · 2026-04-14T12:41:39Z

src/Databases/DataLake/S3TablesCatalog.cpp

+    }
+}
+
+DB::Names S3TablesCatalog::getTables() const


Can you add a comment explaining why this has to be overriden? The base method seems to be kind of similar

arthurpassos · 2026-04-14T12:46:33Z

src/Databases/DataLake/S3TablesCatalog.cpp

+    if (!result.requiresCredentials())
+        return true;
+
+    bool need_credentials = !result.hasStorageCredentials() || !result.getStorageCredentials();


As far as I can tell, neither TableMetadata::hasStorageCredentials or TableMetadata::getStorageCredentials are virtual. And their default implementation reference the same member TableMetadata::storage_credentials. Which means !result.hasStorageCredentials() || !result.getStorageCredentials() is redundant and can be simplified to !result.hasStorageCredentials()

Actually, isn't it simpler and equivalent to just do:

bool need_credentials = true if (const auto storage_credentials = result.getStorageCredentials()) { auto creds = std::dynamic_pointer_cast<S3Credentials>(storage_credentials); if (creds && !creds->isEmpty()) need_credentials = false; }

arthurpassos · 2026-04-14T12:54:34Z

src/Databases/DataLake/S3TablesCatalog.cpp

+        LOG_DEBUG(log, "S3 Tables: no vended credentials for {}.{}, injecting catalog IAM credentials", namespace_name, table_name);
+        auto aws_creds = credentials_provider->GetAWSCredentials();
+        result.setStorageCredentials(std::make_shared<S3Credentials>(
+            String(aws_creds.GetAWSAccessKeyId().c_str(), aws_creds.GetAWSAccessKeyId().size()),


Do you really need to grab the internal const char pointer and then create a String from it? I suspect you can just do: std::make_shared<S3Credentials>(aws_creds.GetAWSAccessKeyId())

See

ClickHouse/src/Databases/DataLake/GlueCatalog.cpp

Line 429 in 9ab63c5

auto s3_creds = std::make_shared<S3Credentials>(credentials.GetAWSAccessKeyId(), credentials.GetAWSSecretKey(), credentials.GetSessionToken());

arthurpassos · 2026-04-14T12:56:55Z

src/Databases/DataLake/S3TablesCatalog.cpp

+            String(aws_creds.GetSessionToken().c_str(), aws_creds.GetSessionToken().size())));
+    }
+
+    if (result.getEndpoint().empty())


IIRC, AWS s3 supports several types of endpoint formats. Are you sure it is ok to support only s3.<region>.amazonaws.com? And I see it is also hard-coded to https.

See

ClickHouse/src/IO/S3/URI.cpp

Line 35 in 9ab63c5

URI::URI(const std::string & uri_, bool allow_archive_path_syntax, bool keep_presigned_query_parameters)

arthurpassos · 2026-04-14T12:58:23Z

src/Databases/DataLake/S3TablesCatalog.cpp

+        result.setEndpoint(regional_endpoint);
+    }
+
+    if (result.hasDataLakeSpecificProperties())


I think this can be simplified to if (const auto props = result.getDataLakeSpecificProperties()). No need to check for hasDataLakeSpecificProperties and props.has_value()

arthurpassos · 2026-04-14T12:59:46Z

@codex review

arthurpassos · 2026-04-14T13:02:32Z

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #1617 (S3 Tables Iceberg REST catalog support):

Confirmed defects:

Medium: Case-sensitive auth-header filtering drops SigV4 headers
Impact: S3 Tables REST requests can be sent without required SigV4 auth headers, causing consistent auth failures (401/403) for catalog operations.
Anchor: src/Databases/DataLake/S3TablesCatalog.cpp / S3TablesCatalog::getAuthHeaders
Trigger: Any path where signRequestWithAWSV4 emits canonical-cased headers (Authorization, X-Amz-*) instead of lowercase (authorization, x-amz-*), which is normal for AWS SDK signers.
Why defect: The filter keeps only exact lowercase names:
h.name == "authorization"
h.name.starts_with("x-amz-")
while upstream signing/validation code is already case-insensitive (Poco::icompare), indicating header case is not guaranteed.
Fix direction (short): Make the filter case-insensitive (Poco::icompare for authorization, case-insensitive prefix check for x-amz-).
Regression test direction (short): Unit/integration test that mocks signer output with Authorization and X-Amz-Date header casing and verifies outgoing HTTP request still contains auth headers.
Coverage summary:

Scope reviewed: Full provided diff for PR #1617 (Settings*, DatabaseDataLake, RestCatalog, new AWSV4Signer, new S3TablesCatalog, ICatalog, StorageCredentials, experimental setting enablement).
Categories failed: SigV4 header propagation / error-contract consistency between signer output and outbound header filtering.
Categories passed: Catalog-type registration and gating, required-setting validation (region/warehouse), drop-table purge path for S3 Tables, metadata-location normalization path, threaded namespace/table enumeration synchronization, C++ lifetime/ownership checks in new code.
Assumptions/limits: Static audit only from supplied diff/context (no runtime build or live AWS integration execution).

chatgpt-codex-connector · 2026-04-14T13:19:05Z

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Added support for s3 tables

f52a708

subkanthi marked this pull request as ready for review April 6, 2026 21:10

subkanthi force-pushed the s3table_support branch 3 times, most recently from 301f82e to dbcf541 Compare April 6, 2026 21:44

svb-alt added antalya antalya-26.1 labels Apr 6, 2026

chatgpt-codex-connector bot reviewed Apr 8, 2026

View reviewed changes

arthurpassos reviewed Apr 8, 2026

View reviewed changes

subkanthi added 2 commits April 11, 2026 03:48

Add retry mechanism

f035cca

Changed from LOGICAL_ERROR to S3_ERROR

0cb0725

Added support for dropTable, moved signing logic from createReadBuffe…

8acaa19

…r to getAuthHeaders

subkanthi requested a review from arthurpassos April 13, 2026 18:38

arthurpassos reviewed Apr 14, 2026

View reviewed changes


		LOG_DEBUG(log, "Requesting: {}", url.toString());

		return DB::BuilderRWBufferFromHTTP(url)


		const auto & context = getContext();

		Poco::URI url(endpoint, /* enable_url_encoding */ false);


		}

		void signRequestWithAWSV4(

Conversation

subkanthi commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

CI/CD Options

Exclude tests:

Regression jobs to run:

Uh oh!

github-actions bot commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subkanthi commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkmkme commented Apr 7, 2026

Uh oh!

arthurpassos commented Apr 8, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subkanthi commented Apr 13, 2026

Uh oh!

arthurpassos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurpassos commented Apr 14, 2026

Uh oh!

arthurpassos commented Apr 14, 2026

subkanthi commented Apr 5, 2026 •

edited

Loading

github-actions bot commented Apr 5, 2026 •

edited

Loading

subkanthi commented Apr 6, 2026 •

edited

Loading