-
Notifications
You must be signed in to change notification settings - Fork 8
Antalya 25.3: Support different warehouses behind Iceberg REST catalog #860
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.
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
Conversation
Tested on test installation in Altinity Cloud
after
|
{ | ||
throw ::DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Expected to find '{}' in data path: '{}'", common_path, data_path); | ||
/// Data files can have different path | ||
pos = data_path.find("://"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method docs say:
// For example, if the full blob path is s3://bucket/table_name/data/00000-1-1234567890.avro, the function will return table_name/data/00000-1-1234567890.avro
Which means the bucket
shall not be included in the return value as far as I understand.
As far as I understand your exception handling,you skip the initial "://"
and then copy everything after the next "/"
. But what if you are using Path-style URIs (i.e, https://s3.region-code.amazonaws.com/bucket-name/key-name)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to remove bucket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same S3/URI
comment here
Added code to support S3 tables
After allow empty path in location:
So added code to avoid insert region in url if region already there. Strange thing: But host with
when with
So now catalog works. But S3 tables not:
but query for data shows this:
|
Dirty fix. |
|
625f31b
to
95f4f0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still did not understand fully the purpose of this PR. Could you please write a summary of the changes explaining the use case, why it did not work and what you did to make it work?
Example:
use case 1: Iceberg specs allow data to live in a completely different storage than its metadata.
Problem in clickhouse 1: clickhouse holds a single storage path for each iceberg installation, it should have two..
Fixed in clickhouse 1: fixed it by adding a new variable and bla bla bla
throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "Unexpected location format: {}", location_); | ||
|
||
pos_to_path = pos_to_bucket + pos_to_path; | ||
{ // empty path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to have this comment, I suggest moving it into a variable instead:
bool empty_path = pos_to_path == std::string::npos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this better? It's an additional variable filled in runtime, and makes no logic changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a very opionated comment. I usually prefer named variables over comments because variables compile and we tend to forget to update comments.
In any case, I should have deleted or mentioned this is not a requirement. Don't worry about it.
throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "Unexpected location format: {}", location_); | ||
|
||
auto pos_to_bucket = pos + std::strlen("://"); | ||
auto pos_to_path = location_.substr(pos_to_bucket).find('/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be easier to just instantiate S3::URI
? AFAIK, it should support all types of S3 endpoints and its constructor takes a URI string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, Here location is just cut on two pieces, with Poco::URI come makes full analysis with splitting in all parts and later need to concat back.
|
||
// Do a list request because head requests don't have body in response | ||
std::optional<Aws::S3::S3Error> Client::updateURIForBucketForHead(const std::string & bucket) const | ||
// S3 Tables don't support ListObjects, so made dirty workaroung - changed on GetObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using GetObjects
with very small byte range 1-2.
// Do a list request because head requests don't have body in response | ||
std::optional<Aws::S3::S3Error> Client::updateURIForBucketForHead(const std::string & bucket) const | ||
// S3 Tables don't support ListObjects, so made dirty workaroung - changed on GetObject | ||
std::optional<Aws::S3::S3Error> Client::updateURIForBucketForHead(const std::string & bucket, const std::string & key) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this method? The name is updateURIForBucketForHead
, but it doesn't seem to update anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic inside AWS SDK. To get proper URI we must make some request to get 301 redirect. In response body AWS sends proper endpoint, and it extracted somewhere inside SDK. After that ClickHouse extracts it in method Client::getURIFromError.
But for this response must have body, while response on HeadObject request does not have one.
This is a reason for workaround with calling ListObjects instead, see this comment by Antonio (initial author of this code).
And ListObjests request is not supported by S3 Tables. That's why I changes it to GetObject for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a short comment about this and even add a link to this discussion? I find it surprising they did not do it.
{ | ||
throw ::DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Expected to find '{}' in data path: '{}'", common_path, data_path); | ||
/// Data files can have different path | ||
pos = data_path.find("://"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same S3/URI
comment here
ClickHouse expects that metadata and data have common non-empty path So this PR
|
Thanks for the explanation. I'll review it more carefully tomorrow morning. Skimmed through |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For change 1, see the comments
For change 2, I haven't reviewed it yet
For change 3, it is ok
For change 4, see the comments
Btw, I think most of these changes should also go into upstream. Please consider submitting it there.
return regions.contains(region); | ||
} | ||
|
||
void URI::addRegionToURI(const std::string ®ion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed during our call, the current use of this method is already "checking" if region is in the endpoint, but it's probably doing it wrong. Please correct the call site, add docs and examples.
It might be a good idea to rename this method to be addRegionToURIIfNeeded
or something of the sort and do the checks inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand code in Client.cpp.
Checks initial_endpoint, and add region to new_uri.
initial_endpoint is https://s3.amazonaws.com
, when new_uri is from error response:
<?xml version="1.0"?>
<?xml version="1.0" encoding="UTF-8"?>
<Error>
<Code>PermanentRedirect</Code>
<Message>The bucket you are attempting to access must be addressed using the specified endpoint. Please send all future requests to this endpoint.</Message>
<Endpoint>91f4f060-1a63-4ff0-oy49hiiqy45utsdypodf5p1j7awk4usw2b--table-s3.s3-us-west-2.amazonaws.com</Endpoint>
<Bucket>91f4f060-1a63-4ff0-oy49hiiqy45utsdypodf5p1j7awk4usw2b--table-s3</Bucket>
<RequestId>DCPGXTJ8PA0HVQ27</RequestId>
<HostId>ANM0Ek0N5xhfUCDwE2bxeVl2MhOl381L8SJz1JjA4lqcKJnZ1ySwT3c3fH1JGjMoCD1uYuTYa3SmEubEU14nx5IHvYGSjXM1</HostId>
</Error>
In response url with region.
What the hell is here? Why initial_endpoint???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial error:
2025.06.19 11:39:14.915513 [ 85 ] {14de1b7e-5a8f-4b3a-af2d-463e0bda9ba8} <Information> AWSClient: Failed to make request to: https://91f4f060-1a63-4ff0-oy49hiiqy45utsdypodf5p1j7awk4usw2b--table-s3.s3-us-west-2.us-west-2.amazonaws.com/metadata/00001-9c6d55c7-3a32-4bd8-a25d-dd2bbdad06ba.metadata.json: Code: 198. DB::NetException: Not found address of host: 91f4f060-1a63-4ff0-oy49hiiqy45utsdypodf5p1j7awk4usw2b--table-s3.s3-us-west-2.us-west-2.amazonaws.com. (DNS_ERROR), Stack trace (when copying this message, always include the lines below):
// For example, if the full blob path is s3://bucket/table_name/data/00000-1-1234567890.avro, the function will return table_name/data/00000-1-1234567890.avro | ||
// Common path should end with "<table_name>" or "<table_name>/". | ||
std::string getProperFilePathFromMetadataInfo(std::string_view data_path, std::string_view common_path, std::string_view table_location) | ||
std::string getProperFilePathFromMetadataInfo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've spent quite some time reviewing this function together and haven't understood it fully. Please document the possible values and examples for all the arguments here, where they come from and the scenarios.
That'll make reviewing this much easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data_path s3://aws-st-2-fs5vug37-iceberg/aws-public-blockchain/btc/metadata/snap-2539904009313210382-1-4f2e6056-d08e-4420-9bc9-47bc0dcbd6f9.avro
common_path aws-public-blockchain/btc
table_location s3://aws-st-2-fs5vug37-iceberg/aws-public-blockchain/btc
common_namespace aws-st-2-fs5vug37-iceberg
data_path s3://aws-st-2-fs5vug37-iceberg/ssb/lineorder_wide/data/199603/data.parquet
common_path ssb/lineorder_wide
table_location s3://aws-st-2-fs5vug37-iceberg/ssb/lineorder_wide
common_namespace aws-st-2-fs5vug37-iceberg
data_path s3://aws-st-2-fs5vug37-iceberg/nyc/test/metadata/snap-7890808452220287820-1-960673bb-b315-4df9-946e-fd34c44b98f7.avro
common_path nyc/test
table_location s3://aws-st-2-fs5vug37-iceberg/nyc/test
common_namespace aws-st-2-fs5vug37-iceberg
…ta_lake Antalya 25.3: Support different warehouses behind Iceberg REST catalog
…ta_lake Antalya 25.3: Support different warehouses behind Iceberg REST catalog
…t_860 25.6.5 Antalya port of #860: Support different warehouses behind Iceberg REST catalog
…t_860 25.8 Antalya port of #860: Support different warehouses behind Iceberg REST catalog
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Allow data and metadata with different paths.
Support S3 tables as a warehouse.
Documentation entry for user-facing changes
Solved #856
Exclude tests: