S3 hive style writes #697

arthurpassos · 2025-03-20T15:16:36Z

More info on ClickHouse#76802

Changelog category (leave one):

New Feature

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

Add support for hive partition style writes. Depends on #710 and #711, tests will not pass otherwise

Documentation entry for user-facing changes

src/Storages/ObjectStorage/S3/Configuration.cpp

src/Storages/ObjectStorage/registerStorageObjectStorage.cpp

src/Storages/PartitionStrategy.cpp

arthurpassos · 2025-03-24T17:23:46Z

Depends on #700

…upposed to be

src/Storages/ObjectStorage/StorageObjectStorage.cpp

arthurpassos · 2025-03-26T20:47:27Z

Depends on #700

and writing more tests are the only thing missing I guess

…rt table function

src/Storages/ObjectStorage/S3/Configuration.cpp

altinity-robot · 2025-03-27T21:22:17Z

This is an automated comment for commit be49e03 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ failure
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Regression aarch64 Tiered Storage s3amazon	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ failure
Regression aarch64 Tiered Storage s3gcs	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ failure
Sign aarch64	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ error
Sign release	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ error
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure

Successful checks

Check name	Description	Status
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Ready for release	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Alter attach partition	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Alter move partition	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Alter replace partition	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Benchmark aws_s3	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Benchmark gcs	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Benchmark minio	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Clickhouse Keeper SSL	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 LDAP authentication	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 LDAP external_user_directory	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 LDAP role_mapping	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Parquet aws_s3	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Parquet minio	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Parquet	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 S3 azure	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 S3 gcs	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 S3 minio	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 Tiered Storage minio	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 aes_encryption	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 atomic_insert	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 base_58	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 clickhouse_keeper	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 data_types	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 datetime64_extended_range	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 disk_level_encryption	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 dns	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 engines	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 example	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 extended_precision_data_types	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 kafka	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 kerberos	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 key_value	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 lightweight_delete	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 memory	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 part_moves_between_shards	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 selects	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 session_timezone	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 tiered_storage	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression aarch64 window_functions	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release Alter move partition	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success

src/Storages/ObjectStorage/StorageObjectStorage.cpp

Enmk · 2025-04-02T18:13:40Z

src/Storages/ObjectStorage/StorageObjectStorage.cpp

+
+        bool has_partition_wildcard = configuration->withPartitionWildcard();
+
+        if (has_partition_wildcard && !partitioning_style_to_wildcard_acceptance.at(configuration->partitioning_style))


Please get that value once and save to a meaningfully named variable, because it is not entirely clear what does this bool flag conveys...

You mean something like the below?

bool partitioning_style_supports_wildcard = partitioning_style_to_wildcard_acceptance.at(configuration->partitioning_style) if (has_partition_wildcard && !partitioning_style_supports_wildcard) { ... }

yes, that'll be nice

src/Storages/PartitionStrategy.h

src/Storages/StorageFile.cpp

Enmk · 2025-04-02T22:28:39Z

src/Storages/ObjectStorage/S3/Configuration.cpp

+    "partitioning_style",
+    "write_partition_columns_into_files"


Those two need some documentation with examples...

Enmk · 2025-04-02T22:35:44Z

src/Storages/ObjectStorage/StorageObjectStorage.cpp

