From fdf92dfaecbc5f2eb8505c40bd78cacec4f3ba62 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Tue, 6 Oct 2026 13:31:26 +0200 Subject: [PATCH 1/5] Fix GROUP BY hostName() over s3Cluster and object_storage_cluster The old analyzer drops constant GROUP BY keys when distributed_depth is 0. Cluster table function reads never increased that depth, so workers folded hostName() and returned only the aggregate. https: //github.com/Altinity/ClickHouse/issues/2139 Co-authored-by: Cursor --- src/Storages/IStorageCluster.cpp | 1 + ...296_s3_cluster_hostname_group_by.reference | 3 ++ .../05296_s3_cluster_hostname_group_by.sql | 32 +++++++++++++++++++ 3 files changed, 36 insertions(+) create mode 100644 tests/queries/0_stateless/05296_s3_cluster_hostname_group_by.reference create mode 100644 tests/queries/0_stateless/05296_s3_cluster_hostname_group_by.sql diff --git a/src/Storages/IStorageCluster.cpp b/src/Storages/IStorageCluster.cpp index c66fc0b4a7709..0d68a4a4c1feb 100644 --- a/src/Storages/IStorageCluster.cpp +++ b/src/Storages/IStorageCluster.cpp @@ -974,6 +974,7 @@ ContextPtr ReadFromCluster::updateSettings(const Settings & settings) auto new_context = Context::createCopy(context); new_context->setSettings(new_settings); + new_context->increaseDistributedDepth(); return new_context; } diff --git a/tests/queries/0_stateless/05296_s3_cluster_hostname_group_by.reference b/tests/queries/0_stateless/05296_s3_cluster_hostname_group_by.reference new file mode 100644 index 0000000000000..456b57d5a4e9c --- /dev/null +++ b/tests/queries/0_stateless/05296_s3_cluster_hostname_group_by.reference @@ -0,0 +1,3 @@ +1 10 +1 10 +1 10 diff --git a/tests/queries/0_stateless/05296_s3_cluster_hostname_group_by.sql b/tests/queries/0_stateless/05296_s3_cluster_hostname_group_by.sql new file mode 100644 index 0000000000000..242e9b4b75646 --- /dev/null +++ b/tests/queries/0_stateless/05296_s3_cluster_hostname_group_by.sql @@ -0,0 +1,32 @@ +-- Tags: no-fasttest +-- Tag no-fasttest: Depends on Minio +-- https://github.com/Altinity/ClickHouse/issues/2139 + +INSERT INTO FUNCTION s3('http://localhost:11111/test/05296_s3_cluster_hostname_group_by.tsv', 'TSV', 'x UInt32') +SELECT number FROM numbers(10) +SETTINGS s3_truncate_on_insert = 1; + +SELECT host != '', c FROM +( + SELECT hostName() AS host, count() AS c + FROM s3Cluster('test_cluster_two_shards_localhost', 'http://localhost:11111/test/05296_s3_cluster_hostname_group_by.tsv', 'TSV', 'x UInt32') + GROUP BY host +) +SETTINGS enable_analyzer = 0; + +SELECT host != '', c FROM +( + SELECT hostName() AS host, count() AS c + FROM s3('http://localhost:11111/test/05296_s3_cluster_hostname_group_by.tsv', 'TSV', 'x UInt32') + GROUP BY host + SETTINGS object_storage_cluster = 'test_cluster_two_shards_localhost' +) +SETTINGS enable_analyzer = 0; + +SELECT host != '', c FROM +( + SELECT hostName() AS host, count() AS c + FROM s3Cluster('test_cluster_two_shards_localhost', 'http://localhost:11111/test/05296_s3_cluster_hostname_group_by.tsv', 'TSV', 'x UInt32') + GROUP BY host +) +SETTINGS enable_analyzer = 1; From f2f790e1fc3d3b864e16495ffd3e9672e90ed95b Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Tue, 6 Oct 2026 14:03:54 +0200 Subject: [PATCH 2/5] Keep server-constant GROUP BY keys without raising distributed_depth Raising distributed_depth on cluster reads also changes parallel replicas, max_distributed_depth, and block marshalling. The old analyzer now keeps a key such as hostName() and still drops ordinary constants. https: //github.com/Altinity/ClickHouse/issues/2139 Co-authored-by: Cursor --- src/Interpreters/ExpressionAnalyzer.cpp | 37 +++++++++++++++++++++++-- src/Storages/IStorageCluster.cpp | 1 - 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index 27359b9199bbf..5c47e12b80e73 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -1,3 +1,5 @@ +#include + #include #include @@ -140,6 +142,35 @@ bool allowEarlyConstantFolding(const ActionsDAG & actions, const Settings & sett return true; } +/// A folded server constant (`hostName()`, `tcpPort()`, ...) must stay a GROUP BY key. +/// Its value differs per shard. Dropping it makes the initiator look up a missing column. +bool containsServerConstant(const ActionsDAG::Node * node) +{ + std::stack stack; + if (node) + stack.push(node); + + while (!stack.empty()) + { + const auto * current = stack.top(); + stack.pop(); + + while (current && current->type == ActionsDAG::ActionType::ALIAS) + current = current->children.empty() ? nullptr : current->children.front(); + + if (!current) + continue; + + if (current->function_base && current->function_base->isServerConstant()) + return true; + + for (const auto * child : current->children) + stack.push(child); + } + + return false; +} + LoggerPtr getLogger() { return ::getLogger("ExpressionAnalyzer"); } } @@ -350,7 +381,8 @@ void ExpressionAnalyzer::analyzeAggregation(ActionsDAG & temp_actions) if (getContext()->getClientInfo().distributed_depth == 0 || settings.distributed_group_by_no_merge > 0) { /// Constant expressions have non-null column pointer at this stage. - if (node->column) + /// Server constants stay: they are different on each shard. + if (node->column && !containsServerConstant(node)) { select_query->group_by_with_constant_keys = true; @@ -404,7 +436,8 @@ void ExpressionAnalyzer::analyzeAggregation(ActionsDAG & temp_actions) if (getContext()->getClientInfo().distributed_depth == 0 || settings.distributed_group_by_no_merge > 0) { /// Constant expressions have non-null column pointer at this stage. - if (node->column) + /// Server constants stay: they are different on each shard. + if (node->column && !containsServerConstant(node)) { select_query->group_by_with_constant_keys = true; diff --git a/src/Storages/IStorageCluster.cpp b/src/Storages/IStorageCluster.cpp index 0d68a4a4c1feb..c66fc0b4a7709 100644 --- a/src/Storages/IStorageCluster.cpp +++ b/src/Storages/IStorageCluster.cpp @@ -974,7 +974,6 @@ ContextPtr ReadFromCluster::updateSettings(const Settings & settings) auto new_context = Context::createCopy(context); new_context->setSettings(new_settings); - new_context->increaseDistributedDepth(); return new_context; } From f0cdc5421bdd49f07e1afabab1ccdbbdd37d4291 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Tue, 6 Oct 2026 18:06:29 +0200 Subject: [PATCH 3/5] Drop __getScalar GROUP BY keys in the old analyzer __getScalar is marked as a server constant, but the initiator evaluates it and sends the same value to every shard. Keeping it made GROUP BY of a scalar subquery with Dynamic throw ILLEGAL_COLUMN. https: //github.com/Altinity/ClickHouse/pull/2490 Co-authored-by: Cursor --- src/Interpreters/ExpressionAnalyzer.cpp | 4 +++- .../05297_scalar_subquery_group_by_old_analyzer.reference | 2 ++ .../05297_scalar_subquery_group_by_old_analyzer.sql | 7 +++++++ 3 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 tests/queries/0_stateless/05297_scalar_subquery_group_by_old_analyzer.reference create mode 100644 tests/queries/0_stateless/05297_scalar_subquery_group_by_old_analyzer.sql diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index 5c47e12b80e73..1b66d92fe9618 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -161,7 +161,9 @@ bool containsServerConstant(const ActionsDAG::Node * node) if (!current) continue; - if (current->function_base && current->function_base->isServerConstant()) + /// `__getScalar` is marked as a server constant, but the initiator evaluates it + /// and sends the same value to every shard, so it does not have to stay a key. + if (current->function_base && current->function_base->isServerConstant() && current->function_base->getName() != "__getScalar") return true; for (const auto * child : current->children) diff --git a/tests/queries/0_stateless/05297_scalar_subquery_group_by_old_analyzer.reference b/tests/queries/0_stateless/05297_scalar_subquery_group_by_old_analyzer.reference new file mode 100644 index 0000000000000..a5c8806279fa7 --- /dev/null +++ b/tests/queries/0_stateless/05297_scalar_subquery_group_by_old_analyzer.reference @@ -0,0 +1,2 @@ +3 +3 diff --git a/tests/queries/0_stateless/05297_scalar_subquery_group_by_old_analyzer.sql b/tests/queries/0_stateless/05297_scalar_subquery_group_by_old_analyzer.sql new file mode 100644 index 0000000000000..b495d5f16573b --- /dev/null +++ b/tests/queries/0_stateless/05297_scalar_subquery_group_by_old_analyzer.sql @@ -0,0 +1,7 @@ +-- A scalar subquery is evaluated once and sent to every shard as `__getScalar`. +-- It must not stay a GROUP BY key just because that function is marked as a server constant. +-- https://github.com/Altinity/ClickHouse/pull/2490 + +SELECT count() FROM numbers(3) GROUP BY (SELECT [1, 2]) SETTINGS enable_analyzer = 0; + +SELECT count() FROM numbers(3) GROUP BY (SELECT [CAST(1, 'Dynamic')]) SETTINGS enable_analyzer = 0; From de846f87651f582609e740036d2a9ba9db179950 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Fri, 9 Oct 2026 17:43:48 +0200 Subject: [PATCH 4/5] Drop local server-constant GROUP BY keys in the old analyzer A local query has one server, so a folded server constant is an ordinary constant. Keeping it made GROUP BY of `hostName` cast to `Dynamic` throw `ILLEGAL_COLUMN`. Secondary queries still keep the key, because cluster table function workers also have `distributed_depth` 0. https: //github.com/Altinity/ClickHouse/pull/2490 Co-authored-by: Cursor --- src/Interpreters/ExpressionAnalyzer.cpp | 17 +++++++++++------ ...ver_constant_group_by_old_analyzer.reference | 3 +++ ...al_server_constant_group_by_old_analyzer.sql | 9 +++++++++ 3 files changed, 23 insertions(+), 6 deletions(-) create mode 100644 tests/queries/0_stateless/05298_local_server_constant_group_by_old_analyzer.reference create mode 100644 tests/queries/0_stateless/05298_local_server_constant_group_by_old_analyzer.sql diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index 1b66d92fe9618..365a0cd5ca644 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -142,8 +142,8 @@ bool allowEarlyConstantFolding(const ActionsDAG & actions, const Settings & sett return true; } -/// A folded server constant (`hostName()`, `tcpPort()`, ...) must stay a GROUP BY key. -/// Its value differs per shard. Dropping it makes the initiator look up a missing column. +/// True when `node` folds a server constant such as `hostName` or `tcpPort`. +/// `__getScalar` is excluded: the initiator evaluates it and sends one value to every shard. bool containsServerConstant(const ActionsDAG::Node * node) { std::stack stack; @@ -347,6 +347,9 @@ void ExpressionAnalyzer::analyzeAggregation(ActionsDAG & temp_actions) else group_by_kind = GroupByKind::ORDINARY; bool use_nulls = group_by_kind != GroupByKind::ORDINARY && getContext()->getSettingsRef()[Setting::group_by_use_nulls]; + /// Cluster table function workers leave `distributed_depth` at 0, same as a local query. + /// `SECONDARY_QUERY` is what distinguishes them: a server constant differs per shard only there. + const bool is_secondary_query = getContext()->getClientInfo().query_kind == ClientInfo::QueryKind::SECONDARY_QUERY; /// For GROUPING SETS with multiple groups we always add virtual __grouping_set column /// With set number, which is used as an additional key at the stage of merging aggregating data. @@ -383,8 +386,9 @@ void ExpressionAnalyzer::analyzeAggregation(ActionsDAG & temp_actions) if (getContext()->getClientInfo().distributed_depth == 0 || settings.distributed_group_by_no_merge > 0) { /// Constant expressions have non-null column pointer at this stage. - /// Server constants stay: they are different on each shard. - if (node->column && !containsServerConstant(node)) + /// A server constant stays only on a secondary query, where each shard has its own value. + /// A local query has one value, so the key is removed with other constants. + if (node->column && !(is_secondary_query && containsServerConstant(node))) { select_query->group_by_with_constant_keys = true; @@ -438,8 +442,9 @@ void ExpressionAnalyzer::analyzeAggregation(ActionsDAG & temp_actions) if (getContext()->getClientInfo().distributed_depth == 0 || settings.distributed_group_by_no_merge > 0) { /// Constant expressions have non-null column pointer at this stage. - /// Server constants stay: they are different on each shard. - if (node->column && !containsServerConstant(node)) + /// A server constant stays only on a secondary query, where each shard has its own value. + /// A local query has one value, so the key is removed with other constants. + if (node->column && !(is_secondary_query && containsServerConstant(node))) { select_query->group_by_with_constant_keys = true; diff --git a/tests/queries/0_stateless/05298_local_server_constant_group_by_old_analyzer.reference b/tests/queries/0_stateless/05298_local_server_constant_group_by_old_analyzer.reference new file mode 100644 index 0000000000000..1f242fa6f0004 --- /dev/null +++ b/tests/queries/0_stateless/05298_local_server_constant_group_by_old_analyzer.reference @@ -0,0 +1,3 @@ +3 +3 +3 diff --git a/tests/queries/0_stateless/05298_local_server_constant_group_by_old_analyzer.sql b/tests/queries/0_stateless/05298_local_server_constant_group_by_old_analyzer.sql new file mode 100644 index 0000000000000..8109ac535241d --- /dev/null +++ b/tests/queries/0_stateless/05298_local_server_constant_group_by_old_analyzer.sql @@ -0,0 +1,9 @@ +-- A local query has one server, so a folded server constant is an ordinary GROUP BY constant. +-- Keeping it makes `Dynamic` fail group-key validation. +-- https://github.com/Altinity/ClickHouse/pull/2490 + +SELECT count() FROM numbers(3) GROUP BY hostName() SETTINGS enable_analyzer = 0; + +SELECT count() FROM numbers(3) GROUP BY CAST(hostName(), 'Dynamic') SETTINGS enable_analyzer = 0; + +SELECT count() FROM numbers(3) GROUP BY [CAST(hostName(), 'Dynamic')] SETTINGS enable_analyzer = 0; From 44cc3cbe9a7fdb3d8bcd25dfdf2117d51fbc7afc Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Thu, 15 Oct 2026 17:16:21 +0200 Subject: [PATCH 5/5] Pass `ExternalStorageCache` into Iceberg DROP PARTITION metadata reads `getManifestFileEntriesHandle` and `generateManifestList` take the cache after external-path support, but `AlterDropPartitionExecutor` still used the old signatures, so the amd binary build failed. https: //altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2575&sha=de846f87651f582609e740036d2a9ba9db179950&name_0=PR&name_1=Build%20%28amd_binary%29 https: //github.com/Altinity/ClickHouse/pull/2575 Co-authored-by: Cursor --- .../DataLakes/Iceberg/AlterDropPartitionExecutor.cpp | 9 +++++++-- .../DataLakes/Iceberg/AlterDropPartitionExecutor.h | 3 +++ .../ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp | 1 + 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/AlterDropPartitionExecutor.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/AlterDropPartitionExecutor.cpp index af38b06ed41aa..faf3b30e2a176 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/AlterDropPartitionExecutor.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/AlterDropPartitionExecutor.cpp @@ -215,6 +215,7 @@ AlterDropPartitionExecutor::AlterDropPartitionExecutor( const IcebergMetadata & metadata_, ContextPtr context_, ObjectStoragePtr object_storage_, + ExternalStorageCache & external_storages_, const PersistentTableComponents & components_, const DataLakeStorageSettings & data_lake_settings_, String write_format_, @@ -223,6 +224,7 @@ AlterDropPartitionExecutor::AlterDropPartitionExecutor( , metadata(metadata_) , context(context_) , object_storage(std::move(object_storage_)) + , external_storages(external_storages_) , components(components_) , data_lake_settings(data_lake_settings_) , write_format(std::move(write_format_)) @@ -365,7 +367,8 @@ AlterDropPartitionExecutor::discoverTargetFilePaths(const SnapshotState & state, for (const auto & manifest_key : state.snapshot->manifest_list_entries) { - auto handle = getManifestFileEntriesHandle(object_storage, components, context, log, manifest_key, state.schema_id); + auto handle = getManifestFileEntriesHandle( + object_storage, components, context, log, manifest_key, state.schema_id, external_storages); collect(handle.getFilesWithoutDeleted(FileContentType::DATA)); collect(handle.getFilesWithoutDeleted(FileContentType::POSITION_DELETE)); @@ -460,7 +463,8 @@ AlterDropPartitionExecutor::buildDropPlan(const SnapshotState & state, const Tar for (const auto & manifest_key : state.snapshot->manifest_list_entries) { - auto handle = getManifestFileEntriesHandle(object_storage, components, context, log, manifest_key, state.schema_id); + auto handle = getManifestFileEntriesHandle( + object_storage, components, context, log, manifest_key, state.schema_id, external_storages); size_t entries_to_keep = 0; size_t entries_to_remove = 0; @@ -575,6 +579,7 @@ AlterDropPartitionExecutor::ManifestListWriteResult AlterDropPartitionExecutor:: components.path_resolver, state.metadata_object, object_storage, + external_storages, context, /*manifest_entry_names=*/{}, new_snapshot, diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/AlterDropPartitionExecutor.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/AlterDropPartitionExecutor.h index de47a35ab9ec4..a462b94962a57 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/AlterDropPartitionExecutor.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/AlterDropPartitionExecutor.h @@ -30,6 +30,7 @@ namespace DB class FileNamesGenerator; class IcebergMetadata; +struct ExternalStorageCache; namespace Iceberg { @@ -42,6 +43,7 @@ class AlterDropPartitionExecutor const IcebergMetadata & metadata_, ContextPtr context_, ObjectStoragePtr object_storage_, + ExternalStorageCache & external_storages_, const PersistentTableComponents & components_, const DataLakeStorageSettings & data_lake_settings_, String write_format_, @@ -106,6 +108,7 @@ class AlterDropPartitionExecutor const IcebergMetadata & metadata; ContextPtr context; ObjectStoragePtr object_storage; + ExternalStorageCache & external_storages; const PersistentTableComponents & components; const DataLakeStorageSettings & data_lake_settings; String write_format; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index 2eb3feca93452..b892c9b105ae7 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -973,6 +973,7 @@ void IcebergMetadata::alterPartitionDropImpl(const PartitionCommand & command, C *this, context, object_storage, + *external_storages, persistent_components, data_lake_settings, write_format,