Allow to read Iceberg data from any location #1092

zvonand · 2025-10-20T04:57:53Z

Successor of #1008.

Closes #963, ClickHouse#84609. PR to upstream is upcoming

Changelog category (leave one):

Improvement

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

Allow to read Iceberg data from any location

CI/CD Options

Exclude tests:

Regression jobs to run:

github-actions · 2025-10-20T04:58:45Z

Workflow [PR], commit [2e942f9]

arthurpassos · 2025-11-21T11:28:05Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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 · 2025-11-21T11:35:13Z

src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergIterator.cpp

+        std::cerr << "[MASTER IcebergIterator::next] file_path=" << manifest_file_entry.file_path 
+                  << " resolved_key=" << resolved_key 
+                  << " storage_type=" << (storage_to_use ? toString(storage_to_use->getType()) : "null")
+                  << " storage_desc=" << (storage_to_use ? storage_to_use->getDescription() : "null")
+                  << " storage_same_as_base=" << (storage_to_use == object_storage ? "true" : "false")


Avoid uncontrolled stderr logging in iterator

IcebergIterator::next() now prints details of every manifest entry directly to std::cerr rather than through the logger. This runs for every data file yielded by the iterator, so a query that scans many files will spam the server’s stderr with unstructured output, bypassing log levels and incurring extra I/O cost. Please drop these ad‑hoc std::cerr writes or route them through the existing logger with configurable levels.

Useful? React with 👍 / 👎.

This is a desperate step to debug code: the fail does not reproduce locally or when running test suite locally, only in CI
These lines, of course, will be removed

arthurpassos

First pass. Will do one more later

src/Disks/ObjectStorages/IObjectStorage.h

src/Interpreters/ClusterFunctionReadTask.cpp

src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergDataObjectInfo.cpp

arthurpassos · 2025-11-21T11:36:01Z

src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergDataObjectInfo.cpp

