Add support for hive partition style reads and writes

[arthurpassos]

Changelog category (leave one):

• Backward Incompatible Change

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

Add support for hive partition style writes and refactor reads implementation (hive partition columns are no longer virtual).

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

[arthurpassos]

I'll add tests soon

[alexey-milovidov]

This is good, and it could replace a problematic feature: #23051
Change to "New Feature".

Let's make sure names and values are URL-encoded.

One potential problem is memory usage when writing to many partitions at the same time. Let's define some limit, so we will create only up to this limit number of buffers for writing to S3 at the same time.

Do we control where exactly the path fragment with partition goes in the URL?
Do we control if the partition columns will be written or omitted from the files?

[clickhouse-gh]

Workflow [PR], commit [27e26d9]

[arthurpassos]

Change to "New Feature".

Done

Let's make sure names and values are URL-encoded.

Ack

[arthurpassos]

Do we control where exactly the path fragment with partition goes in the URL?

As of now, it us up to the user to choose where in the path the partition goes by using the {_partition_id} macro/ placeholder when creating the table.

Maybe we can make it simpler: user is responsible for defining the table root, that's all. The rest (partition key location, filename and file extension) clickhouse will generate.

engine = s3('bucket/table_root') partition by (year, country) -> 'bucket/table_root/year=2025/country=spain/<generated_uuid>.<format from table>'.

If the user specifies the partition_id placeholder and use_hive=1, we throw exception.

What do you think? @alexey-milovidov

[arthurpassos]

Do we control if the partition columns will be written or omitted from the files?

I suppose that could be implemented, but perhaps we should leave it for a follow up PR?

[alexey-milovidov]

Maybe we can make it simpler: user is responsible for defining the table root, that's all. The rest (partition key location, filename and file extension) clickhouse will generate.

Yes, this is a great idea!

This PR is good, but what I'd like to see in addition, before merging it, is - fixing the memory consumption problem with PARTITION BY. It's an old flaw of the current mechanism. Having this new feature will make it more frequently used, and the users will bump into this problem more frequently.

[arthurpassos]

Maybe we can make it simpler: user is responsible for defining the table root, that's all. The rest (partition key location, filename and file extension) clickhouse will generate.

Yes, this is a great idea!

This PR is good, but what I'd like to see in addition, before merging it, is - fixing the memory consumption problem with PARTITION BY. It's an old flaw of the current mechanism. Having this new feature will make it more frequently used, and the users will bump into this problem more frequently.

Is there an issue that describes this issue in depth? I could look into that

[alexey-milovidov]

1. Exporting using s3 table function might create too many threads #51533
2. Deadlock in WriteBufferFromS3::waitForAllBackGroundTasks #48136

[arthurpassos]

needs to be completed

[arthurpassos]

Messed up, need to re-think a couple of things

[arthurpassos]

The shitty part about this is keeping backwards compatibility with {_partition_id}

[arthurpassos]

@alexey-milovidov couple of questions:

1. The existing use_hive_partitioning is used for something else, and it can be tweaked in-between table creation and data insertion. We need a new variable to control the partitioning style at a table level. Should it be a new setting or a new argument to table engines? e.g, partitioning_strategy=['hive' | 'simple', others in the future]. In case we vote for argument, it should be implemented for S3, File and URL table engines.
2. We have settled on asking for the user to specify the table root and we generate the rest (partition style path, filename and file extension). Being that said, should we forbid the user to create a table with {_partition_id} macro in case partition strategy is hive style?

[arthurpassos]

Once I am done with this PR, I'll look into the max threads/streams thing

[kssenii]

@arthurpassos from now on please avoid force pushes in this PR because, after I fixed conflicts in private synchronization, each force push later on requires me to fix it again.

[arthurpassos]

@arthurpassos from now on please avoid force pushes in this PR because, after I fixed conflicts in private synchronization, each force push later on requires me to fix it again.

Ouch, I did not know that. I am sorry. Do you want me to revert it (if possible) and do it without force pushes?

[kssenii]

Ouch, I did not now that. I am sorry.

I understand, no problem.

Do you want me to revert it (if possible)

I am not sure it will work, I will just fix it again then.

[arthurpassos]

@kssenii btw, once CI/CD finishes, I would appreciate if we could merge it. Object storage is a popular file in ClickHouse nowadays and I am having to fix merge conflicts almost daily.

[arthurpassos]

Integration test_dictionaries_all_layouts_separate_sources - #81246

Stateless tests (amd_binary, ParallelReplicas, s3 storage, sequential) - 00002_log_and_exception_messages_formatting - Failed in one of your PRs as well: #84463

Stateless tests (amd_asan, distributed plan, sequential) - 00002_log_and_exception_messages_formatting - Failed in one of your PRs as well: #84463

@kssenii can we merge it?

[kssenii]

CH Inc sync — Failed. Needs manual intervention. See job ID for details:46853436543

I guess the status just did not auto-update for some reason, the sync is fixed, all commits are present and fully green.

[PedroTadim]

Dumb question. I don't understand this error:

CREATE TABLE t0 (c0 Int) ENGINE = AzureBlobStorage(azure, blob_path = 'f0', format = 'CSV', compression = 'none', partition_strategy = 'hive', partition_columns_in_data_file = 1);
/*
DB::Exception: Received from localhost:9000. DB::Exception: Unexpected key `partition_columns_in_data_file` in
named collection. Required keys: container, blob_path, optional keys: storage_account_url, compression_method,
account_name, connection_string, structure, partition_strategy, account_key, compression, format. (BAD_ARGUMENTS)
*/

[arthurpassos]

Dumb question. I don't understand this error:

CREATE TABLE t0 (c0 Int) ENGINE = AzureBlobStorage(azure, blob_path = 'f0', format = 'CSV', compression = 'none', partition_strategy = 'hive', partition_columns_in_data_file = 1);
/*
DB::Exception: Received from localhost:9000. DB::Exception: Unexpected key `partition_columns_in_data_file` in
named collection. Required keys: container, blob_path, optional keys: storage_account_url, compression_method,
account_name, connection_string, structure, partition_strategy, account_key, compression, format. (BAD_ARGUMENTS)
*/

Not dumb, thanks for finding this. I forgot to specify it here and test it as well: https://github.com/ClickHouse/ClickHouse/pull/76802/files#diff-62aeafbb0f3222329276b3f14e00adcba1d258d3f44a71b9bf93e79091c0f06aR59

Fix: #85373