diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index 27359b9199bbf..365a0cd5ca644 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -1,3 +1,5 @@ +#include + #include #include @@ -140,6 +142,37 @@ bool allowEarlyConstantFolding(const ActionsDAG & actions, const Settings & sett return true; } +/// 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; + 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; + + /// `__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) + stack.push(child); + } + + return false; +} + LoggerPtr getLogger() { return ::getLogger("ExpressionAnalyzer"); } } @@ -314,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. @@ -350,7 +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. - if (node->column) + /// 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; @@ -404,7 +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. - if (node->column) + /// 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/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, 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; 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; 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;