+    ASTPtr getPartitionByAst(const ASTPtr & table_level_partition_by, const ASTPtr & query, const StorageObjectStorage::ConfigurationPtr & configuration)
+    {
+        ASTPtr query_partition_by = nullptr;
+        if (const auto insert_query = std::dynamic_pointer_cast<ASTInsertQuery>(query))


why not query->as<ASTInsertQuery>() here?

Enmk · 2025-04-02T22:42:18Z

src/Storages/PartitionStrategy.cpp

+{
+    Names extractPartitionRequiredColumns(ASTPtr & partition_by, const Block & sample_block, ContextPtr context)
+    {
+        auto pby_clone = partition_by->clone();


Why do we need to clone here?

Enmk · 2025-04-02T22:48:11Z

src/Storages/ObjectStorage/S3/Configuration.cpp

@@ -74,7 +127,9 @@ static const std::unordered_set<std::string_view> optional_configuration_keys =
    "max_single_part_upload_size",
    "max_connections",
    "expiration_window_seconds",
-    "no_sign_request"
+    "no_sign_request",
+    "partitioning_style",


I was doubting if "partition_strategy" might be a better name, and then I saw that you have a class PartitionStrategy that represents a thing that is controlled by this setting, and now I am certain.

Enmk · 2025-04-02T23:08:09Z

src/Storages/PartitionStrategy.h

+    ContextPtr context;
+};
+
+struct PartitionStrategyProvider


I would prefer name PartitionStrategyFactory here, the nuances of the name is debatable, but IMO provider is:

more vague, while this particular class just builds an instance based on input arguments.

most often used in CH codebase in different meaning: like a simpler interface to the underlying complex system (metrics provider, columns in dictionaries), or give access to some pre-existing pre-configured component, managed elsewhere.

Also since it returns std::shared_ptr it is not entirely clear if that is a pre-existing object (that already has some state) and implicitly shared between callers, or a new one (which caller may freely modify and do not be afraid of side effects elsewhere)

Enmk · 2025-04-02T23:09:58Z

src/Storages/PartitionedSink.cpp

+    /*
+     * `write_partition_columns_into_files`
+     */
+    const auto chunk = getPartitionStrategy()->getChunkWithoutPartitionColumnsIfNeeded(input_chunk);


why not just partition_strategy here?

Suggested change

const auto chunk = getPartitionStrategy()->getChunkWithoutPartitionColumnsIfNeeded(input_chunk);

const auto chunk = partition_strategy->getChunkWithoutPartitionColumnsIfNeeded(input_chunk);

Enmk · 2025-04-02T23:13:27Z

src/Storages/PartitionStrategy.h

+namespace DB
+{
+
+struct PartitionStrategy


is PartitionStrategy usable with non-object-storage storage?

Enmk · 2025-04-02T23:27:10Z

src/Storages/ObjectStorage/StorageObjectStorage.cpp

 }

 namespace StorageObjectStorageSetting
 {
 extern const StorageObjectStorageSettingsBool allow_dynamic_metadata_for_data_lakes;
 }

+namespace
+{
+    void sanityCheckPartitioningConfiguration(


IMO this whole function (and getPartitionByAst) can be moved inside PartitionStrategyProvider, since those only validate/prepare parameters for PartitionStrategyProvider::get.

arthurpassos · 2025-04-03T13:43:28Z

src/Storages/ObjectStorage/StorageObjectStorage.cpp

+    {
+        if (!table_level_partition_by && !query_partition_by)
+        {
+            // do we want to assert that `partitioning_style` is not set to something different style AND


sounds like we want it

arthurpassos · 2025-04-03T18:07:21Z

src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp

    setVirtuals(VirtualColumnUtils::getVirtualsForFileLikeStorage(metadata.columns, context_, sample_path));
+    setInMemoryMetadata(metadata);


arthurpassos · 2025-04-03T22:34:17Z

src/Storages/PartitionStrategy.cpp

+{
+    std::string path;
+
+    if (!prefix.empty())


I should probably add some logic here to prevent double slashes

This is not a problem in my tests because AWS S3 sdk splits the key into several string using the slash delimiter, but we should be careful

arthurpassos · 2025-04-04T12:54:59Z

For some odd reason linking is failing on some builds:

cmake/libabsl_bad_variant_access.a  contrib/abseil-cpp-cmake/libabsl_raw_logging_internal.a  contrib/abseil-cpp-cmake/libabsl_log_severity.a  rust/workspace/lib_ch_rust_skim_rust.a  contrib/unixodbc-cmake/lib_ltdl.a  -Wl,--start-group  contrib/libcxx-cmake/libcxx.a  contrib/libcxxabi-cmake/libcxxabi.a  contrib/libunwind-cmake/libunwind.a  base/glibc-compatibility/libglibc-compatibility.a  base/glibc-compatibility/memcpy/libmemcpy.a  -Wl,--end-group  -nodefaultlibs /usr/lib/llvm-19/lib/clang/19/lib/linux/libclang_rt.builtins-x86_64.a   -lc -lm -lrt -lpthread -ldl && :
Apr 03 22:12:37 ld.lld-19: error: undefined symbol: DB::generateSnowflakeID()
Apr 03 22:12:37 >>> referenced by PartitionStrategy.cpp:222 (./build_docker/./src/Storages/PartitionStrategy.cpp:222)
Apr 03 22:12:37 >>>               PartitionStrategy.cpp.o:(DB::HiveStylePartitionStrategy::getPath(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)) in archive src/libdbms.a
Apr 03 22:12:37 clang++-19: error: linker command failed with exit code 1 (use -v to see invocation)
Apr 03 22:12:38 [13326/13732] Generating StorageSystemLicenses.generated.cpp
Apr 03 22:38:46 [13327/13732] Building CXX object src/AggregateFunctions/CMakeFiles/clickhouse_aggregate_functions.dir/AggregateFunctionAvgWeighted.cpp.o
Apr 03 22:38:46 sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
Apr 03 22:38:46 ninja: build stopped: subcommand failed.

arthurpassos · 2025-04-04T20:53:39Z

src/Storages/ObjectStorage/StorageObjectStorage.cpp

+    // create table s3_table engine=s3('{_partition_id}'); -- partition id wildcard set, but no partition expression
+    // create table s3_table engine=s3(partition_strategy='hive'); -- partition strategy set, but no partition expression
+    if (partition_by)
+    {


This is somewhat problematic.

PartitionStrategyFactory needs a sample block, so it needs to be after the call to VirtualColumnUtils::getVirtualsForFileLikeStorage(metadata.columns, context, sample_path, format_settings) since it might alter the number of columns. This function needs a sample_path, that is resolved by getPathSample.

On the other hand, PartitionStrategyFactory performs some bucket & key validation which should happen before getPathSample

arthurpassos · 2025-04-04T21:09:03Z

Tests are broken because we are missing some fixes by upstream, the below pr contains the list which I believe to fix those issues:

ClickHouse#71636

Once we have 25.2, it'll be easier.

arthurpassos added 15 commits March 20, 2025 12:00

Add suport for s3 hive partition style writes

152fc42

storageurl and storagefile

9af41a7

simplify code

1aa7db4

reduce changes

4f2ef27

add tests for s3, enforce some rules

301e61e

some refactoring

7d25cce

extern not_implemented

f3f7e9a

copy sample bock

6a75897

focus on engine s3 only, new argument to control partition style

839af35

small adjustments

c8805b5

tests

1926495

comment out test until we figure out what to do with that syntax

33b61d4

fix tests

35d875a

fix

fc546c9

conflicts

f9777a5

arthurpassos commented Mar 21, 2025

View reviewed changes

src/Storages/ObjectStorage/S3/Configuration.cpp Outdated Show resolved Hide resolved

arthurpassos commented Mar 21, 2025

View reviewed changes

src/Storages/ObjectStorage/registerStorageObjectStorage.cpp Outdated Show resolved Hide resolved

arthurpassos commented Mar 21, 2025

View reviewed changes

src/Storages/PartitionStrategy.cpp Outdated Show resolved Hide resolved

do not allow partition_id to be specified in the bucket portion

de868f3

arthurpassos added 4 commits March 24, 2025 16:13

add tests to validate partition id macro isn't used where it is not s…

b4bb1a2

…upposed to be

remove no longer needed test

4622f9e

some silly implementation for formatToFileExtension

3ca53e1

move check for wildcard inside initialize method

82ee821

arthurpassos commented Mar 24, 2025

View reviewed changes

src/Storages/ObjectStorage/StorageObjectStorage.cpp Outdated Show resolved Hide resolved

remove some unrelated changes

d9409cc

add some sanity checks for partitioning configuration, tests for inse…

b1bca06

…rt table function

arthurpassos commented Mar 27, 2025

View reviewed changes

src/Storages/ObjectStorage/S3/Configuration.cpp Outdated Show resolved Hide resolved

Enmk reviewed Apr 2, 2025

View reviewed changes

src/Storages/ObjectStorage/StorageObjectStorage.cpp Outdated Show resolved Hide resolved

Enmk reviewed Apr 2, 2025

View reviewed changes

arthurpassos commented Apr 2, 2025

View reviewed changes

src/Storages/PartitionStrategy.h Show resolved Hide resolved

arthurpassos commented Apr 2, 2025

View reviewed changes

src/Storages/StorageFile.cpp Outdated Show resolved Hide resolved

Enmk reviewed Apr 2, 2025

View reviewed changes

src/Storages/PartitionStrategy.h

namespace DB

{

struct PartitionStrategy

Copy link

Member

Enmk Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is PartitionStrategy usable with non-object-storage storage?

Enmk reviewed Apr 2, 2025

View reviewed changes

arthurpassos commented Apr 3, 2025

View reviewed changes

arthurpassos added 6 commits April 3, 2025 14:25

refactor extractNamedArgumentAndRemoveFromList

043ce22

address a few pr comments

3f93ac0

address some more comments

db9bfd0

simmplify extractPartitionRequiredColumns

0a31b75

rename setting

1dc7f9d

revert some small changes on storagefile

baefb50

arthurpassos commented Apr 3, 2025

View reviewed changes

arthurpassos added 3 commits April 3, 2025 15:08

some intermediate docs

1ac2a9c

add missing test file updt

7e8b4d7

simplify sanity check of partition config

ef99792

arthurpassos commented Apr 3, 2025

View reviewed changes

default partition strategy in factory

be49e03

arthurpassos commented Apr 4, 2025

View reviewed changes

MyroTk added antalya-25.2.2 Planned for 25.2.2 release and removed antalya-25.2.2 Planned for 25.2.2 release labels Apr 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 hive style writes #697

S3 hive style writes #697

arthurpassos commented Mar 20, 2025 •

edited

Loading

arthurpassos commented Mar 24, 2025

arthurpassos commented Mar 26, 2025

altinity-robot commented Mar 27, 2025 •

edited

Loading

Enmk Apr 2, 2025

arthurpassos Apr 2, 2025

Enmk Apr 2, 2025

Enmk Apr 2, 2025

Enmk Apr 2, 2025

Enmk Apr 2, 2025

Enmk Apr 2, 2025 •

edited

Loading

Enmk Apr 2, 2025 •

edited

Loading

Enmk Apr 2, 2025

Enmk Apr 2, 2025

Enmk Apr 2, 2025

arthurpassos Apr 3, 2025

arthurpassos Apr 3, 2025

arthurpassos Apr 3, 2025

arthurpassos Apr 3, 2025

arthurpassos commented Apr 4, 2025

arthurpassos Apr 4, 2025

arthurpassos commented Apr 4, 2025


		bool has_partition_wildcard = configuration->withPartitionWildcard();

		if (has_partition_wildcard && !partitioning_style_to_wildcard_acceptance.at(configuration->partitioning_style))

	const auto chunk = getPartitionStrategy()->getChunkWithoutPartitionColumnsIfNeeded(input_chunk);
	const auto chunk = partition_strategy->getChunkWithoutPartitionColumnsIfNeeded(input_chunk);

		setVirtuals(VirtualColumnUtils::getVirtualsForFileLikeStorage(metadata.columns, context_, sample_path));
		setInMemoryMetadata(metadata);

S3 hive style writes #697

Are you sure you want to change the base?

S3 hive style writes #697

Conversation

arthurpassos commented Mar 20, 2025 • edited Loading

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

arthurpassos commented Mar 24, 2025

arthurpassos commented Mar 26, 2025

altinity-robot commented Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enmk Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Enmk Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurpassos commented Apr 4, 2025

Choose a reason for hiding this comment

arthurpassos commented Apr 4, 2025

arthurpassos commented Mar 20, 2025 •

edited

Loading

altinity-robot commented Mar 27, 2025 •

edited

Loading

Enmk Apr 2, 2025 •

edited

Loading

Enmk Apr 2, 2025 •

edited

Loading