+    , underlying_format_read_schema_id(data_manifest_file_entry_.schema_id)
+    , sequence_number(data_manifest_file_entry_.added_sequence_number)
+{
+    if (!position_deletes_objects.empty() && toupper(data_manifest_file_entry_.file_format) != "PARQUET")


I thought the PR was about reading data from any location. What does it have to do with "position deletes"?

because we also use paths there

src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergIterator.cpp

src/Storages/ObjectStorage/Utils.cpp

arthurpassos · 2025-11-21T11:49:07Z

src/Storages/ObjectStorage/Utils.cpp

-extern const SettingsString filesystem_cache_name;
-extern const SettingsUInt64 filesystem_cache_boundary_alignment;
+    if (!isAbsolutePath(path))
+        return {base_storage, normalizePathToStorageRoot(table_location, path)}; // Relative path definitely goes to base storage


If it is not absolute, it is relative. Why do you need to "normalize" it to root? Is it because you are now making everything relative to the root instead of the data directory?

yes, we normalize it to storage root here. previously, paths were (and still could be) relative to table location.

added a comment

src/Storages/ObjectStorage/Utils.cpp

try to fix(2)

arthurpassos · 2025-11-22T00:20:53Z

@codex review

maybe I'm onto something another attempt polish

chatgpt-codex-connector · 2025-11-22T00:30:41Z

💡 Codex Review

ClickHouse/src/Storages/ObjectStorage/Utils.cpp

Lines 333 to 379 in 7059ca8

    
           if (target_scheme_normalized == "s3" || target_scheme_normalized == "https" || target_scheme_normalized == "http") 
        
           { 
        
               std::string normalized_path = path; 
        
               if (target_decomposed.scheme == "s3a" || target_decomposed.scheme == "s3n") 
        
               { 
        
                   normalized_path = "s3://" + target_decomposed.authority + "/" + target_decomposed.key; 
        
               } 
        
               S3::URI s3_uri(normalized_path); 
        
               std::string key_to_use = s3_uri.key; 
        
               bool use_base_storage = false; 
        
               if (base_storage->getType() == ObjectStorageType::S3) 
        
               { 
        
                   if (auto s3_storage = std::dynamic_pointer_cast<S3ObjectStorage>(base_storage)) 
        
                   { 
        
                       const std::string base_bucket = s3_storage->getObjectsNamespace(); 
        
                       const std::string base_endpoint = s3_storage->getDescription(); 
        
                       if (s3URIMatches(s3_uri, base_bucket, base_endpoint, target_scheme_normalized)) 
        
                           use_base_storage = true; 
        
                   } 
        
               } 
        
               if (!use_base_storage && (base_scheme_normalized == "s3" || base_scheme_normalized == "https" || base_scheme_normalized == "http")) 
        
               { 
        
                   std::string normalized_table_location = table_location; 
        
                   if (table_location_decomposed.scheme == "s3a" || table_location_decomposed.scheme == "s3n") 
        
                   { 
        
                       normalized_table_location = "s3://" + table_location_decomposed.authority + "/" + table_location_decomposed.key; 
        
                   } 
        
                   S3::URI base_s3_uri(normalized_table_location); 
        
                   if (s3URIMatches(s3_uri, base_s3_uri.bucket, base_s3_uri.endpoint, target_scheme_normalized)) 
        
                       use_base_storage = true; 
        
               } 
        
               if (use_base_storage) 
        
                   return {base_storage, key_to_use}; 
        
               const std::string storage_cache_key = "s3://" + s3_uri.bucket + "@" + (s3_uri.endpoint.empty() ? "amazonaws.com" : s3_uri.endpoint); 
        
               return getOrCreateStorageAndKey( 
        
                   storage_cache_key, 
        
                   key_to_use, 
        
                   "s3", 
        
                   secondary_storages,

Avoid forcing all HTTPS paths through S3 resolver

The new resolveObjectStorageForPath treats every https/http URI as S3 and unconditionally returns a S3 storage (lines 333‑379). For Iceberg data or manifests stored on a non‑S3 HTTPS endpoint (e.g., Azure https://account.blob.core.windows.net/... or GCS), this branch will now bypass the Azure/HDFS/file fallbacks and try to instantiate an S3 client with the wrong credentials, causing such tables to become unreadable. Previously those URLs would have been handled by the appropriate storage or the base storage, so this is a regression for any non‑S3 HTTPS paths in metadata.

ℹ️ 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".

arthurpassos

In general it looks ok to me. We must push this to upstream as well, otherwise it will be a pain to rebase

arthurpassos · 2025-11-22T00:22:29Z

src/Disks/ObjectStorages/IObjectStorage.h

        : relative_path(std::move(command_or_path))
        , metadata(std::move(metadata_))
        , command(relative_path)
+        , absolute_path((absolute_path_.has_value() && !absolute_path_.value().empty()) ? absolute_path_ : std::nullopt)


If empty is being treated the same as std::nullopt, why not just std::string?

initial idea was to make the object reading logic shorter and avoid a lot of if (!absolute_path.empty())
I agree, maybe it shall be refactored. But since this is a more or less style isue, I would prefer to merge it as is now not to delay release any further and improve it when submitting to upstream.

zvonand · 2025-11-22T10:43:17Z

We must push this to upstream as well

This is still my plan. I just avoided submitting a PR that is not ready -- there is then a bigger chance no one will ever notice it when it is ready.

Follow-up for #1092

zvonand added antalya port-antalya PRs to be ported to all new Antalya releases antalya-25.8 labels Oct 20, 2025

zvonand mentioned this pull request Oct 20, 2025

[WiP] Allow to read Iceberg data from any location #1008

Closed

13 tasks

zvonand force-pushed the bugfix/antalya-25.8/963 branch 3 times, most recently from 71a727b to 8dee385 Compare October 22, 2025 16:13

zvonand force-pushed the bugfix/antalya-25.8/963 branch from a5703dd to dad344b Compare October 28, 2025 09:09

zvonand force-pushed the bugfix/antalya-25.8/963 branch 6 times, most recently from 84e859c to 9a8abbd Compare November 14, 2025 10:48

zvonand force-pushed the bugfix/antalya-25.8/963 branch 11 times, most recently from 95b9526 to 91a7e14 Compare November 21, 2025 09:13

chatgpt-codex-connector bot reviewed Nov 21, 2025

View reviewed changes

arthurpassos reviewed Nov 21, 2025

View reviewed changes

zvonand force-pushed the bugfix/antalya-25.8/963 branch from 91a7e14 to 7f3d8dc Compare November 21, 2025 11:55

zvonand added 6 commits November 21, 2025 22:23

Allow to read Iceberg data from any location (pt 1)

cf964c0

works for file

7f1cc56

all integration tests work

963e7fe

try to fix last test

28c8048

Cleanup

8d5ee53

try to fix

07f9c57

try to fix(2)

zvonand force-pushed the bugfix/antalya-25.8/963 branch from 891a001 to 7059ca8 Compare November 21, 2025 22:10

zvonand requested a review from arthurpassos November 21, 2025 22:11

desperate step: add debug output to see what happens in CI

2e942f9

maybe I'm onto something another attempt polish

zvonand force-pushed the bugfix/antalya-25.8/963 branch from 7059ca8 to 2e942f9 Compare November 22, 2025 00:30

arthurpassos approved these changes Nov 22, 2025

View reviewed changes

zvonand changed the title ~~[WiP] Allow to read Iceberg data from any location~~ Allow to read Iceberg data from any location Nov 22, 2025

zvonand merged commit 3841418 into antalya-25.8 Nov 22, 2025
292 of 294 checks passed

zvonand mentioned this pull request Nov 23, 2025

Follow-up for #1092 #1163

Merged

25 tasks

zvonand added a commit that referenced this pull request Nov 24, 2025

Merge pull request #1163 from Altinity/bugfix/antalya-25.8/963-followup

ed69896

Follow-up for #1092

zvonand mentioned this pull request Nov 24, 2025

Registering a file to Iceberg table with name that is almost identical to table path cause query failure #963

Closed

zvonand mentioned this pull request Dec 10, 2025

Antalya 25.8: Fix reading Iceberg tables with local metadata and data #1212

Merged

25 tasks

Allow to read Iceberg data from any location #1092

Allow to read Iceberg data from any location #1092

Uh oh!

Conversation

zvonand commented Oct 20, 2025 • 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):

CI/CD Options

Exclude tests:

Regression jobs to run:

Uh oh!

github-actions bot commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurpassos commented Nov 21, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurpassos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

arthurpassos commented Nov 22, 2025

Uh oh!

chatgpt-codex-connector bot commented Nov 22, 2025

💡 Codex Review

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!

zvonand commented Nov 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zvonand commented Oct 20, 2025 •

edited

Loading

github-actions bot commented Oct 20, 2025 •

edited

Loading