From f777f8df664a2c830fda607b11f48f5b937cdd6a Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Wed, 16 Sep 2026 15:33:49 +0530 Subject: [PATCH 01/15] Add `object_storage_cluster_join_mode='distributed'` for whole-query JOIN dispatch When a JOIN's driving table is a `DataLake` catalog table distributed via `object_storage_cluster`, send the whole query to that table's cluster and merge the partial aggregates on the initiator, instead of reading every table back to the initiator and joining there. The JOIN and any `GROUP BY` then run on every node of the cluster rather than on one. `findDistributedObjectStorageCandidate` decides eligibility. It walks down the left side of the join tree to find the driving table, passing through a subquery only when that subquery does not itself aggregate, deduplicate, sort or limit -- each worker runs it against its own slice of the driver, so anything that finalizes across rows would turn a partial result into a final one. Every other table reachable in the query must resolve through a `DataLake` catalog, carry no row policy, and be readable by the current user. `buildDistributedObjectStorageQueryPlan` then replaces the driver with an explicit `*Cluster()` table function, keyed on the exact query tree node, and reads the result back at `WithMergeableState` through a single `ReadFromCluster` step, so the planner's ordinary finalization applies on top. It follows `buildQueryPlanForParallelReplicas` step for step, including the position-based conversion from the rewritten query's header back to the original's. Only the driving table is partitioned across the cluster; every other table in the query is read and recomputed in full on each node. That cost is not estimated when deciding to dispatch. Co-Authored-By: Claude Sonnet 5 Signed-off-by: VighneshPath --- src/Core/Settings.cpp | 1 + src/Core/SettingsEnums.cpp | 7 +- src/Core/SettingsEnums.h | 3 +- src/Databases/DataLake/DatabaseDataLake.cpp | 15 + src/Planner/Planner.cpp | 20 +- ...buildDistributedObjectStorageQueryPlan.cpp | 140 +++++ .../buildDistributedObjectStorageQueryPlan.h | 28 + .../findDistributedObjectStorageCandidate.cpp | 268 +++++++++ .../findDistributedObjectStorageCandidate.h | 50 ++ ...stributed_object_storage_join_dispatch.cpp | 549 +++++++++++++++++ ...d_distributed_object_storage_candidate.cpp | 563 ++++++++++++++++++ src/Storages/IStorageCluster.cpp | 175 +++++- src/Storages/IStorageCluster.h | 41 +- .../StorageObjectStorageCluster.h | 6 + 14 files changed, 1857 insertions(+), 9 deletions(-) create mode 100644 src/Planner/buildDistributedObjectStorageQueryPlan.cpp create mode 100644 src/Planner/buildDistributedObjectStorageQueryPlan.h create mode 100644 src/Planner/findDistributedObjectStorageCandidate.cpp create mode 100644 src/Planner/findDistributedObjectStorageCandidate.h create mode 100644 src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp create mode 100644 src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 5d0f3cc34cc2..324de519fd2b 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -2138,6 +2138,7 @@ Possible values: - `local` — Replaces the database and table in the subquery with local ones for the destination server (shard), leaving the normal `IN`/`JOIN.` - `global` — Replaces the `IN`/`JOIN` query with `GLOBAL IN`/`GLOBAL JOIN.` Right table executes first and is added to the secondary query as temporay table. - `allow` — Default value. Allows the use of these types of subqueries. +- `distributed` — Experimental. Lets a `JOIN`'s leftmost table become the whole query's driver when it is a DataLake-catalog table distributed via `object_storage_cluster`, even when it sits inside a subquery or CTE: the highest enclosing query whose entire `JOIN`/subquery tree is safe (any `GROUP BY` on top included) is dispatched to that cluster's nodes and executed there, instead of pulling the driver's rows back to the initiator first. This only takes effect when every other table reachable in that tree also resolves through some DataLake catalog, has no row-level security policy of its own, and the current user has `SELECT` access to it (an explicit `*Cluster()` table function, an ordinary local/`Distributed` table, or any table failing one of those checks, falls back to ordinary (non-distributed) planning for the level it appears at); ClickHouse does not verify that such a table is configured identically on every node of the driver's cluster — that consistency is the deployment's responsibility. Falls back to ordinary planning entirely when `additional_table_filters` is set, or when `object_storage_remote_initiator` is enabled. )", 0) \ \ DECLARE(UInt64, max_concurrent_queries_for_all_users, 0, R"( diff --git a/src/Core/SettingsEnums.cpp b/src/Core/SettingsEnums.cpp index a5bd46940a30..291acca4be8a 100644 --- a/src/Core/SettingsEnums.cpp +++ b/src/Core/SettingsEnums.cpp @@ -100,9 +100,10 @@ IMPLEMENT_SETTING_ENUM(DistributedProductMode, ErrorCodes::UNKNOWN_DISTRIBUTED_P {"allow", DistributedProductMode::ALLOW}}) IMPLEMENT_SETTING_ENUM(ObjectStorageClusterJoinMode, ErrorCodes::BAD_ARGUMENTS, - {{"local", ObjectStorageClusterJoinMode::LOCAL}, - {"global", ObjectStorageClusterJoinMode::GLOBAL}, - {"allow", ObjectStorageClusterJoinMode::ALLOW}}) + {{"local", ObjectStorageClusterJoinMode::LOCAL}, + {"global", ObjectStorageClusterJoinMode::GLOBAL}, + {"allow", ObjectStorageClusterJoinMode::ALLOW}, + {"distributed", ObjectStorageClusterJoinMode::DISTRIBUTED}}) IMPLEMENT_SETTING_ENUM(QueryResultCacheNondeterministicFunctionHandling, ErrorCodes::BAD_ARGUMENTS, diff --git a/src/Core/SettingsEnums.h b/src/Core/SettingsEnums.h index ebf65cc5b39f..3b77a06b39bb 100644 --- a/src/Core/SettingsEnums.h +++ b/src/Core/SettingsEnums.h @@ -170,7 +170,8 @@ enum class ObjectStorageClusterJoinMode : uint8_t { LOCAL, /// Convert to local query GLOBAL, /// Convert to global query - ALLOW /// Enable + ALLOW, /// Enable + DISTRIBUTED /// Let a distributed object-storage driver own the whole JOIN and run it on cluster workers }; DECLARE_SETTING_ENUM(ObjectStorageClusterJoinMode) diff --git a/src/Databases/DataLake/DatabaseDataLake.cpp b/src/Databases/DataLake/DatabaseDataLake.cpp index ba2fc89c7799..4ab46979edd9 100644 --- a/src/Databases/DataLake/DatabaseDataLake.cpp +++ b/src/Databases/DataLake/DatabaseDataLake.cpp @@ -107,6 +107,7 @@ namespace Setting extern const SettingsBool parallel_replicas_for_cluster_engines; extern const SettingsString cluster_for_parallel_replicas; extern const SettingsBool database_datalake_require_metadata_access; + extern const SettingsObjectStorageClusterJoinMode object_storage_cluster_join_mode; } @@ -751,6 +752,18 @@ StoragePtr DatabaseDataLake::tryGetTableImpl(const String & name, ContextPtr con if (cluster_name.empty() && can_use_parallel_replicas && !is_secondary_query) cluster_name = parallel_replicas_cluster_name; + /// Under object_storage_cluster_join_mode='distributed', a co-resolved DataLake table must localize + /// rather than dispatch its own ReadFromCluster (the driver itself is rewritten separately into an + /// explicit `*Cluster()` call before being sent to workers -- see findDistributedObjectStorageCandidate.h). + /// query_kind == SECONDARY_QUERY alone is too wide (also true for an unrelated Distributed/remote() query), + /// so mirror TableFunctionObjectStorageCluster's own worker-detection signal instead. + const auto & client_info = context_->getClientInfo(); + const bool is_distributed_object_storage_worker + = is_secondary_query && client_info.collaborate_with_initiator && context_->hasClusterFunctionReadTaskCallback(); + + if (is_distributed_object_storage_worker && query_settings[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) + cluster_name.clear(); + auto storage_cluster = std::make_shared( cluster_name, configuration, @@ -775,6 +788,8 @@ StoragePtr DatabaseDataLake::tryGetTableImpl(const String & name, ContextPtr con if (context_->hasQueryContext() && context_->getSettingsRef()[Setting::log_queries]) context_->getQueryContext()->addQueryFactoriesInfo(Context::QueryLogFactories::Storage, storage_cluster->getName()); + storage_cluster->markResolvedViaDataLakeCatalog(); + storage_cluster->startup(); return storage_cluster; } diff --git a/src/Planner/Planner.cpp b/src/Planner/Planner.cpp index faf2e42ea909..4f410851368f 100644 --- a/src/Planner/Planner.cpp +++ b/src/Planner/Planner.cpp @@ -82,6 +82,8 @@ #include #include #include +#include +#include #include #include #include @@ -2302,7 +2304,23 @@ void Planner::buildPlanForQueryNode() } JoinTreeQueryPlan join_tree_query_plan; - if (planner_context->getMutableQueryContext()->canUseTaskBasedParallelReplicas() + /// object_storage_cluster_join_mode='distributed': only the outermost, initial-query Planner instance may + /// claim this optimization -- a nested Planner created for a subquery/CTE (select_query_options.is_subquery) + /// or a plain analysis pass (only_analyze) never does, and neither does a secondary-query Planner running on + /// a worker. findDistributedObjectStorageCandidate() itself never recurses into a narrower candidate, so + /// this is a whole-or-nothing decision for the outermost query alone: if it's not safe, ordinary planning + /// (including its own nested-Planner recursion for any subquery, see PlannerJoinTree.cpp's + /// buildQueryPlanForTableExpression()) handles the whole query. Independent of parallel replicas below. + std::optional distributed_object_storage_candidate; + if (!select_query_options.only_analyze && !select_query_options.is_subquery + && query_context->getClientInfo().query_kind == ClientInfo::QueryKind::INITIAL_QUERY) + distributed_object_storage_candidate = findDistributedObjectStorageCandidate(query_tree, query_context); + + if (distributed_object_storage_candidate) + { + join_tree_query_plan = buildDistributedObjectStorageQueryPlan(query_tree, *distributed_object_storage_candidate, select_query_info, planner_context); + } + else if (planner_context->getMutableQueryContext()->canUseTaskBasedParallelReplicas() && planner_context->getGlobalPlannerContext()->parallel_replicas_node == &query_node) { join_tree_query_plan = buildQueryPlanForParallelReplicas(query_node, planner_context, select_query_info.storage_limits); diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp new file mode 100644 index 000000000000..b2aecf6af810 --- /dev/null +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp @@ -0,0 +1,140 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace DB +{ +namespace ErrorCodes +{ + extern const int LOGICAL_ERROR; +} + +JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( + const QueryTreeNodePtr & dispatch_boundary_node, + const DistributedObjectStorageCandidate & candidate, + const SelectQueryInfo & select_query_info, + const PlannerContextPtr & planner_context) +{ + const auto context = planner_context->getQueryContext(); + constexpr auto processed_stage = QueryProcessingStage::WithMergeableState; + + /// The header stock (unmodified) planning would have produced, computed against the query tree before + /// the driver is rewritten -- so downstream code (the caller's own finalization) sees exactly the column + /// names/types it would have without this optimization, matching buildQueryPlanForParallelReplicas()'s own + /// original-vs-worker header handling. + auto initial_header = InterpreterSelectQueryAnalyzer::getSampleBlock( + dispatch_boundary_node->clone(), context, SelectQueryOptions(processed_stage).analyze()); + + /// Reuses the exact snapshot the analyzer resolved the driver against (TableNode owns it), rather than + /// fetching a fresh one here: metadata could otherwise have changed between analysis and dispatch, leaving + /// the dispatched query resolved against one snapshot and the replacement built from another. + const auto & driver_storage_snapshot = candidate.driver->getStorageSnapshot(); + + auto cluster_function_ast = candidate.driver_storage->buildClusterTableFunctionAST( + candidate.driver_storage->getClusterName(context), driver_storage_snapshot, context); + + auto cluster_function_query_tree = buildQueryTree(cluster_function_ast, context); + auto & cluster_function_node = cluster_function_query_tree->as(); + + auto replacement = std::make_shared(cluster_function_node.getFunctionName()); + replacement->getArgumentsNode() = cluster_function_node.getArgumentsNode(); + replacement->setSettingsChanges(cluster_function_node.getSettingsChanges()); + if (candidate.driver->hasTableExpressionModifiers()) + replacement->setTableExpressionModifiers(*candidate.driver->getTableExpressionModifiers()); + replacement->setAlias(candidate.driver->getAlias()); + + { + QueryAnalysisPass query_analysis_pass; + QueryTreeNodePtr node = replacement; + query_analysis_pass.run(node, context); + } + + /// candidate.driver's own subtree is not traversed further -- it becomes a leaf, exact-node replacement, + /// mirroring StorageDistributed::buildQueryTreeDistributed()'s own pattern. cloneAndReplace() rebinds every + /// weak reference (e.g. ColumnNode source pointers) elsewhere in the tree from the old node to + /// `replacement`. + IQueryTreeNode::ReplacementMap replacement_map; + replacement_map.emplace(candidate.driver, replacement); + auto modified_query_tree = dispatch_boundary_node->cloneAndReplace(replacement_map); + + auto [remote_header, new_planner_context] = InterpreterSelectQueryAnalyzer::getSampleBlockAndPlannerContext( + modified_query_tree, context, SelectQueryOptions(processed_stage).analyze()); + + /// Convert grouping function specializations (e.g. groupingForGroupingSets -> grouping) in a separate + /// clone so the AST sent to the driver's cluster contains the generic function name that can be + /// re-resolved by each worker's own analyzer -- modified_query_tree itself must keep the specialized + /// functions, since it was already used above for header computation and its planner context. + auto query_tree_for_ast = modified_query_tree->clone(); + removeGroupingFunctionSpecializations(query_tree_for_ast); + ASTPtr query_to_send = queryNodeToDistributedSelectQuery(query_tree_for_ast); + + if (!query_to_send->as()) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Distributed object-storage dispatch: expected a plain SELECT at the dispatch boundary"); + + /// SourceStepWithFilter::required_source_columns (and thus updatePrewhereInfo()'s own + /// driver_storage_snapshot->getSampleBlockForColumns(required_source_columns) lookup) is checked against + /// storage_snapshot, i.e. the driver's own snapshot here -- not the whole dispatched query's output schema + /// (that's remote_header, a separate concept). Use the driver's own physical columns, matching what a + /// normal per-table read() of the driver alone would pass. + Names column_names = driver_storage_snapshot->getColumns(GetColumnsOptions(GetColumnsOptions::AllPhysical)).getNames(); + + SelectQueryInfo query_info = select_query_info; + query_info.query = query_to_send; + query_info.query_tree = modified_query_tree; + query_info.planner_context = new_planner_context; + + JoinTreeQueryPlan result; + result.stage = processed_stage; + + candidate.driver_storage->readPreparedClusterQuery( + result.query_plan, + column_names, + driver_storage_snapshot, + query_info, + context, + processed_stage, + query_to_send, + remote_header); + + /// The remote result's header uses whatever column naming the rewritten/re-analyzed query produced; + /// convert it back, by position, to the header the unmodified query would have produced -- e.g. an + /// aggregate like sum() is still AggregateFunction(sum, ...) at this stage, not its finalized type, + /// matching buildQueryPlanForParallelReplicas()'s own original-vs-worker header conversion. Generic and + /// position-based rather than the previous per-projection-node ColumnNode renaming, which broke down for + /// a complex projection mixing CASE expressions over both JOIN sides with an aggregate. + auto converting_actions = ActionsDAG::makeConvertingActions( + result.query_plan.getCurrentHeader()->getColumnsWithTypeAndName(), + initial_header->getColumnsWithTypeAndName(), + ActionsDAG::MatchColumnsMode::Position, + context, + false, + false, + nullptr); + + auto converting_step = std::make_unique(result.query_plan.getCurrentHeader(), std::move(converting_actions)); + converting_step->setStepDescription("Convert columns to the original query's header"); + result.query_plan.addStep(std::move(converting_step)); + + return result; +} + +} diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.h b/src/Planner/buildDistributedObjectStorageQueryPlan.h new file mode 100644 index 000000000000..20884a5bfe1c --- /dev/null +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.h @@ -0,0 +1,28 @@ +#pragma once + +#include +#include + +namespace DB +{ + +class PlannerContext; +using PlannerContextPtr = std::shared_ptr; +struct SelectQueryInfo; + +/// Builds the whole-query dispatch plan for `candidate`: replaces `candidate.driver` in `dispatch_boundary_node` +/// (the exact QueryTreeNodePtr findDistributedObjectStorageCandidate() was called with) with a resolved, +/// explicit `*Cluster()` TableFunctionNode via IQueryTreeNode::cloneAndReplace() -- mirroring +/// StorageDistributed::buildQueryTreeDistributed()'s own exact-node replacement pattern -- serializes the +/// result, and executes it via IStorageCluster::readPreparedClusterQuery(): a single ReadFromCluster step at +/// WithMergeableState, so the caller's normal finalization applies unmodified on top (see +/// Planner::buildPlanForQueryNode()). A generic, position-based ActionsDAG::makeConvertingActions() converts +/// the remote result back to the header the unmodified `dispatch_boundary_node` would have produced, matching +/// buildQueryPlanForParallelReplicas()'s own original-vs-worker header handling. +JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( + const QueryTreeNodePtr & dispatch_boundary_node, + const DistributedObjectStorageCandidate & candidate, + const SelectQueryInfo & select_query_info, + const PlannerContextPtr & planner_context); + +} diff --git a/src/Planner/findDistributedObjectStorageCandidate.cpp b/src/Planner/findDistributedObjectStorageCandidate.cpp new file mode 100644 index 000000000000..80e644c44d3b --- /dev/null +++ b/src/Planner/findDistributedObjectStorageCandidate.cpp @@ -0,0 +1,268 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace DB +{ +namespace Setting +{ + extern const SettingsObjectStorageClusterJoinMode object_storage_cluster_join_mode; + extern const SettingsBool object_storage_remote_initiator; + extern const SettingsMap additional_table_filters; +} + +namespace +{ + +/// A row-level security filter is normally attached to a table's own SelectQueryInfo during per-table +/// planning (PlannerJoinTree.cpp), keyed by that table's own catalog identity; every table admitted here, +/// driver or partner, is either rewritten away (the driver, into its explicit `*Cluster()` form) or +/// independently re-resolved by each worker's own DatabaseDataLake lookup (a partner) -- neither preserves or +/// safely re-derives the initiator user's own effective policy. Conservative: a nontrivial policy on any table +/// in the candidate subtree blocks this whole-query dispatch outright (see the setting's own documentation). +bool hasEffectiveRowPolicy(const TableNode & table_node, const ContextPtr & context) +{ + const auto & storage_id = table_node.getStorageID(); + if (!storage_id.hasDatabase()) + return false; + + auto row_policy_filter = context->getRowPolicyFilter(storage_id.getDatabaseName(), storage_id.getTableName(), RowPolicyFilterType::SELECT_FILTER); + return row_policy_filter && !row_policy_filter->isAlwaysTrue(); +} + +/// Stock per-table planning (prepareBuildQueryPlanForTableExpression() in PlannerJoinTree.cpp) checks SELECT +/// access on every TableNode it plans, including ones buried in a subquery reached only under only_analyze via +/// its own separate check_subquery_table_access path. This whole-query dispatch replaces that per-table walk +/// entirely, so every table it admits -- driver or partner, at any depth -- needs the same check performed +/// here instead. A conservative, non-throwing, table-level (not column-level) check: missing access simply +/// falls back to ordinary planning, which enforces the real, precise access rules with its own error. +bool hasSelectAccess(const TableNode & table_node, const ContextPtr & context) +{ + const auto & storage_id = table_node.getStorageID(); + if (!storage_id.hasDatabase()) + return false; + + return context->getAccess()->isGranted(AccessType::SELECT, storage_id.getDatabaseName(), storage_id.getTableName()); +} + +/// Whether `table_node` is trustworthy to include in the dispatch at all, as either the driver or a JOIN +/// partner / WHERE-HAVING-projection reference: a DataLake-catalog table (so every worker can independently +/// and identically re-resolve it -- see the setting's own documentation for what is and isn't verified here), +/// with no row policy that dispatch would silently drop, and visible to the current user. +bool isSafeDataLakeLeaf(const TableNode & table_node, const ContextPtr & context) +{ + auto * storage_cluster = dynamic_cast(table_node.getStorage().get()); + if (!storage_cluster || !storage_cluster->isResolvedViaDataLakeCatalog()) + return false; + + if (hasEffectiveRowPolicy(table_node, context)) + return false; + + return hasSelectAccess(table_node, context); +} + +bool isEligibleDriver(const TableNode & table_node, const ContextPtr & context, IStorageCluster *& out_storage) +{ + if (!isSafeDataLakeLeaf(table_node, context)) + return false; + + auto * storage_cluster = dynamic_cast(table_node.getStorage().get()); + if (storage_cluster->getClusterName(context).empty()) + return false; + + out_storage = storage_cluster; + return true; +} + +/// True if `node`'s own subtree (not crossing into a nested QueryNode/UnionNode) contains an aggregate or +/// window function -- catches e.g. `SELECT count() FROM driver`, which has no GROUP BY node at all. +bool containsAggregateOrWindowFunction(const QueryTreeNodePtr & node) +{ + if (!node) + return false; + + if (const auto * function_node = node->as()) + if (function_node->isAggregateFunction() || function_node->isWindowFunction()) + return true; + + if (node->as() || node->as()) + return false; + + for (const auto & child : node->getChildren()) + if (containsAggregateOrWindowFunction(child)) + return true; + + return false; +} + +/// The dispatch boundary itself may freely aggregate/order/limit -- WithMergeableState plus stock +/// finalization on top handles that correctly. But an *intermediate* QueryNode sitting between the dispatch +/// boundary and the driver (crossed via a nested subquery, strictly on the driver's own left path) gets +/// executed independently and completely on each worker's own partition of the driver; if it aggregates, +/// dedups, or limits, worker-local partial results get treated as final ones, which is wrong whenever a +/// group/row spans multiple workers' partitions. Conservative: reject any such construct here rather than try +/// to prove which ones happen to be partition-preserving. This never applies to a JOIN partner's own subquery +/// on the right of a JOIN -- that content is recomputed in full on every worker (see +/// allWorkerLocalReferencesAreSafe()), so GROUP BY/LIMIT/etc there is not a hazard at all. +bool isSafeIntermediateSubquery(const QueryNode & query_node) +{ + return !query_node.isDistinct() && !query_node.hasGroupBy() && !query_node.hasHaving() && !query_node.hasWindow() + && !query_node.hasQualify() && !query_node.hasOrderBy() && !query_node.hasInterpolate() && !query_node.hasLimitBy() + && !query_node.hasLimit() && !query_node.hasOffset() && !containsAggregateOrWindowFunction(query_node.getProjectionNode()) + && !containsAggregateOrWindowFunction(query_node.getWithNode()); +} + +struct DriverPathResult +{ + bool unusable = false; + const TableNode * driver = nullptr; + IStorageCluster * driver_storage = nullptr; + + /// Whether a supported JoinNode was found anywhere on the path down to the driver -- a candidate with no + /// JOIN at all has nothing for this mode to optimize, and dispatching it anyway would just replace stock + /// IStorageCluster::read() with a narrower prepared path. + bool has_join = false; +}; + +DriverPathResult unusableDriverPath() +{ + DriverPathResult result; + result.unusable = true; + return result; +} + +/// Walks strictly down the left spine looking for exactly one scheduling driver. Never inspects the right +/// side of a JOIN for a competing driver -- the right side is validated separately, as worker-local content +/// (see allWorkerLocalReferencesAreSafe()), which is why an RHS DataLake-catalog table, or a nested JOIN/ +/// GROUP BY over several such tables, never poisons or competes with the driver found here. +DriverPathResult findDriverOnLeftSpine(const QueryTreeNodePtr & node, const ContextPtr & context) +{ + if (const auto * table_node = node->as()) + { + IStorageCluster * storage = nullptr; + if (!isEligibleDriver(*table_node, context, storage)) + return unusableDriverPath(); + + DriverPathResult result; + result.driver = table_node; + result.driver_storage = storage; + return result; + } + + if (const auto * query_node = node->as()) + { + const auto & join_tree = query_node->getJoinTree(); + if (!join_tree) + return unusableDriverPath(); + + auto result = findDriverOnLeftSpine(join_tree, context); + if (result.unusable) + return result; + + /// `node` is crossed as an intermediate subquery here, not the dispatch boundary itself (that's the + /// QueryNode originally passed to findDistributedObjectStorageCandidate()). + if (!isSafeIntermediateSubquery(*query_node)) + return unusableDriverPath(); + + return result; + } + + if (const auto * join_node = node->as()) + { + const auto join_kind = join_node->getKind(); + const auto join_strictness = join_node->getStrictness(); + if ((join_kind != JoinKind::Inner || join_strictness != JoinStrictness::All) && join_kind != JoinKind::Left) + return unusableDriverPath(); + + auto left = findDriverOnLeftSpine(join_node->getLeftTableExpression(), context); + if (left.unusable) + return unusableDriverPath(); + + left.has_join = true; + return left; + } + + return unusableDriverPath(); +} + +/// After a driver is found, every other table reachable anywhere in the whole dispatch-boundary subtree -- +/// on the right of any JOIN, nested arbitrarily deep in a JOIN/GROUP BY of its own, or referenced from a +/// WHERE/HAVING/projection subquery -- must be safe to recompute in full, identically, on every worker: a +/// DataLake-catalog table with no row policy of its own and visible to the current user. This walk does not +/// classify anything as another driver and does not restrict GROUP BY/JOIN/LIMIT anywhere in this content -- +/// unlike the driver's own left-spine path, it is never partitioned, so each worker simply recomputes it +/// whole (see the setting's own documentation and findDistributedObjectStorageCandidate.h). +bool allWorkerLocalReferencesAreSafe(const QueryTreeNodePtr & node, const TableNode * driver, const ContextPtr & context) +{ + if (!node) + return true; + + if (const auto * table_node = node->as()) + return table_node == driver || isSafeDataLakeLeaf(*table_node, context); + + if (node->as()) + return false; + + for (const auto & child : node->getChildren()) + if (!allWorkerLocalReferencesAreSafe(child, driver, context)) + return false; + + return true; +} + +} + +std::optional findDistributedObjectStorageCandidate( + const QueryTreeNodePtr & query_node, const ContextPtr & context) +{ + if (context->getSettingsRef()[Setting::object_storage_cluster_join_mode] != ObjectStorageClusterJoinMode::DISTRIBUTED) + return {}; + + /// readPreparedClusterQuery() goes straight to the driver's own cluster, bypassing the remote-initiator + /// topology (convertToRemote()) that stock IStorageCluster::read() applies for this setting; falls back to + /// ordinary planning instead of silently ignoring it. + if (context->getSettingsRef()[Setting::object_storage_remote_initiator]) + return {}; + + /// additional_table_filters keys are matched against the initiator's current_database and the query's own + /// aliasing/naming, both of which shift once forwarded as fully serialized remote SQL -- Planner.cpp + /// disables parallel replicas for the exact same reason (see the comment there). Rather than replicate + /// case-by-case matching here, disable the combination entirely, same as that precedent. + if (!context->getSettingsRef()[Setting::additional_table_filters].value.empty()) + return {}; + + const auto * query_node_typed = query_node->as(); + if (!query_node_typed) + return {}; + + const auto & join_tree = query_node_typed->getJoinTree(); + if (!join_tree) + return {}; + + auto driver_path = findDriverOnLeftSpine(join_tree, context); + if (driver_path.unusable || !driver_path.driver || !driver_path.has_join) + return {}; + + if (!allWorkerLocalReferencesAreSafe(query_node, driver_path.driver, context)) + return {}; + + DistributedObjectStorageCandidate candidate; + candidate.driver = driver_path.driver; + candidate.driver_storage = driver_path.driver_storage; + return candidate; +} + +} diff --git a/src/Planner/findDistributedObjectStorageCandidate.h b/src/Planner/findDistributedObjectStorageCandidate.h new file mode 100644 index 000000000000..9ab626f16ab8 --- /dev/null +++ b/src/Planner/findDistributedObjectStorageCandidate.h @@ -0,0 +1,50 @@ +#pragma once +#include +#include +#include + +namespace DB +{ + +class TableNode; +class IStorageCluster; + +class IQueryTreeNode; +using QueryTreeNodePtr = std::shared_ptr; + +class Context; +using ContextPtr = std::shared_ptr; + +/// A whole-query JOIN-pushdown candidate for object_storage_cluster_join_mode='distributed'. `query_node` +/// (the exact QueryTreeNodePtr passed to findDistributedObjectStorageCandidate()) is always the dispatch +/// boundary: the entire query is forwarded as a whole to `driver`'s cluster, never a narrower subquery. +struct DistributedObjectStorageCandidate +{ + /// The driving TableNode, reachable from the dispatch boundary only via the left path of every + /// JOIN/subquery crossing (see findDriverOnLeftSpine() in the .cpp). + const TableNode * driver = nullptr; + + /// driver's resolved storage. + IStorageCluster * driver_storage = nullptr; +}; + +/// Whole-query dispatch is an all-or-nothing decision for `query_node` itself: either the entire subtree +/// reachable from it is safe to forward as one query to a single DataLake-catalog driver's cluster, or it +/// isn't and the caller falls back to ordinary planning for this exact QueryNode -- there is no narrower +/// fallback candidate search. Returns nullopt when: mode isn't 'distributed', `query_node` has no JOIN at +/// all (nothing here for this mode to optimize), no eligible driver is reachable via the left spine, or any +/// unsafe leaf is reachable anywhere in the subtree (explicit `*Cluster()`, local/Distributed table, +/// row policy, missing SELECT access, or a structurally unsupported shape). +/// +/// The driver is found by walking strictly down the left spine of `query_node`'s own JOIN/subquery tree: +/// QueryNode -> its join tree; supported JoinNode (INNER ALL or LEFT) -> left operand only; intermediate +/// QueryNode crossed along the way -> only if partition-preserving (see isSafeIntermediateSubquery() in the +/// .cpp); TableNode -> an eligible DataLake-catalog driver with a non-empty cluster. The right side of any +/// JOIN, and anything below it, is never inspected for a competing driver -- it is validated only as +/// worker-local, safe-to-recompute-in-full content (see allWorkerLocalReferencesAreSafe() in the .cpp), which +/// is why a DataLake-catalog table, or even a nested JOIN/GROUP BY over several such tables, is accepted on +/// the right without ever being considered for the driver role itself. +std::optional findDistributedObjectStorageCandidate( + const QueryTreeNodePtr & query_node, const ContextPtr & context); + +} diff --git a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp new file mode 100644 index 000000000000..11baeddb296d --- /dev/null +++ b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp @@ -0,0 +1,549 @@ +#include + +#include + +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace DB; + +namespace +{ + +NamesAndTypesList driverColumns() +{ + return {{"id", std::make_shared()}}; +} + +NamesAndTypesList lookupColumns() +{ + return {{"lookup_id", std::make_shared()}}; +} + +struct State; + +/// The builder resolves its driver replacement via QueryAnalysisPass, which -- like any table function +/// reference -- looks `fakeDriverFunction` up in the real TableFunctionFactory (QueryAnalyzer::resolveTableFunction()). +/// Registers a minimal stand-in that just returns the test's own driver storage, so the resolved +/// TableFunctionNode carries the same columns real production code would get back from e.g. icebergS3Cluster(). +/// executeImpl() is defined out-of-line, after State, since it needs State to be a complete type. +class FakeDriverTableFunction : public ITableFunction +{ +public: + static constexpr auto name = "fakeDriverFunction"; + std::string getName() const override { return name; } + bool hasStaticStructure() const override { return true; } + ColumnsDescription getActualTableStructure(ContextPtr, bool) const override { return ColumnsDescription{driverColumns()}; } + +protected: + /// The default implementation looks getStorageEngineName() up in StorageFactory for source-access checking, + /// which "FakeDriverStorage" (a test-only stand-in, never registered there) doesn't have. + std::optional getSourceAccessObject() const override { return std::nullopt; } + +private: + StoragePtr executeImpl(const ASTPtr &, ContextPtr, const std::string &, ColumnsDescription, bool) const override; + const char * getStorageEngineName() const override { return "FakeDriverStorage"; } +}; + +/// Minimal driver stand-in; getTaskIteratorExtension() is only invoked during real pipeline execution, +/// which these tests never trigger -- they only check the plan. +class FakeDriverStorage : public IStorageCluster +{ +public: + FakeDriverStorage(const StorageID & table_id, String cluster_name_) + : IStorageCluster(cluster_name_, table_id, getLogger("test")) + { + StorageInMemoryMetadata metadata; + metadata.setColumns(ColumnsDescription{driverColumns()}); + setInMemoryMetadata(metadata); + } + + std::string getName() const override { return "FakeDriverStorage"; } + bool isResolvedViaDataLakeCatalog() const override { return true; } + + RemoteQueryExecutor::Extension getTaskIteratorExtension( + const ActionsDAG::Node *, const ActionsDAG *, const ContextPtr &, ClusterPtr, StorageMetadataPtr) const override + { + return {}; + } + +protected: + /// Mirrors StorageObjectStorageCluster::updateQueryForDistributedEngineIfNeeded()'s alias handling + /// closely enough to exercise buildDistributedObjectStorageQueryPlan.cpp's own fallback-alias fix: + /// transfers whatever alias the driver's table identifier already had (empty if none) onto the + /// replacement table function, exactly like the real rewrite. + void updateQueryToSendIfNeeded(ASTPtr & query, const StorageSnapshotPtr &, const ContextPtr &, bool make_cluster_function) override + { + if (!make_cluster_function) + return; + + auto * select_query = query->as(); + if (!select_query || !select_query->tables()) + return; + + auto * tables = select_query->tables()->as(); + auto * table_expression = tables->children.at(0)->as()->table_expression->as(); + if (!table_expression || !table_expression->database_and_table_name) + return; + + auto table_alias = table_expression->database_and_table_name->tryGetAlias(); + auto function_ast = makeASTFunction("fakeDriverFunction"); + function_ast->setAlias(table_alias); + + table_expression->database_and_table_name = nullptr; + table_expression->table_function = function_ast; + table_expression->children[0] = function_ast; + } +}; + +/// Stand-in for a second table from the same DataLake catalog, e.g. ice.geo_location_lookup. +class FakeSafeLookupStorage : public IStorageCluster +{ +public: + explicit FakeSafeLookupStorage(const StorageID & table_id) + : IStorageCluster(/*cluster_name_*/ "", table_id, getLogger("test")) + { + StorageInMemoryMetadata metadata; + metadata.setColumns(ColumnsDescription{lookupColumns()}); + setInMemoryMetadata(metadata); + } + + std::string getName() const override { return "FakeSafeLookupStorage"; } + bool isResolvedViaDataLakeCatalog() const override { return true; } + + RemoteQueryExecutor::Extension getTaskIteratorExtension( + const ActionsDAG::Node *, const ActionsDAG *, const ContextPtr &, ClusterPtr, StorageMetadataPtr) const override + { + return {}; + } + +protected: + /// Empty cluster name -> plain reads (e.g. 'allow' mode) fall back here instead of ReadFromCluster. + void readFallBackToPure( + QueryPlan & query_plan, + const Names & column_names, + const StorageSnapshotPtr & storage_snapshot, + SelectQueryInfo &, + ContextPtr, + QueryProcessingStage::Enum, + size_t, + size_t) override + { + auto header = std::make_shared(storage_snapshot->getSampleBlockForColumns(column_names)); + Pipe pipe(std::make_shared(header)); + query_plan.addStep(std::make_unique(std::move(pipe))); + } +}; + +/// Port 1 is never listening; irrelevant since the test only builds the QueryPlan. +void registerUnreachableCluster(const ContextMutablePtr & context, const String & cluster_name) +{ + std::ostringstream config_text; + config_text << "<" << cluster_name << ">" + << "127.0.0.11" + << ""; + std::istringstream config_stream(config_text.str()); + Poco::AutoPtr config = new Poco::Util::XMLConfiguration(config_stream); + context->setClustersConfig(config, /*enable_discovery=*/false); +} + +/// Test fixture, modelled on src/Planner/tests/gtest_planner_empty_projection.cpp. +struct State +{ + State(const State &) = delete; + + ContextMutablePtr context; + std::shared_ptr driver; + + static State & instance() + { + static State state; + return state; + } + +private: + explicit State() + : context(Context::createCopy(getContext().context)) + { + tryRegisterFunctions(); + tryRegisterAggregateFunctions(); + + /// Default test context leaves query_kind at NO_QUERY; must look like a real initiator query. + ClientInfo client_info = context->getClientInfo(); + client_info.query_kind = ClientInfo::QueryKind::INITIAL_QUERY; + context->setClientInfo(client_info); + + /// The driver rewrite resolves its replacement TableFunctionNode via QueryAnalysisPass, which (like any + /// table function resolution -- QueryAnalyzer::resolveTableFunction()) requires a real query context + /// (context->getQueryContext() throws THERE_IS_NO_QUERY otherwise); every real query already has one. + context->makeQueryContext(); + + TableFunctionFactory::instance().registerFunction(FunctionDocumentation{}); + + static constexpr auto database_name = "distributed_object_storage_join_dispatch_test_db"; + static constexpr auto cluster_name = "vig-test"; + + DatabasePtr database = std::make_shared(database_name, context); + + driver = std::make_shared(StorageID(database_name, "driver"), cluster_name); + database->attachTable(context, "driver", driver, {}); + + database->attachTable(context, "safe_lookup", std::make_shared(StorageID(database_name, "safe_lookup")), {}); + database->attachTable(context, "dim2", std::make_shared(StorageID(database_name, "dim2")), {}); + + DatabaseCatalog::instance().attachDatabase(database->getDatabaseName(), database); + context->setCurrentDatabase(database_name); + + registerUnreachableCluster(context, cluster_name); + } +}; + +StoragePtr FakeDriverTableFunction::executeImpl(const ASTPtr &, ContextPtr, const std::string &, ColumnsDescription, bool) const +{ + return State::instance().driver; +} + +/// getQueryPlan() returns a reference into the interpreter's own move-only plan, so build+explain in one scope. +String planAndExplain(const String & query, const ContextMutablePtr & context) +{ + ParserSelectQuery parser; + ASTPtr ast = parseQuery(parser, query, 1000, 1000, 1000000); + auto query_tree = buildQueryTree(ast, context); + QueryTreePassManager pass_manager(context); + addQueryTreePasses(pass_manager); + pass_manager.run(query_tree); + + SelectQueryOptions options; + InterpreterSelectQueryAnalyzer interpreter(query_tree, context, options); + auto & plan = interpreter.getQueryPlan(); + + WriteBufferFromOwnString buffer; + plan.explainPlan(buffer, {.header = true, .description = true, .actions = true}); + return buffer.str(); +} + +/// Like planAndExplain(), but also runs the query plan optimizer (predicate pushdown included) before +/// explaining -- this is what actually drives ReadFromCluster::applyFilters() during a real EXPLAIN/execution, +/// which planAndExplain() alone never touches. Needed to reproduce the live q17 crash trigger: a WHERE on the +/// driver gets pushed down as a filter onto the whole-query ReadFromCluster step, whose SelectQueryInfo used to +/// carry a mismatched planner_context/table_expression pair for a per-table filter lookup that this step isn't. +String planOptimizeAndExplain(const String & query, const ContextMutablePtr & context) +{ + ParserSelectQuery parser; + ASTPtr ast = parseQuery(parser, query, 1000, 1000, 1000000); + auto query_tree = buildQueryTree(ast, context); + QueryTreePassManager pass_manager(context); + addQueryTreePasses(pass_manager); + pass_manager.run(query_tree); + + SelectQueryOptions options; + InterpreterSelectQueryAnalyzer interpreter(query_tree, context, options); + auto & plan = interpreter.getQueryPlan(); + plan.optimize(QueryPlanOptimizationSettings(context)); + + WriteBufferFromOwnString buffer; + plan.explainPlan(buffer, {.header = true, .description = true, .actions = true}); + return buffer.str(); +} + +/// Finds the single ReadFromCluster step in `node`'s subtree, or nullptr. +ReadFromCluster * findReadFromCluster(QueryPlan::Node * node) +{ + if (!node) + return nullptr; + if (auto * read_from_cluster = dynamic_cast(node->step.get())) + return read_from_cluster; + for (auto * child : node->children) + if (auto * found = findReadFromCluster(child)) + return found; + return nullptr; +} + +} + +/// Core "q17" regression: whole query dispatches as one ReadFromCluster step, no local JOIN. +TEST(DistributedObjectStorageJoinDispatch, DriverOwnsWholeJoinWhenModeIsDistributed) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planAndExplain("SELECT driver.id, safe_lookup.lookup_id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.lookup_id", state.context); + + EXPECT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; + EXPECT_NE(plan_text.find("INNER JOIN"), String::npos) << "expected the whole JOIN forwarded in ReadFromCluster's query, got:\n" << plan_text; + EXPECT_EQ(plan_text.find("JoinLogical"), String::npos) << "expected no local JOIN step, got:\n" << plan_text; +} + +/// The driver has no explicit alias in this query, yet its column references must still resolve once the +/// forwarded query is re-parsed and re-analyzed on the worker: collectTableExpressionData() assigns every +/// table (aliased or not) a globally unique `__tableN` identifier, which is what queryNodeToDistributedSelectQuery() +/// actually uses to qualify column references (see ColumnNode::toASTImpl()) -- and rewriteQueryToExplicitClusterForm() +/// transfers that same identifier onto the rewritten table function as its alias, via tryGetAlias(). Verifies that +/// invariant directly: whatever alias the rewritten driver function carries must match the qualifier its own +/// `id` column reference uses. +TEST(DistributedObjectStorageJoinDispatch, UnaliasedDriverKeepsColumnReferencesResolvableAfterRewrite) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planAndExplain( + "SELECT driver.id, safe_lookup.lookup_id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.lookup_id", state.context); + + ASSERT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; + + auto function_pos = plan_text.find("fakeDriverFunction()"); + ASSERT_NE(function_pos, String::npos) << "expected the driver to be rewritten to its fake cluster function, got:\n" << plan_text; + auto as_pos = plan_text.find(" AS ", function_pos); + ASSERT_NE(as_pos, String::npos) << "expected the rewritten driver function to carry an alias, got:\n" << plan_text; + auto alias_start = as_pos + 4; + auto alias_end = plan_text.find_first_of(" \n", alias_start); + auto alias = plan_text.substr(alias_start, alias_end - alias_start); + + EXPECT_NE(plan_text.find(alias + ".id"), String::npos) + << "expected the driver's own `id` reference to be qualified with its rewritten function's alias `" << alias + << "`, got:\n" << plan_text; +} + +/// Default mode ('allow'): unaffected, JOIN still executes locally. +TEST(DistributedObjectStorageJoinDispatch, DriverIsWrappedWhenModeIsNotDistributed) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("allow")); + + auto plan_text = planAndExplain("SELECT driver.id, safe_lookup.lookup_id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.lookup_id", state.context); + + EXPECT_NE(plan_text.find("JoinLogical"), String::npos) << "expected a local JOIN step, got:\n" << plan_text; +} + +/// "q21" regression: driver buried in a subquery, outer JOIN + GROUP BY all dispatch as one ReadFromCluster, +/// with stock MergingAggregated finalization reused on top. +TEST(DistributedObjectStorageJoinDispatch, BuriedDriverOwnsWholeOuterQueryWithGroupBy) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planAndExplain( + "SELECT x.id, count() FROM " + "(SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.lookup_id) AS x " + "INNER JOIN dim2 ON x.id = dim2.lookup_id " + "GROUP BY x.id", + state.context); + + EXPECT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; + EXPECT_NE(plan_text.find("INNER JOIN"), String::npos) << "expected both JOINs forwarded in ReadFromCluster's query, got:\n" << plan_text; + EXPECT_EQ(plan_text.find("JoinLogical"), String::npos) << "expected no local JOIN step, got:\n" << plan_text; + EXPECT_NE(plan_text.find("MergingAggregated"), String::npos) + << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; +} + +/// The ReadFromCluster header must reflect the WithMergeableState stage (unmerged aggregate states), not the +/// query's final projection types -- count() is UInt64 only after MergingAggregated, AggregateFunction(count) +/// beforehand. A header built from the final projection instead would declare the wrong type here, undetected +/// by plan structure alone since ReadFromCluster's header is never validated against nothing at plan time. +TEST(DistributedObjectStorageJoinDispatch, ReadFromClusterHeaderCarriesUnmergedAggregateState) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planAndExplain( + "SELECT x.id, count() FROM " + "(SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.lookup_id) AS x " + "INNER JOIN dim2 ON x.id = dim2.lookup_id " + "GROUP BY x.id", + state.context); + + auto read_from_cluster_pos = plan_text.find("ReadFromCluster"); + ASSERT_NE(read_from_cluster_pos, String::npos) << plan_text; + auto merging_aggregated_pos = plan_text.find("MergingAggregated"); + ASSERT_NE(merging_aggregated_pos, String::npos) << plan_text; + + /// explainPlan() prints children before parents, so ReadFromCluster's own header block sits between the + /// two step names. + auto read_from_cluster_block = plan_text.substr(read_from_cluster_pos, merging_aggregated_pos - read_from_cluster_pos); + EXPECT_NE(read_from_cluster_block.find("AggregateFunction(count"), String::npos) + << "expected ReadFromCluster's header to carry the unmerged aggregate state type, got:\n" << read_from_cluster_block; +} + +/// Real "q17"-shaped projection: a CASE expression reading columns from both sides of the JOIN, plus count(), +/// GROUP BY, ORDER BY and LIMIT all on the dispatch boundary itself. Exercises the header/rename machinery +/// with more than one plain passthrough column, unlike the trivial single-column projections above. +TEST(DistributedObjectStorageJoinDispatch, ComplexQ17ProjectionBuildsSuccessfully) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planAndExplain( + "SELECT " + " CASE WHEN safe_lookup.lookup_id > 0 THEN driver.id ELSE safe_lookup.lookup_id END AS dst_city, " + " count() AS c " + "FROM driver LEFT JOIN safe_lookup ON driver.id = safe_lookup.lookup_id " + "GROUP BY dst_city " + "ORDER BY dst_city " + "LIMIT 10", + state.context); + + EXPECT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; + EXPECT_NE(plan_text.find("LEFT JOIN"), String::npos) << "expected the whole JOIN forwarded in ReadFromCluster's query, got:\n" << plan_text; + EXPECT_EQ(plan_text.find("JoinLogical"), String::npos) << "expected no local JOIN step, got:\n" << plan_text; + EXPECT_NE(plan_text.find("MergingAggregated"), String::npos) + << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; +} + +/// Regression for the live q17 SIGSEGV: a WHERE on the driver survives real plan optimization (not just +/// candidate discovery/plan construction), which pushes it down as a filter onto the whole-query +/// ReadFromCluster step and calls ReadFromCluster::applyFilters() -> SourceStepWithFilter::applyFilters() -> +/// SelectQueryInfo::buildNodeNameToInputNodeColumn(). That used to look up a per-table `table_expression` in a +/// `planner_context` describing the *whole* dispatched query, throwing while formatting the error message by +/// dereferencing a null table_expression. readPreparedClusterQuery() must not leave that pair set on the +/// SelectQueryInfo it hands to ReadFromCluster. +TEST(DistributedObjectStorageJoinDispatch, ComplexQ17ProjectionWithWhereSurvivesPlanOptimization) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planOptimizeAndExplain( + "SELECT " + " CASE WHEN safe_lookup.lookup_id > 0 THEN driver.id ELSE safe_lookup.lookup_id END AS dst_city, " + " count() AS c " + "FROM driver LEFT JOIN safe_lookup ON driver.id = safe_lookup.lookup_id " + "WHERE driver.id > 0 " + "GROUP BY dst_city " + "ORDER BY dst_city " + "LIMIT 10", + state.context); + + EXPECT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; + EXPECT_EQ(plan_text.find("JoinLogical"), String::npos) << "expected no local JOIN step, got:\n" << plan_text; +} + +/// Real "q21" shape: driver behind one intermediate subquery, outer LEFT JOIN against a RHS subquery that +/// itself LEFT JOINs a further GROUP BY subquery and also has its own GROUP BY. None of that RHS content is a +/// competing driver -- it's recomputed whole on every worker -- and the whole thing must still build into a +/// single dispatched ReadFromCluster with stock finalization for the outer GROUP BY on top. +TEST(DistributedObjectStorageJoinDispatch, RealQ21ShapeBuildsSuccessfully) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planAndExplain( + "SELECT transaction_event.id, count() FROM " + "(SELECT driver.id FROM driver) AS transaction_event " + "LEFT JOIN " + "(SELECT safe_lookup.lookup_id AS id FROM safe_lookup LEFT JOIN " + "(SELECT dim2.lookup_id AS id FROM dim2 GROUP BY dim2.lookup_id) AS policy_matches " + "ON safe_lookup.lookup_id = policy_matches.id GROUP BY safe_lookup.lookup_id) AS alert_events " + "ON transaction_event.id = alert_events.id " + "GROUP BY transaction_event.id", + state.context); + + EXPECT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; + EXPECT_EQ(plan_text.find("JoinLogical"), String::npos) << "expected no local JOIN step, got:\n" << plan_text; + EXPECT_NE(plan_text.find("MergingAggregated"), String::npos) + << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; +} + +/// The same q21 shape, but asserting the rewrite itself: exactly one driver -- `driver`, buried inside +/// `transaction_event` -- becomes the explicit cluster function; every other DataLake table reachable from the +/// RHS (`safe_lookup`, `dim2`) stays an ordinary catalog identifier, never itself rewritten into a driver. +TEST(DistributedObjectStorageJoinDispatch, RealQ21ShapeRewritesOnlyTheBuriedDriver) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planAndExplain( + "SELECT transaction_event.id, count() FROM " + "(SELECT driver.id FROM driver) AS transaction_event " + "LEFT JOIN " + "(SELECT safe_lookup.lookup_id AS id FROM safe_lookup LEFT JOIN " + "(SELECT dim2.lookup_id AS id FROM dim2 GROUP BY dim2.lookup_id) AS policy_matches " + "ON safe_lookup.lookup_id = policy_matches.id GROUP BY safe_lookup.lookup_id) AS alert_events " + "ON transaction_event.id = alert_events.id " + "GROUP BY transaction_event.id", + state.context); + + size_t driver_function_count = 0; + for (size_t pos = plan_text.find("fakeDriverFunction("); pos != String::npos; pos = plan_text.find("fakeDriverFunction(", pos + 1)) + ++driver_function_count; + EXPECT_EQ(driver_function_count, 1u) << "expected exactly one explicit driver cluster function, got:\n" << plan_text; + + EXPECT_NE(plan_text.find("safe_lookup"), String::npos) << "expected safe_lookup to remain an ordinary catalog identifier, got:\n" << plan_text; + EXPECT_NE(plan_text.find("dim2"), String::npos) << "expected dim2 to remain an ordinary catalog identifier, got:\n" << plan_text; +} + +/// SourceStepWithFilter::required_source_columns is checked against the driver's own StorageSnapshot +/// (updatePrewhereInfo() calls storage_snapshot->getSampleBlockForColumns(required_source_columns)) -- it must +/// be the driver's physical columns, never the whole dispatched query's own output projection (that's a +/// separate concept, carried by ReadFromCluster's header/sample_block instead). +TEST(DistributedObjectStorageJoinDispatch, RequiredSourceColumnsAreDriverColumnsNotOuterProjection) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + ParserSelectQuery parser; + String query = + "SELECT " + " CASE WHEN safe_lookup.lookup_id > 0 THEN driver.id ELSE safe_lookup.lookup_id END AS dst_city, " + " count() AS c " + "FROM driver LEFT JOIN safe_lookup ON driver.id = safe_lookup.lookup_id " + "GROUP BY dst_city"; + ASTPtr ast = parseQuery(parser, query, 1000, 1000, 1000000); + auto query_tree = buildQueryTree(ast, state.context); + QueryTreePassManager pass_manager(state.context); + addQueryTreePasses(pass_manager); + pass_manager.run(query_tree); + + SelectQueryOptions options; + InterpreterSelectQueryAnalyzer interpreter(query_tree, state.context, options); + auto & plan = interpreter.getQueryPlan(); + + auto * read_from_cluster = findReadFromCluster(plan.getRootNode()); + ASSERT_NE(read_from_cluster, nullptr); + EXPECT_EQ(read_from_cluster->requiredSourceColumns(), Names{"id"}) + << "expected the driver's own physical columns, not the outer query's projection (dst_city, c)"; +} + +/// The mode can also arrive via a query-level SETTINGS clause rather than context->setSetting(); the header's +/// hook-disabling context copy (Context::createCopy(context)) must still see it and disable the hook for the +/// sample-block analysis, or this recurses instead of the ambient session-level context not mattering here. +TEST(DistributedObjectStorageJoinDispatch, HeaderComputationHandlesQueryLevelModeSetting) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("allow")); + + auto plan_text = planAndExplain( + "SELECT driver.id, count() FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.lookup_id " + "GROUP BY driver.id SETTINGS object_storage_cluster_join_mode = 'distributed'", + state.context); + + EXPECT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; + EXPECT_NE(plan_text.find("AggregateFunction(count"), String::npos) + << "expected header computation to still see WithMergeableState types under a query-level SETTINGS override, got:\n" << plan_text; +} diff --git a/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp b/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp new file mode 100644 index 000000000000..ddea6fac890e --- /dev/null +++ b/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp @@ -0,0 +1,563 @@ +#include + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace DB; + +namespace +{ + +NamesAndTypesList testColumns() +{ + return {{"id", std::make_shared()}}; +} + +/// Minimal IStorageCluster test double, configurable per instance. +class FakeClusterStorage : public IStorageCluster +{ +public: + FakeClusterStorage(const StorageID & table_id, String cluster_name_, bool resolved_via_datalake_catalog_) + : IStorageCluster(cluster_name_, table_id, getLogger("test")) + , resolved_via_datalake_catalog(resolved_via_datalake_catalog_) + { + StorageInMemoryMetadata metadata; + metadata.setColumns(ColumnsDescription{testColumns()}); + setInMemoryMetadata(metadata); + } + + std::string getName() const override { return "FakeClusterStorage"; } + bool isResolvedViaDataLakeCatalog() const override { return resolved_via_datalake_catalog; } + + RemoteQueryExecutor::Extension getTaskIteratorExtension( + const ActionsDAG::Node *, const ActionsDAG *, const ContextPtr &, ClusterPtr, StorageMetadataPtr) const override + { + return {}; + } + +private: + bool resolved_via_datalake_catalog; +}; + +/// Modelled on src/Planner/tests/gtest_planner_empty_projection.cpp. +struct State +{ + State(const State &) = delete; + + ContextMutablePtr context; + + static const State & instance() + { + static State state; + return state; + } + +private: + explicit State() + : context(Context::createCopy(getContext().context)) + { + tryRegisterFunctions(); + tryRegisterAggregateFunctions(); + + static constexpr auto database_name = "find_distributed_object_storage_candidate_test_db"; + DatabasePtr database = std::make_shared(database_name, context); + + auto attach_cluster_table = [&](const String & table_name, String cluster_name, bool resolved_via_datalake_catalog) + { + database->attachTable( + context, + table_name, + std::make_shared(StorageID(database_name, table_name), std::move(cluster_name), resolved_via_datalake_catalog), + {}); + }; + + /// A distributed driver, e.g. ice.event_page. + attach_cluster_table("driver", "vig-test", /*resolved_via_datalake_catalog=*/true); + /// A second DataLake-catalog table under the same cluster -- never an independent driver on the + /// right of a JOIN, just a plain (if wasteful) safe partner. + attach_cluster_table("second_datalake_table", "vig-test", /*resolved_via_datalake_catalog=*/true); + /// A safe co-resolved table with no cluster dispatch of its own (e.g. ice.geo_location_lookup). + attach_cluster_table("safe_lookup", "", /*resolved_via_datalake_catalog=*/true); + /// A Cluster-engine table not resolved through DatabaseDataLake -- not safe. + attach_cluster_table("unsafe_cluster_table", "some-cluster", /*resolved_via_datalake_catalog=*/false); + + database->attachTable( + context, + "local_table", + std::make_shared( + StorageID(database_name, "local_table"), ColumnsDescription{testColumns()}, ConstraintsDescription{}, String{}, MemorySettings{}), + {}); + + DatabaseCatalog::instance().attachDatabase(database->getDatabaseName(), database); + context->setCurrentDatabase(database_name); + } +}; + +QueryTreeNodePtr analyze(const String & query, const ContextMutablePtr & context) +{ + ParserSelectQuery parser; + ASTPtr ast = parseQuery(parser, query, 1000, 1000, 1000000); + auto query_tree = buildQueryTree(ast, context); + QueryTreePassManager pass_manager(context); + addQueryTreePasses(pass_manager); + pass_manager.run(query_tree); + return query_tree; +} + +/// AccessControl is shared across every Context::createCopy() of the same test-global context, so entity +/// names must be unique per call, not just per helper, or a second test's insert() throws "already exists". +size_t nextTestAccessEntitySuffix() +{ + static std::atomic counter{0}; + return counter++; +} + +/// A context copy whose user has no grants at all -- Context::getAccess() gives the global context (no +/// setUserID() call) full access unconditionally, so a real, minimally-privileged user is needed to exercise +/// the access-denied path at all. +ContextMutablePtr contextWithNoGrants(const ContextMutablePtr & base_context) +{ + auto context = Context::createCopy(base_context); + context->getAccessControl().addMemoryStorage("find_distributed_object_storage_candidate_test_storage", /*allow_backup_*/ false); + auto user = std::make_shared(); + user->setName(fmt::format("find_distributed_object_storage_candidate_test_no_grants_user_{}", nextTestAccessEntitySuffix())); + auto user_id = context->getAccessControl().insert(user); + context->setUser(user_id); + return context; +} + +/// A context copy whose user has full access, but a nontrivial row policy applies to `table_name`. +ContextMutablePtr contextWithRowPolicy(const ContextMutablePtr & base_context, const String & database_name, const String & table_name) +{ + auto context = Context::createCopy(base_context); + context->getAccessControl().addMemoryStorage("find_distributed_object_storage_candidate_test_storage", /*allow_backup_*/ false); + auto suffix = nextTestAccessEntitySuffix(); + + auto user = std::make_shared(); + user->setName(fmt::format("find_distributed_object_storage_candidate_test_row_policy_user_{}", suffix)); + user->access.grant(AccessType::ALL); + auto user_id = context->getAccessControl().insert(user); + + auto policy = std::make_shared(); + policy->setFullName(fmt::format("find_distributed_object_storage_candidate_test_policy_{}", suffix), database_name, table_name); + policy->filters[static_cast(RowPolicyFilterType::SELECT_FILTER)] = "id != -1"; + policy->to_roles = RolesOrUsersSet(user_id); + context->getAccessControl().insert(policy); + + context->setUser(user_id); + return context; +} + +} + +TEST(FindDistributedObjectStorageCandidate, AcceptsDriverJoinedWithSafeDataLakeTable) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id", state.context); + + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); + ASSERT_NE(candidate->driver, nullptr); + EXPECT_EQ(candidate->driver->getStorageID().table_name, "driver"); +} + +TEST(FindDistributedObjectStorageCandidate, DoesNotInterceptPlainDistributedObjectStorageQuery) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM driver", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsOrdinaryLocalTableAsRhs) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN local_table ON driver.id = local_table.id", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsWhenModeIsNotDistributed) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("allow")); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsWhenRemoteInitiatorIsSet) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + state.context->setSetting("object_storage_remote_initiator", true); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); + + state.context->setSetting("object_storage_remote_initiator", false); +} + +/// additional_table_filters matching depends on the initiator's current_database and the query's own +/// aliasing, both of which shift once forwarded as fully serialized remote SQL (the same reasoning +/// Planner.cpp already uses to disable parallel replicas for this combination). Rejected as a blanket +/// disablement whenever the setting is nonempty at all, regardless of which table it names. +TEST(FindDistributedObjectStorageCandidate, RejectsWhenAdditionalTableFiltersIsSet) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + state.context->setSetting("additional_table_filters", String("{'driver': 'id > 0'}")); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); + + state.context->setSetting("additional_table_filters", String("")); +} + +/// Ordinary per-table planning checks SELECT access on every table it plans (checkAccessRights() in +/// PlannerJoinTree.cpp); this whole-query dispatch replaces that walk entirely, so it must perform the +/// same check itself for the driver and every JOIN partner, or a user without SELECT could gain access to +/// the driver's data simply by enabling 'distributed' mode. +TEST(FindDistributedObjectStorageCandidate, RejectsDriverWithoutSelectAccess) +{ + const auto & state = State::instance(); + auto context = contextWithNoGrants(state.context); + context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id", context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, context).has_value()); +} + +/// Same access check, but the table without SELECT access is buried inside a subquery ("q21" shape) rather +/// than at the dispatch boundary's own top-level JOIN -- the recursive intermediate-subquery walk must reach +/// it too, not just the tables directly visible at the outermost level. +TEST(FindDistributedObjectStorageCandidate, RejectsBuriedDriverWithoutSelectAccess) +{ + const auto & state = State::instance(); + auto context = contextWithNoGrants(state.context); + context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT x.id FROM (SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id) AS x " + "INNER JOIN second_datalake_table ON x.id = second_datalake_table.id", + context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, context).has_value()); +} + +/// A row policy on the driver itself: the driver is rewritten to its explicit *Cluster() form and no longer +/// resolves via its catalog identity, so the policy can never be reattached on the worker. +TEST(FindDistributedObjectStorageCandidate, RejectsDriverWithRowPolicy) +{ + const auto & state = State::instance(); + auto context = contextWithRowPolicy(state.context, "find_distributed_object_storage_candidate_test_db", "driver"); + context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id", context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, context).has_value()); +} + +/// A row policy on a non-driver JOIN partner: unlike the driver, this table stays a plain catalog identifier +/// and is independently re-resolved by DatabaseDataLake on each worker -- there is no cheap way to prove that +/// re-resolution applies the same effective policy as the initiator's own user, so it must conservatively +/// block whole-query dispatch too, not just a policy on the driver. +TEST(FindDistributedObjectStorageCandidate, RejectsNonDriverPartnerWithRowPolicy) +{ + const auto & state = State::instance(); + auto context = contextWithRowPolicy(state.context, "find_distributed_object_storage_candidate_test_db", "safe_lookup"); + context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id", context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, AcceptsSecondDataLakeTableAsRhsEvenThoughItIsAlsoDispatchCapable) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree + = analyze("SELECT driver.id FROM driver INNER JOIN second_datalake_table ON driver.id = second_datalake_table.id", state.context); + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); + EXPECT_EQ(candidate->driver->getStorageID().table_name, "driver"); +} + +/// A second driver-capable table sitting in its own subquery on the right of the outer JOIN is never inspected +/// as a competing driver at all -- it's just worker-local content, safe because it's a DataLake-catalog table, +/// same as AcceptsSecondDataLakeTableAsRhsEvenThoughItIsAlsoDispatchCapable above but one level deeper. +TEST(FindDistributedObjectStorageCandidate, AcceptsSecondDriverCapableSubqueryAsRhs) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT x.id FROM (SELECT driver.id FROM driver) AS x " + "INNER JOIN (SELECT second_datalake_table.id FROM second_datalake_table) AS y ON x.id = y.id", + state.context); + + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); + EXPECT_EQ(candidate->driver->getStorageID().table_name, "driver"); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsUnsafeClusterTableAsDriver) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree + = analyze("SELECT unsafe_cluster_table.id FROM unsafe_cluster_table INNER JOIN safe_lookup ON unsafe_cluster_table.id = safe_lookup.id", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsUnsafeIStorageClusterTableAsRhs) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree + = analyze("SELECT driver.id FROM driver INNER JOIN unsafe_cluster_table ON driver.id = unsafe_cluster_table.id", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +/// Nested JoinNodes within one QueryNode (chained JOINs) must be fully understood, not just one JOIN. +TEST(FindDistributedObjectStorageCandidate, AcceptsDriverInMultiJoinQueryNode) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT driver.id FROM driver " + "INNER JOIN safe_lookup ON driver.id = safe_lookup.id " + "INNER JOIN second_datalake_table ON driver.id = second_datalake_table.id", + state.context); + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); + EXPECT_EQ(candidate->driver->getStorageID().table_name, "driver"); +} + +/// A GROUP BY inside the intermediate subquery `x` (between the dispatch boundary and the driver, on the +/// driver's own left path) would be executed independently per worker partition if the whole query were +/// dispatched -- unsafe, since a group spanning multiple workers' partitions would never get merged. No +/// fallback: the whole query is simply not a candidate; stock planning handles it (which plans `x`'s own +/// GROUP BY correctly, as the boundary of its own ordinary subquery plan). +TEST(FindDistributedObjectStorageCandidate, RejectsWhenDriverPathIntermediateSubqueryHasGroupBy) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT x.id, x.c FROM " + "(SELECT driver.id, count() AS c FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id GROUP BY driver.id) AS x " + "INNER JOIN second_datalake_table ON x.id = second_datalake_table.id", + state.context); + + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +/// Same hazard as GROUP BY: a LIMIT inside a driver-path intermediate subquery would apply per worker +/// partition, not globally, if the whole query were dispatched. No fallback. +TEST(FindDistributedObjectStorageCandidate, RejectsWhenDriverPathIntermediateSubqueryHasLimit) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT x.id FROM " + "(SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id LIMIT 10) AS x " + "INNER JOIN second_datalake_table ON x.id = second_datalake_table.id", + state.context); + + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +/// A plain WHERE/projection-only intermediate subquery is partition-preserving (each row stays independent), +/// so the outer query is still the accepted dispatch boundary, same as before this safety check existed. +TEST(FindDistributedObjectStorageCandidate, AcceptsOuterCandidateWhenIntermediateSubqueryIsWhereProjectionOnly) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT x.id FROM " + "(SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id WHERE driver.id > 0) AS x " + "INNER JOIN second_datalake_table ON x.id = second_datalake_table.id", + state.context); + + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); +} + +/// The right side of a JOIN is never searched for a driver, no matter what it contains -- so a driver-bearing +/// subquery sitting there simply isn't found; the left side alone decides whether there's a candidate at all +/// (here it doesn't have one, since `safe_lookup` has no cluster of its own). No fallback: rejected outright. +TEST(FindDistributedObjectStorageCandidate, RejectsWhenLeftSpineHasNoDriverEvenThoughRhsSubqueryDoes) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT x.id FROM safe_lookup LEFT JOIN " + "(SELECT driver.id FROM driver) AS x " + "ON safe_lookup.id = x.id", + state.context); + + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsUnsafeSubqueryInWhereClause) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id " + "WHERE driver.id IN (SELECT id FROM local_table)", + state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsCrossJoin) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM driver CROSS JOIN safe_lookup", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsDriverOnTheRightOfRightJoin) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM safe_lookup RIGHT JOIN driver ON safe_lookup.id = driver.id", state.context); + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, RejectsExplicitClusterTableFunctionAsRhs) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze("SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id", state.context); + + /// Build a TableFunctionNode directly -- no real TableFunctionFactory registration needed. + auto & query_node = query_tree->as(); + auto & join_node = query_node.getJoinTree()->as(); + + auto table_function_node = std::make_shared("icebergS3Cluster"); + auto explicit_cluster_storage = std::make_shared( + StorageID("system", "explicit_cluster_table"), "other-cluster", /*resolved_via_datalake_catalog=*/false); + table_function_node->resolve(nullptr, explicit_cluster_storage, state.context, {}); + join_node.getRightTableExpression() = table_function_node; + + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +/// "q21" shape: driver buried in a subquery joined against another safe table; outer query is the candidate. +TEST(FindDistributedObjectStorageCandidate, AcceptsBuriedDriverWithSafeOuterJoin) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT x.id FROM (SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id) AS x " + "INNER JOIN second_datalake_table ON x.id = second_datalake_table.id", + state.context); + + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); + EXPECT_EQ(candidate->driver->getStorageID().table_name, "driver"); +} + +/// Same shape, but the outer JOIN partner is an ordinary local table: whole-query dispatch would have to +/// forward it too (it's part of the same dispatch boundary), which isn't safe -- rejected outright, no +/// fallback to dispatching the inner subquery alone even though it would be safe on its own. +TEST(FindDistributedObjectStorageCandidate, RejectsWhenOuterJoinPartnerIsUnsafe) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT x.id FROM (SELECT driver.id FROM driver INNER JOIN safe_lookup ON driver.id = safe_lookup.id) AS x " + "INNER JOIN local_table ON x.id = local_table.id", + state.context); + + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +/// Real "q21" shape: the driver sits behind one intermediate subquery (`transaction_event`, standing in for +/// `txnlog`) on the left of the outer LEFT JOIN; the right side (`alert_events`) is itself a LEFT JOIN against +/// a further subquery with its own GROUP BY (`policy_matches`), and `alert_events` itself also has a GROUP BY. +/// None of that RHS structure is inspected for a competing driver or restricted for GROUP BY/JOIN -- it's +/// worker-local content, recomputed whole on every worker. The outer query's own GROUP BY/ORDER BY/LIMIT are +/// the dispatch boundary's own, handled by stock finalization on top of the dispatched read. +TEST(FindDistributedObjectStorageCandidate, AcceptsRealQ21Shape) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT transaction_event.id, count() AS c FROM " + "(SELECT driver.id FROM driver WHERE driver.id > 0) AS transaction_event " + "LEFT JOIN " + "(SELECT safe_lookup.id FROM safe_lookup LEFT JOIN " + "(SELECT second_datalake_table.id FROM second_datalake_table GROUP BY second_datalake_table.id) AS policy_matches " + "ON safe_lookup.id = policy_matches.id GROUP BY safe_lookup.id) AS alert_events " + "ON transaction_event.id = alert_events.id " + "GROUP BY transaction_event.id " + "ORDER BY transaction_event.id " + "LIMIT 10", + state.context); + + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); + EXPECT_EQ(candidate->driver->getStorageID().table_name, "driver"); +} + +/// Real "q17" shape: one root LEFT JOIN between the driver and a safe lookup table, with WHERE, GROUP BY, +/// ORDER BY and LIMIT all sitting directly on the dispatch boundary itself (not an intermediate subquery) -- +/// freely allowed there, unlike on a driver-path intermediate subquery. +TEST(FindDistributedObjectStorageCandidate, AcceptsRealQ17Shape) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "SELECT driver.id, count() FROM driver LEFT JOIN safe_lookup ON driver.id = safe_lookup.id " + "WHERE driver.id > 0 GROUP BY driver.id ORDER BY driver.id LIMIT 10", + state.context); + + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); + EXPECT_EQ(candidate->driver->getStorageID().table_name, "driver"); +} diff --git a/src/Storages/IStorageCluster.cpp b/src/Storages/IStorageCluster.cpp index fe1a51a861c0..77a15cfb33c8 100644 --- a/src/Storages/IStorageCluster.cpp +++ b/src/Storages/IStorageCluster.cpp @@ -45,6 +45,10 @@ #include #include #include +#include +#include +#include +#include #include @@ -71,6 +75,7 @@ namespace Setting extern const SettingsBool object_storage_remote_initiator; extern const SettingsString object_storage_remote_initiator_cluster; extern const SettingsObjectStorageClusterJoinMode object_storage_cluster_join_mode; + extern const SettingsString object_storage_cluster; } namespace ErrorCodes @@ -132,6 +137,49 @@ ActionsDAG andListingFilterDAGs(ActionsDAG first, ActionsDAG second) } +namespace +{ + +/// Whole-query dispatch and ordinary remote execution need opposite `object_storage_cluster*` settings on the +/// worker side (see ReadFromCluster::updateSettings()'s own comment): this mirrors that same normalization +/// onto `query_to_send`'s own query-level SETTINGS clause, since a query-level SETTINGS entry there would +/// otherwise re-override whatever ReadFromCluster::updateSettings() sets on the outgoing context. +void sanitizeObjectStorageClusterQuerySettings(ASTPtr & query, bool is_whole_query_dispatch) +{ + auto * select_query = query->as(); + if (!select_query) + return; + + auto settings_ast = select_query->settings(); + if (!settings_ast) + return; + + auto & changes = settings_ast->as().changes; + bool changed = false; + + if (is_whole_query_dispatch) + { + changed = changes.removeSetting("object_storage_cluster"); + } + else + { + for (auto & change : changes) + { + if (change.name != "object_storage_cluster_join_mode") + continue; + if (change.value.safeGet() != "distributed") + continue; + change.value = Field(String("allow")); + changed = true; + } + } + + if (changed && changes.empty()) + select_query->setExpression(ASTSelectQuery::Expression::SETTINGS, {}); +} + +} + void ReadFromCluster::applyFilters(ActionDAGNodes added_filter_nodes) { SourceStepWithFilter::applyFilters(std::move(added_filter_nodes)); @@ -158,9 +206,12 @@ void ReadFromCluster::createExtension() if (extension) return; - const ActionsDAG * filter = listing_filter_dag - ? listing_filter_dag.get() - : (filter_actions_dag ? filter_actions_dag.get() : query_info.filter_actions_dag.get()); + /// In whole-query dispatch mode this step's output is the entire dispatched JOIN/aggregate query's + /// result, not the driver's raw columns -- any filter pushed down onto it (see the class comment) must + /// not be forwarded as a driver-table predicate for object-storage file-level pruning. + const ActionsDAG * filter = is_whole_query_dispatch + ? nullptr + : (listing_filter_dag ? listing_filter_dag.get() : (filter_actions_dag ? filter_actions_dag.get() : query_info.filter_actions_dag.get())); const ActionsDAG::Node * predicate = filter ? filter->getOutputs().at(0) : nullptr; extension = storage->getTaskIteratorExtension( predicate, @@ -415,6 +466,9 @@ void IStorageCluster::updateQueryWithJoinToSendIfNeeded( } case ObjectStorageClusterJoinMode::ALLOW: // Do nothing special return; + case ObjectStorageClusterJoinMode::DISTRIBUTED: + /// A whole-query dispatch never goes through read() -- see readPreparedClusterQuery() below. + return; } } @@ -548,6 +602,102 @@ void IStorageCluster::read( query_plan.addStep(std::move(reading)); } +/// Smaller sibling of read(): the caller already has query_to_send and sample_block, so no join-stripping, +/// no sample-block computation, and no RestoreQualifiedNamesVisitor (which assumes position 0). +void IStorageCluster::readPreparedClusterQuery( + QueryPlan & query_plan, + const Names & column_names, + const StorageSnapshotPtr & storage_snapshot, + SelectQueryInfo & query_info, + ContextPtr context, + QueryProcessingStage::Enum processed_stage, + ASTPtr query_to_send, + SharedHeader sample_block) +{ + auto cluster_name_from_settings = getClusterName(context); + const auto & settings = context->getSettingsRef(); + auto cluster = getClusterImpl(context, cluster_name_from_settings, isObjectStorage() ? settings[Setting::object_storage_max_nodes] : 0); + + AddDefaultDatabaseVisitor visitor(context, context->getCurrentDatabase(), + /* only_replace_current_database_function_= */false, + /* only_replace_in_join_= */true); + visitor.visit(query_to_send); + + auto this_ptr = std::static_pointer_cast(shared_from_this()); + + std::optional external_tables = std::nullopt; + if (query_info.planner_context && query_info.planner_context->getMutableQueryContext()) + external_tables = query_info.planner_context->getMutableQueryContext()->getExternalTables(); + + /// query_info.planner_context/table_expression describe the *whole* dispatched query here, not a single + /// per-table expression the way SourceStepWithFilter/applyFilters() expect: buildNodeNameToInputNodeColumn() + /// looks up query_info.table_expression in query_info.planner_context, which throws -- and dereferences a + /// null table_expression while formatting that very error -- if it's ever consulted. This step is not a + /// normal per-table Planner source, so drop them once external_tables above is captured; the driver's own + /// filter/task-iterator pruning is separately suppressed for is_whole_query_dispatch (see createExtension()). + query_info.planner_context.reset(); + query_info.table_expression.reset(); + + auto reading = std::make_unique( + column_names, + query_info, + storage_snapshot, + context, + sample_block, + std::move(this_ptr), + std::move(query_to_send), + processed_stage, + cluster, + log, + external_tables, + /*is_whole_query_dispatch_*/ true); + + query_plan.addStep(std::move(reading)); +} + +ASTPtr IStorageCluster::buildClusterTableFunctionAST( + const String & dispatch_cluster_name, const StorageSnapshotPtr & storage_snapshot, const ContextPtr & context) +{ + const auto & storage_id = getStorageID(); + ASTPtr identifier = storage_id.hasDatabase() + ? make_intrusive(storage_id.getDatabaseName(), storage_id.getTableName()) + : make_intrusive(storage_id.getTableName()); + + auto table_expression = make_intrusive(); + table_expression->database_and_table_name = identifier; + table_expression->children.push_back(identifier); + + auto tables_element = make_intrusive(); + tables_element->table_expression = table_expression; + tables_element->children.push_back(table_expression); + + auto tables = make_intrusive(); + tables->children.push_back(tables_element); + + auto select_query = make_intrusive(); + select_query->setExpression(ASTSelectQuery::Expression::TABLES, tables); + + ASTPtr query = select_query; + + /// updateQueryForDistributedEngineIfNeeded() (called via updateQueryToSendIfNeeded() below) resolves the + /// dispatch cluster via getClusterName(context), which itself prefers the query-level `object_storage_cluster` + /// setting -- scope that here on a throwaway context copy rather than relying on the real query's own + /// settings, since this may be called to build a driver replacement whose own cluster differs from + /// whatever the initiator's ambient context carries. + auto scoped_context = Context::createCopy(context); + scoped_context->setSetting("object_storage_cluster", dispatch_cluster_name); + + updateQueryToSendIfNeeded(query, storage_snapshot, scoped_context, /*make_cluster_function*/ true); + + auto * table_function = extractTableFunctionFromSelectQuery(query); + if (!table_function) + throw Exception(ErrorCodes::LOGICAL_ERROR, + "Distributed object-storage dispatch: failed to build an explicit cluster table function for {}", + storage_id.getNameForLogs()); + + return ASTPtr(table_function); +} + IStorageCluster::RemoteCallVariables IStorageCluster::convertToRemote( ClusterPtr cluster, ContextPtr context, @@ -655,6 +805,10 @@ void ReadFromCluster::initializePipeline(QueryPipelineBuilder & pipeline, const Pipes pipes; auto new_context = updateSettings(context->getSettingsRef()); const auto & current_settings = new_context->getSettingsRef(); + + /// Mirrors the context-level normalization in updateSettings() onto query_to_send's own query-level + /// SETTINGS clause, which would otherwise re-override it on the worker (see that function's comment). + sanitizeObjectStorageClusterQuerySettings(query_to_send, is_whole_query_dispatch); auto timeouts = ConnectionTimeouts::getTCPTimeoutsWithFailover(current_settings); size_t replica_index = 0; @@ -804,6 +958,21 @@ ContextPtr ReadFromCluster::updateSettings(const Settings & settings) /// Cluster table functions should always skip unavailable shards. new_settings[Setting::skip_unavailable_shards] = true; + /// Worker-localization scoping for object_storage_cluster_join_mode='distributed' (see + /// findDistributedObjectStorageCandidate.h): on the whole-query dispatch path + /// (readPreparedClusterQuery()), the driver is already rewritten into its own explicit `*Cluster()` call, + /// so any *other* DataLake-catalog table re-resolved on the worker (via DatabaseDataLake) must not itself + /// pick up a leftover `object_storage_cluster` from the initiator's session/query settings -- that setting + /// takes priority over a table's own configured cluster in StorageObjectStorageCluster::getClusterName(), + /// which would otherwise silently re-distribute a table this optimization already proved safe to + /// recompute in full, locally, on every worker. An ordinary (non-whole-query) ReadFromCluster reached + /// after the candidate was rejected must conversely behave exactly like `allow`, not leak 'distributed' + /// worker localization it never actually earned. + if (is_whole_query_dispatch) + new_settings[Setting::object_storage_cluster] = ""; + else if (new_settings[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) + new_settings[Setting::object_storage_cluster_join_mode] = ObjectStorageClusterJoinMode::ALLOW; + auto new_context = Context::createCopy(context); new_context->setSettings(new_settings); return new_context; diff --git a/src/Storages/IStorageCluster.h b/src/Storages/IStorageCluster.h index e316b1985330..9ade7b939670 100644 --- a/src/Storages/IStorageCluster.h +++ b/src/Storages/IStorageCluster.h @@ -53,6 +53,36 @@ class IStorageCluster : public IStorage QueryProcessingStage::Enum getQueryProcessingStage(ContextPtr, QueryProcessingStage::Enum, const StorageSnapshotPtr &, SelectQueryInfo &) const override; + /// Executes an already-prepared cluster query (see Planner/buildDistributedObjectStorageQueryPlan.h) + /// through the existing *Cluster() task-iterator protocol: resolves the cluster, default-database- + /// qualifies `query_to_send`, adds a single ReadFromCluster step. Unlike read(), does no query + /// preparation itself -- the caller has already produced a self-contained AST with the driver rewritten + /// into its explicit `*Cluster()` form, wherever it sits. + void readPreparedClusterQuery( + QueryPlan & query_plan, + const Names & column_names, + const StorageSnapshotPtr & storage_snapshot, + SelectQueryInfo & query_info, + ContextPtr context, + QueryProcessingStage::Enum processed_stage, + ASTPtr query_to_send, + SharedHeader sample_block); + + /// Builds a standalone, resolved explicit `*Cluster(cluster_name, ...)` AST function call for this exact + /// storage, reusing the same per-engine rewrite rules updateQueryToSendIfNeeded() applies to a real query + /// (credentials/structure/format arguments included) instead of reconstructing them here. Used by + /// buildDistributedObjectStorageQueryPlan.cpp to build the replacement for a driver TableNode via + /// IQueryTreeNode::cloneAndReplace() -- mirrors StorageDistributed::buildQueryTreeDistributed()'s own + /// exact-node replacement pattern. Does not mutate any query already in flight: builds and rewrites a + /// throwaway single-table SELECT of its own. + ASTPtr buildClusterTableFunctionAST(const String & dispatch_cluster_name, const StorageSnapshotPtr & storage_snapshot, const ContextPtr & context); + + /// Whether this storage is known to resolve identically/safely on every worker when re-resolved during a + /// SECONDARY_QUERY under object_storage_cluster_join_mode='distributed' -- used by + /// findDistributedObjectStorageCandidate() both for driver eligibility and non-driver JOIN-partner + /// safety. False by default; overridden by StorageObjectStorageCluster. + virtual bool isResolvedViaDataLakeCatalog() const { return false; } + bool isRemote() const final { return true; } bool supportsSubcolumns() const override { return true; } bool supportsOptimizationToSubcolumns() const override { return false; } @@ -149,7 +179,8 @@ class ReadFromCluster : public SourceStepWithFilter QueryProcessingStage::Enum processed_stage_, ClusterPtr cluster_, LoggerPtr log_, - std::optional external_tables_) + std::optional external_tables_, + bool is_whole_query_dispatch_ = false) : SourceStepWithFilter( std::move(sample_block), column_names_, @@ -162,6 +193,7 @@ class ReadFromCluster : public SourceStepWithFilter , cluster(std::move(cluster_)) , log(log_) , external_tables(external_tables_) + , is_whole_query_dispatch(is_whole_query_dispatch_) { } @@ -176,6 +208,13 @@ class ReadFromCluster : public SourceStepWithFilter std::shared_ptr listing_filter_dag; std::optional external_tables; + /// True only for the object_storage_cluster_join_mode='distributed' whole-query dispatch path + /// (readPreparedClusterQuery()): this step's own output represents the entire dispatched + /// JOIN/aggregate query, not one table, so a filter pushed down onto it by the optimizer describes + /// that output -- not a predicate over the driver's own raw columns -- and must never be handed to + /// getTaskIteratorExtension() for object-storage file-level pruning (see createExtension()). + bool is_whole_query_dispatch = false; + void createExtension(); ContextPtr updateSettings(const Settings & settings); }; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageCluster.h b/src/Storages/ObjectStorage/StorageObjectStorageCluster.h index 6894bb76d2e1..83ba8b98558a 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageCluster.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageCluster.h @@ -66,6 +66,11 @@ class StorageObjectStorageCluster : public IStorageCluster String getClusterName(ContextPtr context) const override; + /// True only for tables resolved through a shared DataLake catalog (set by DatabaseDataLake::tryGetTableImpl()), + /// not for an explicit engine table like `CREATE TABLE ... ENGINE = IcebergS3Cluster(...)`. + bool isResolvedViaDataLakeCatalog() const override { return resolved_via_datalake_catalog; } + void markResolvedViaDataLakeCatalog() { resolved_via_datalake_catalog = true; } + QueryProcessingStage::Enum getQueryProcessingStage(ContextPtr, QueryProcessingStage::Enum, const StorageSnapshotPtr &, SelectQueryInfo &) const override; std::optional distributedWrite( @@ -217,6 +222,7 @@ class StorageObjectStorageCluster : public IStorageCluster StorageObjectStorageConfigurationPtr configuration; const ObjectStoragePtr object_storage; bool cluster_name_in_settings; + bool resolved_via_datalake_catalog = false; /// non-clustered storage to fall back on pure realisation if needed std::shared_ptr pure_storage; From 7eb31dff1631e8f2689923e8a03ecb6957ef2934 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Wed, 16 Sep 2026 15:34:02 +0530 Subject: [PATCH 02/15] Simplify distributed JOIN dispatch, correct its docs and add tests Reuse mechanisms that already exist rather than adding parallel ones: `DatabaseCatalog::isDatalakeCatalog` in place of a per-storage marker, `StorageObjectStorageCluster::getClusterName` in place of a second worker-localization path in `DatabaseDataLake`, and `SourceStepWithFilterBase::applyFilters` in place of clearing fields on the `SelectQueryInfo` handed to `ReadFromCluster`. `DatabaseDataLake.cpp` is no longer touched by this feature at all. The setting's documentation described a search for the highest eligible enclosing query, and a fallback for the level an ineligible table appears at. Neither exists: dispatch is attempted only for the outermost `SELECT` of an initial query, and is all-or-nothing. It now says so, and states that only the driving table is partitioned while the rest is recomputed in full per node. `allWorkerLocalReferencesAreSafe` is renamed to `allWorkerLocalTableReferencesAreSafe`, because it proves nothing about ordinary functions: `dictGet`, a user-defined function or `hostName` move from the initiator to the workers unexamined, as they do for `Distributed`. Tests cover both shapes against a real `DataLake` catalog -- the driver as the JOIN's leftmost table, and the driver behind a subquery with the aggregation on the enclosing query and a nested JOIN on the right -- in derived-table and CTE spellings, comparing results against `object_storage_cluster_join_mode='allow'`. They assert that the whole query reaches a worker, that only the driver becomes a cluster function, and that no partner table fans out again from a worker. A local `Memory` JOIN partner must fall back to ordinary planning. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: VighneshPath --- src/Core/Settings.cpp | 10 +- src/Databases/DataLake/DatabaseDataLake.cpp | 15 -- src/Planner/Planner.cpp | 17 +- ...buildDistributedObjectStorageQueryPlan.cpp | 49 ++-- .../buildDistributedObjectStorageQueryPlan.h | 15 +- .../findDistributedObjectStorageCandidate.cpp | 121 ++++----- .../findDistributedObjectStorageCandidate.h | 37 ++- ...stributed_object_storage_join_dispatch.cpp | 89 +++++-- ...d_distributed_object_storage_candidate.cpp | 105 ++++++-- src/Storages/IStorageCluster.cpp | 52 ++-- src/Storages/IStorageCluster.h | 35 +-- .../StorageObjectStorageCluster.cpp | 13 + .../StorageObjectStorageCluster.h | 6 - .../integration/test_database_iceberg/test.py | 242 ++++++++++++++++++ .../test_cluster_joins.py | 2 +- 15 files changed, 553 insertions(+), 255 deletions(-) diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 324de519fd2b..5cebb435cc01 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -2138,7 +2138,15 @@ Possible values: - `local` — Replaces the database and table in the subquery with local ones for the destination server (shard), leaving the normal `IN`/`JOIN.` - `global` — Replaces the `IN`/`JOIN` query with `GLOBAL IN`/`GLOBAL JOIN.` Right table executes first and is added to the secondary query as temporay table. - `allow` — Default value. Allows the use of these types of subqueries. -- `distributed` — Experimental. Lets a `JOIN`'s leftmost table become the whole query's driver when it is a DataLake-catalog table distributed via `object_storage_cluster`, even when it sits inside a subquery or CTE: the highest enclosing query whose entire `JOIN`/subquery tree is safe (any `GROUP BY` on top included) is dispatched to that cluster's nodes and executed there, instead of pulling the driver's rows back to the initiator first. This only takes effect when every other table reachable in that tree also resolves through some DataLake catalog, has no row-level security policy of its own, and the current user has `SELECT` access to it (an explicit `*Cluster()` table function, an ordinary local/`Distributed` table, or any table failing one of those checks, falls back to ordinary (non-distributed) planning for the level it appears at); ClickHouse does not verify that such a table is configured identically on every node of the driver's cluster — that consistency is the deployment's responsibility. Falls back to ordinary planning entirely when `additional_table_filters` is set, or when `object_storage_remote_initiator` is enabled. +- `distributed` — Experimental. Dispatches a whole `JOIN` query to the cluster of its driving table, so the `JOIN` and any `GROUP BY` run on the cluster's nodes instead of on the initiator, which then only merges the partial aggregate states. The driving table is found by walking down the left side of the query -- through `INNER ALL`/`LEFT JOIN`s and through a subquery or CTE that does not itself aggregate, deduplicate, sort or limit -- and must be a `DataLake` catalog table distributed via `object_storage_cluster`. + + This is attempted only for the outermost `SELECT` of an initial query, and it is all-or-nothing: either the entire query is eligible and is dispatched as one unit, or ordinary planning handles the entire query. No narrower, nested candidate is attempted, and nothing falls back per level. + + Eligibility requires that every other table reachable anywhere in the query also resolves through a `DataLake` catalog, has no row-level security policy, and is readable by the current user. An explicit `*Cluster()` table function, an ordinary local or `Distributed` table, or any table failing one of those checks makes the whole query ineligible. The combination is disabled outright when `additional_table_filters` is set or `object_storage_remote_initiator` is enabled. + + Performance characteristic to be aware of: only the driving table is partitioned across the cluster. Every other table in the query is read and recomputed **in full on every node**, including any `JOIN` or `GROUP BY` over them. That is a win when those relations are small relative to the driver and a loss when they are not; ClickHouse does not estimate this cost when deciding to dispatch. + + ClickHouse also does not verify that the tables, dictionaries or user-defined functions the query references are configured identically on every node of the driver's cluster -- as with `Distributed`, that consistency is the deployment's responsibility. )", 0) \ \ DECLARE(UInt64, max_concurrent_queries_for_all_users, 0, R"( diff --git a/src/Databases/DataLake/DatabaseDataLake.cpp b/src/Databases/DataLake/DatabaseDataLake.cpp index 4ab46979edd9..ba2fc89c7799 100644 --- a/src/Databases/DataLake/DatabaseDataLake.cpp +++ b/src/Databases/DataLake/DatabaseDataLake.cpp @@ -107,7 +107,6 @@ namespace Setting extern const SettingsBool parallel_replicas_for_cluster_engines; extern const SettingsString cluster_for_parallel_replicas; extern const SettingsBool database_datalake_require_metadata_access; - extern const SettingsObjectStorageClusterJoinMode object_storage_cluster_join_mode; } @@ -752,18 +751,6 @@ StoragePtr DatabaseDataLake::tryGetTableImpl(const String & name, ContextPtr con if (cluster_name.empty() && can_use_parallel_replicas && !is_secondary_query) cluster_name = parallel_replicas_cluster_name; - /// Under object_storage_cluster_join_mode='distributed', a co-resolved DataLake table must localize - /// rather than dispatch its own ReadFromCluster (the driver itself is rewritten separately into an - /// explicit `*Cluster()` call before being sent to workers -- see findDistributedObjectStorageCandidate.h). - /// query_kind == SECONDARY_QUERY alone is too wide (also true for an unrelated Distributed/remote() query), - /// so mirror TableFunctionObjectStorageCluster's own worker-detection signal instead. - const auto & client_info = context_->getClientInfo(); - const bool is_distributed_object_storage_worker - = is_secondary_query && client_info.collaborate_with_initiator && context_->hasClusterFunctionReadTaskCallback(); - - if (is_distributed_object_storage_worker && query_settings[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) - cluster_name.clear(); - auto storage_cluster = std::make_shared( cluster_name, configuration, @@ -788,8 +775,6 @@ StoragePtr DatabaseDataLake::tryGetTableImpl(const String & name, ContextPtr con if (context_->hasQueryContext() && context_->getSettingsRef()[Setting::log_queries]) context_->getQueryContext()->addQueryFactoriesInfo(Context::QueryLogFactories::Storage, storage_cluster->getName()); - storage_cluster->markResolvedViaDataLakeCatalog(); - storage_cluster->startup(); return storage_cluster; } diff --git a/src/Planner/Planner.cpp b/src/Planner/Planner.cpp index 4f410851368f..e3d36a4b71f8 100644 --- a/src/Planner/Planner.cpp +++ b/src/Planner/Planner.cpp @@ -2304,13 +2304,16 @@ void Planner::buildPlanForQueryNode() } JoinTreeQueryPlan join_tree_query_plan; - /// object_storage_cluster_join_mode='distributed': only the outermost, initial-query Planner instance may - /// claim this optimization -- a nested Planner created for a subquery/CTE (select_query_options.is_subquery) - /// or a plain analysis pass (only_analyze) never does, and neither does a secondary-query Planner running on - /// a worker. findDistributedObjectStorageCandidate() itself never recurses into a narrower candidate, so - /// this is a whole-or-nothing decision for the outermost query alone: if it's not safe, ordinary planning - /// (including its own nested-Planner recursion for any subquery, see PlannerJoinTree.cpp's - /// buildQueryPlanForTableExpression()) handles the whole query. Independent of parallel replicas below. + /// object_storage_cluster_join_mode='distributed': only the outermost, initial-query Planner may dispatch. + /// All three guards below are load-bearing; each was added after a live failure: + /// - only_analyze: buildDistributedObjectStorageQueryPlan calls getSampleBlock, which starts its own + /// analyze-only Planner over the same query. Without this guard that Planner dispatches again, and so on + /// -- `Code: 306. TOO_DEEP_RECURSION`. + /// - is_subquery: a nested query dispatched on its own gets its `__tableN` identifiers renumbered locally, + /// which no longer match what the enclosing scope resolved against -- `Not found column + /// __table7.appinfo_ccl in block. There are only columns: __table2.appinfo_ccl, ...`. + /// - INITIAL_QUERY: a worker must execute what it was sent, not dispatch it onwards. + /// Whole-or-nothing for the outermost query: on rejection, ordinary planning handles everything. std::optional distributed_object_storage_candidate; if (!select_query_options.only_analyze && !select_query_options.is_subquery && query_context->getClientInfo().query_kind == ClientInfo::QueryKind::INITIAL_QUERY) diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp index b2aecf6af810..9c9fa718f384 100644 --- a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp @@ -28,6 +28,9 @@ namespace ErrorCodes extern const int LOGICAL_ERROR; } +/// This mirrors buildQueryPlanForParallelReplicas (Planner/findParallelReplicasQuery.cpp) step for step: +/// header of the original query -> rewrite the tree -> header of the rewritten tree -> serialize to SQL -> +/// remote read -> convert the remote header back to the original one by position. Keep the two in sync. JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( const QueryTreeNodePtr & dispatch_boundary_node, const DistributedObjectStorageCandidate & candidate, @@ -37,20 +40,18 @@ JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( const auto context = planner_context->getQueryContext(); constexpr auto processed_stage = QueryProcessingStage::WithMergeableState; - /// The header stock (unmodified) planning would have produced, computed against the query tree before - /// the driver is rewritten -- so downstream code (the caller's own finalization) sees exactly the column - /// names/types it would have without this optimization, matching buildQueryPlanForParallelReplicas()'s own - /// original-vs-worker header handling. + /// The header the unmodified query would have produced, so the caller's finalization sees the column + /// names/types it expects. auto initial_header = InterpreterSelectQueryAnalyzer::getSampleBlock( dispatch_boundary_node->clone(), context, SelectQueryOptions(processed_stage).analyze()); - /// Reuses the exact snapshot the analyzer resolved the driver against (TableNode owns it), rather than - /// fetching a fresh one here: metadata could otherwise have changed between analysis and dispatch, leaving - /// the dispatched query resolved against one snapshot and the replacement built from another. + /// Reuse the snapshot the analyzer resolved the driver against, so the dispatched query and its replacement + /// are built from the same metadata version. + auto * driver_storage = candidate.driver_storage; const auto & driver_storage_snapshot = candidate.driver->getStorageSnapshot(); - auto cluster_function_ast = candidate.driver_storage->buildClusterTableFunctionAST( - candidate.driver_storage->getClusterName(context), driver_storage_snapshot, context); + auto cluster_function_ast = driver_storage->buildClusterTableFunctionAST( + driver_storage->getClusterName(context), driver_storage_snapshot, context); auto cluster_function_query_tree = buildQueryTree(cluster_function_ast, context); auto & cluster_function_node = cluster_function_query_tree->as(); @@ -68,10 +69,8 @@ JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( query_analysis_pass.run(node, context); } - /// candidate.driver's own subtree is not traversed further -- it becomes a leaf, exact-node replacement, - /// mirroring StorageDistributed::buildQueryTreeDistributed()'s own pattern. cloneAndReplace() rebinds every - /// weak reference (e.g. ColumnNode source pointers) elsewhere in the tree from the old node to - /// `replacement`. + /// Exact-node replacement, as StorageDistributed::buildQueryTreeDistributed does. cloneAndReplace rebinds + /// every weak reference to the driver (e.g. ColumnNode sources) elsewhere in the tree. IQueryTreeNode::ReplacementMap replacement_map; replacement_map.emplace(candidate.driver, replacement); auto modified_query_tree = dispatch_boundary_node->cloneAndReplace(replacement_map); @@ -79,10 +78,8 @@ JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( auto [remote_header, new_planner_context] = InterpreterSelectQueryAnalyzer::getSampleBlockAndPlannerContext( modified_query_tree, context, SelectQueryOptions(processed_stage).analyze()); - /// Convert grouping function specializations (e.g. groupingForGroupingSets -> grouping) in a separate - /// clone so the AST sent to the driver's cluster contains the generic function name that can be - /// re-resolved by each worker's own analyzer -- modified_query_tree itself must keep the specialized - /// functions, since it was already used above for header computation and its planner context. + /// Strip grouping-function specializations in a separate clone: the workers re-resolve the generic function + /// themselves, but modified_query_tree must keep them, having already produced the header above. auto query_tree_for_ast = modified_query_tree->clone(); removeGroupingFunctionSpecializations(query_tree_for_ast); ASTPtr query_to_send = queryNodeToDistributedSelectQuery(query_tree_for_ast); @@ -90,11 +87,8 @@ JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( if (!query_to_send->as()) throw Exception(ErrorCodes::LOGICAL_ERROR, "Distributed object-storage dispatch: expected a plain SELECT at the dispatch boundary"); - /// SourceStepWithFilter::required_source_columns (and thus updatePrewhereInfo()'s own - /// driver_storage_snapshot->getSampleBlockForColumns(required_source_columns) lookup) is checked against - /// storage_snapshot, i.e. the driver's own snapshot here -- not the whole dispatched query's output schema - /// (that's remote_header, a separate concept). Use the driver's own physical columns, matching what a - /// normal per-table read() of the driver alone would pass. + /// SourceStepWithFilter checks required_source_columns against storage_snapshot, which here is the driver's. + /// Pass the driver's own physical columns, exactly as an ordinary per-table read would. Names column_names = driver_storage_snapshot->getColumns(GetColumnsOptions(GetColumnsOptions::AllPhysical)).getNames(); SelectQueryInfo query_info = select_query_info; @@ -105,7 +99,7 @@ JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( JoinTreeQueryPlan result; result.stage = processed_stage; - candidate.driver_storage->readPreparedClusterQuery( + driver_storage->readPreparedClusterQuery( result.query_plan, column_names, driver_storage_snapshot, @@ -115,12 +109,9 @@ JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( query_to_send, remote_header); - /// The remote result's header uses whatever column naming the rewritten/re-analyzed query produced; - /// convert it back, by position, to the header the unmodified query would have produced -- e.g. an - /// aggregate like sum() is still AggregateFunction(sum, ...) at this stage, not its finalized type, - /// matching buildQueryPlanForParallelReplicas()'s own original-vs-worker header conversion. Generic and - /// position-based rather than the previous per-projection-node ColumnNode renaming, which broke down for - /// a complex projection mixing CASE expressions over both JOIN sides with an aggregate. + /// The rewritten query numbers its tables independently, so the remote header's column names differ from the + /// original's (e.g. `__table1` vs `__table5`) even though the types line up. Rename by position, the same way + /// buildQueryPlanForParallelReplicas does. Aggregates are still AggregateFunction(...) at this stage. auto converting_actions = ActionsDAG::makeConvertingActions( result.query_plan.getCurrentHeader()->getColumnsWithTypeAndName(), initial_header->getColumnsWithTypeAndName(), diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.h b/src/Planner/buildDistributedObjectStorageQueryPlan.h index 20884a5bfe1c..c122696f14e5 100644 --- a/src/Planner/buildDistributedObjectStorageQueryPlan.h +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.h @@ -10,15 +10,12 @@ class PlannerContext; using PlannerContextPtr = std::shared_ptr; struct SelectQueryInfo; -/// Builds the whole-query dispatch plan for `candidate`: replaces `candidate.driver` in `dispatch_boundary_node` -/// (the exact QueryTreeNodePtr findDistributedObjectStorageCandidate() was called with) with a resolved, -/// explicit `*Cluster()` TableFunctionNode via IQueryTreeNode::cloneAndReplace() -- mirroring -/// StorageDistributed::buildQueryTreeDistributed()'s own exact-node replacement pattern -- serializes the -/// result, and executes it via IStorageCluster::readPreparedClusterQuery(): a single ReadFromCluster step at -/// WithMergeableState, so the caller's normal finalization applies unmodified on top (see -/// Planner::buildPlanForQueryNode()). A generic, position-based ActionsDAG::makeConvertingActions() converts -/// the remote result back to the header the unmodified `dispatch_boundary_node` would have produced, matching -/// buildQueryPlanForParallelReplicas()'s own original-vs-worker header handling. +/// Builds the whole-query dispatch plan for `candidate`: replaces the driver with an explicit, resolved +/// `*Cluster()` table function, serializes the result, and reads it back through a single ReadFromCluster step +/// at WithMergeableState, so the caller's normal finalization (MergingAggregated and the rest) applies on top. +/// +/// Structurally the same as buildQueryPlanForParallelReplicas in Planner/findParallelReplicasQuery.cpp, +/// including the position-based conversion back to the original query's header. JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( const QueryTreeNodePtr & dispatch_boundary_node, const DistributedObjectStorageCandidate & candidate, diff --git a/src/Planner/findDistributedObjectStorageCandidate.cpp b/src/Planner/findDistributedObjectStorageCandidate.cpp index 80e644c44d3b..2fa7bf646407 100644 --- a/src/Planner/findDistributedObjectStorageCandidate.cpp +++ b/src/Planner/findDistributedObjectStorageCandidate.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include namespace DB @@ -27,51 +28,41 @@ namespace Setting namespace { -/// A row-level security filter is normally attached to a table's own SelectQueryInfo during per-table -/// planning (PlannerJoinTree.cpp), keyed by that table's own catalog identity; every table admitted here, -/// driver or partner, is either rewritten away (the driver, into its explicit `*Cluster()` form) or -/// independently re-resolved by each worker's own DatabaseDataLake lookup (a partner) -- neither preserves or -/// safely re-derives the initiator user's own effective policy. Conservative: a nontrivial policy on any table -/// in the candidate subtree blocks this whole-query dispatch outright (see the setting's own documentation). +/// Dispatch drops the initiator's row policies: the driver is rewritten into an explicit `*Cluster()` call and +/// every partner is re-resolved independently by each worker, so neither carries the policy across. Reject. bool hasEffectiveRowPolicy(const TableNode & table_node, const ContextPtr & context) { const auto & storage_id = table_node.getStorageID(); - if (!storage_id.hasDatabase()) - return false; - - auto row_policy_filter = context->getRowPolicyFilter(storage_id.getDatabaseName(), storage_id.getTableName(), RowPolicyFilterType::SELECT_FILTER); - return row_policy_filter && !row_policy_filter->isAlwaysTrue(); + auto filter = context->getRowPolicyFilter( + storage_id.getDatabaseName(), storage_id.getTableName(), RowPolicyFilterType::SELECT_FILTER); + return filter && !filter->isAlwaysTrue(); } -/// Stock per-table planning (prepareBuildQueryPlanForTableExpression() in PlannerJoinTree.cpp) checks SELECT -/// access on every TableNode it plans, including ones buried in a subquery reached only under only_analyze via -/// its own separate check_subquery_table_access path. This whole-query dispatch replaces that per-table walk -/// entirely, so every table it admits -- driver or partner, at any depth -- needs the same check performed -/// here instead. A conservative, non-throwing, table-level (not column-level) check: missing access simply -/// falls back to ordinary planning, which enforces the real, precise access rules with its own error. +/// Dispatch replaces the per-table access check that `prepareBuildQueryPlanForTableExpression` would have run on +/// each table, so do it here instead. Table-level and non-throwing: a failure falls back to ordinary planning, +/// which then enforces the real column-level rules with its own error. bool hasSelectAccess(const TableNode & table_node, const ContextPtr & context) { const auto & storage_id = table_node.getStorageID(); - if (!storage_id.hasDatabase()) - return false; - - return context->getAccess()->isGranted(AccessType::SELECT, storage_id.getDatabaseName(), storage_id.getTableName()); + return context->getAccess()->isGranted( + AccessType::SELECT, storage_id.getDatabaseName(), storage_id.getTableName()); } -/// Whether `table_node` is trustworthy to include in the dispatch at all, as either the driver or a JOIN -/// partner / WHERE-HAVING-projection reference: a DataLake-catalog table (so every worker can independently -/// and identically re-resolve it -- see the setting's own documentation for what is and isn't verified here), -/// with no row policy that dispatch would silently drop, and visible to the current user. +/// Safe to include in the dispatch, as driver or as partner: resolved through a DataLake catalog, so every worker +/// re-resolves it identically; no row policy; visible to the user. bool isSafeDataLakeLeaf(const TableNode & table_node, const ContextPtr & context) { - auto * storage_cluster = dynamic_cast(table_node.getStorage().get()); - if (!storage_cluster || !storage_cluster->isResolvedViaDataLakeCatalog()) + const auto & storage_id = table_node.getStorageID(); + if (!storage_id.hasDatabase()) + return false; + + if (!dynamic_cast(table_node.getStorage().get())) return false; - if (hasEffectiveRowPolicy(table_node, context)) + if (!DatabaseCatalog::instance().isDatalakeCatalog(storage_id.getDatabaseName())) return false; - return hasSelectAccess(table_node, context); + return !hasEffectiveRowPolicy(table_node, context) && hasSelectAccess(table_node, context); } bool isEligibleDriver(const TableNode & table_node, const ContextPtr & context, IStorageCluster *& out_storage) @@ -79,16 +70,16 @@ bool isEligibleDriver(const TableNode & table_node, const ContextPtr & context, if (!isSafeDataLakeLeaf(table_node, context)) return false; - auto * storage_cluster = dynamic_cast(table_node.getStorage().get()); - if (storage_cluster->getClusterName(context).empty()) + auto * storage = dynamic_cast(table_node.getStorage().get()); + if (storage->getClusterName(context).empty()) return false; - out_storage = storage_cluster; + out_storage = storage; return true; } -/// True if `node`'s own subtree (not crossing into a nested QueryNode/UnionNode) contains an aggregate or -/// window function -- catches e.g. `SELECT count() FROM driver`, which has no GROUP BY node at all. +/// Aggregate/window function in this node's own subtree, not crossing into a nested query. Catches +/// `SELECT count() FROM driver`, which has no GROUP BY node at all. bool containsAggregateOrWindowFunction(const QueryTreeNodePtr & node) { if (!node) @@ -108,15 +99,10 @@ bool containsAggregateOrWindowFunction(const QueryTreeNodePtr & node) return false; } -/// The dispatch boundary itself may freely aggregate/order/limit -- WithMergeableState plus stock -/// finalization on top handles that correctly. But an *intermediate* QueryNode sitting between the dispatch -/// boundary and the driver (crossed via a nested subquery, strictly on the driver's own left path) gets -/// executed independently and completely on each worker's own partition of the driver; if it aggregates, -/// dedups, or limits, worker-local partial results get treated as final ones, which is wrong whenever a -/// group/row spans multiple workers' partitions. Conservative: reject any such construct here rather than try -/// to prove which ones happen to be partition-preserving. This never applies to a JOIN partner's own subquery -/// on the right of a JOIN -- that content is recomputed in full on every worker (see -/// allWorkerLocalReferencesAreSafe()), so GROUP BY/LIMIT/etc there is not a hazard at all. +/// A subquery crossed on the way down to the driver runs independently on each worker's own partition of the +/// driver, so anything that finalizes across rows (aggregation, DISTINCT, LIMIT, ...) would turn a partial result +/// into a final one whenever a group spans two workers. Reject rather than prove which ones are partition-safe. +/// Does not apply to a JOIN partner's subquery, which every worker recomputes in full. bool isSafeIntermediateSubquery(const QueryNode & query_node) { return !query_node.isDistinct() && !query_node.hasGroupBy() && !query_node.hasHaving() && !query_node.hasWindow() @@ -131,9 +117,8 @@ struct DriverPathResult const TableNode * driver = nullptr; IStorageCluster * driver_storage = nullptr; - /// Whether a supported JoinNode was found anywhere on the path down to the driver -- a candidate with no - /// JOIN at all has nothing for this mode to optimize, and dispatching it anyway would just replace stock - /// IStorageCluster::read() with a narrower prepared path. + /// No JOIN on the path means there is nothing here for this mode to optimize; stock `IStorageCluster::read` + /// already handles a plain single-table cluster read. bool has_join = false; }; @@ -144,10 +129,9 @@ DriverPathResult unusableDriverPath() return result; } -/// Walks strictly down the left spine looking for exactly one scheduling driver. Never inspects the right -/// side of a JOIN for a competing driver -- the right side is validated separately, as worker-local content -/// (see allWorkerLocalReferencesAreSafe()), which is why an RHS DataLake-catalog table, or a nested JOIN/ -/// GROUP BY over several such tables, never poisons or competes with the driver found here. +/// Walks strictly down the left spine for exactly one driver. The right side of a JOIN is never inspected here -- +/// it is validated separately as worker-local content by `allWorkerLocalTableReferencesAreSafe`, which is why a +/// DataLake table (or a whole nested JOIN/GROUP BY) on the right never competes for the driver role. DriverPathResult findDriverOnLeftSpine(const QueryTreeNodePtr & node, const ContextPtr & context) { if (const auto * table_node = node->as()) @@ -172,8 +156,8 @@ DriverPathResult findDriverOnLeftSpine(const QueryTreeNodePtr & node, const Cont if (result.unusable) return result; - /// `node` is crossed as an intermediate subquery here, not the dispatch boundary itself (that's the - /// QueryNode originally passed to findDistributedObjectStorageCandidate()). + /// Crossed as an intermediate subquery, not as the dispatch boundary (that is the node originally passed + /// to findDistributedObjectStorageCandidate). if (!isSafeIntermediateSubquery(*query_node)) return unusableDriverPath(); @@ -198,14 +182,15 @@ DriverPathResult findDriverOnLeftSpine(const QueryTreeNodePtr & node, const Cont return unusableDriverPath(); } -/// After a driver is found, every other table reachable anywhere in the whole dispatch-boundary subtree -- -/// on the right of any JOIN, nested arbitrarily deep in a JOIN/GROUP BY of its own, or referenced from a -/// WHERE/HAVING/projection subquery -- must be safe to recompute in full, identically, on every worker: a -/// DataLake-catalog table with no row policy of its own and visible to the current user. This walk does not -/// classify anything as another driver and does not restrict GROUP BY/JOIN/LIMIT anywhere in this content -- -/// unlike the driver's own left-spine path, it is never partitioned, so each worker simply recomputes it -/// whole (see the setting's own documentation and findDistributedObjectStorageCandidate.h). -bool allWorkerLocalReferencesAreSafe(const QueryTreeNodePtr & node, const TableNode * driver, const ContextPtr & context) +/// Everything else reachable from the dispatch boundary is recomputed in full on every worker, so every table it +/// reaches must re-resolve identically there. No structural restrictions apply here (unlike the driver's own +/// path), because none of this content is partitioned. +/// +/// Proves this for table references only. Ordinary FunctionNodes are accepted unexamined, so anything the query +/// calls that is server-local or externally backed -- `hostName`, a dictionary via `dictGet`, a user-defined +/// function -- moves from the initiator to the workers and must be present and consistent across the cluster. +/// Same assumption `Distributed` makes; stated in the setting's own documentation. +bool allWorkerLocalTableReferencesAreSafe(const QueryTreeNodePtr & node, const TableNode * driver, const ContextPtr & context) { if (!node) return true; @@ -217,7 +202,7 @@ bool allWorkerLocalReferencesAreSafe(const QueryTreeNodePtr & node, const TableN return false; for (const auto & child : node->getChildren()) - if (!allWorkerLocalReferencesAreSafe(child, driver, context)) + if (!allWorkerLocalTableReferencesAreSafe(child, driver, context)) return false; return true; @@ -231,16 +216,14 @@ std::optional findDistributedObjectStorageCan if (context->getSettingsRef()[Setting::object_storage_cluster_join_mode] != ObjectStorageClusterJoinMode::DISTRIBUTED) return {}; - /// readPreparedClusterQuery() goes straight to the driver's own cluster, bypassing the remote-initiator - /// topology (convertToRemote()) that stock IStorageCluster::read() applies for this setting; falls back to - /// ordinary planning instead of silently ignoring it. + /// Dispatch goes straight to the driver's cluster, bypassing the remote-initiator topology that + /// `IStorageCluster::read` would apply via convertToRemote. if (context->getSettingsRef()[Setting::object_storage_remote_initiator]) return {}; - /// additional_table_filters keys are matched against the initiator's current_database and the query's own - /// aliasing/naming, both of which shift once forwarded as fully serialized remote SQL -- Planner.cpp - /// disables parallel replicas for the exact same reason (see the comment there). Rather than replicate - /// case-by-case matching here, disable the combination entirely, same as that precedent. + /// additional_table_filters is keyed by the initiator's current_database and the query's own naming, both of + /// which shift once the query is serialized for remote execution. Planner.cpp disables parallel replicas for + /// the same reason. if (!context->getSettingsRef()[Setting::additional_table_filters].value.empty()) return {}; @@ -256,7 +239,7 @@ std::optional findDistributedObjectStorageCan if (driver_path.unusable || !driver_path.driver || !driver_path.has_join) return {}; - if (!allWorkerLocalReferencesAreSafe(query_node, driver_path.driver, context)) + if (!allWorkerLocalTableReferencesAreSafe(query_node, driver_path.driver, context)) return {}; DistributedObjectStorageCandidate candidate; diff --git a/src/Planner/findDistributedObjectStorageCandidate.h b/src/Planner/findDistributedObjectStorageCandidate.h index 9ab626f16ab8..ee48ed7e4f1a 100644 --- a/src/Planner/findDistributedObjectStorageCandidate.h +++ b/src/Planner/findDistributedObjectStorageCandidate.h @@ -15,35 +15,30 @@ using QueryTreeNodePtr = std::shared_ptr; class Context; using ContextPtr = std::shared_ptr; -/// A whole-query JOIN-pushdown candidate for object_storage_cluster_join_mode='distributed'. `query_node` -/// (the exact QueryTreeNodePtr passed to findDistributedObjectStorageCandidate()) is always the dispatch -/// boundary: the entire query is forwarded as a whole to `driver`'s cluster, never a narrower subquery. +/// A whole-query JOIN-pushdown candidate for `object_storage_cluster_join_mode='distributed'`. The entire query +/// passed to findDistributedObjectStorageCandidate is dispatched to `driver`'s cluster as one unit. struct DistributedObjectStorageCandidate { - /// The driving TableNode, reachable from the dispatch boundary only via the left path of every - /// JOIN/subquery crossing (see findDriverOnLeftSpine() in the .cpp). + /// The driving table, reachable from the dispatch boundary only via the left path of every JOIN/subquery + /// crossing. const TableNode * driver = nullptr; - /// driver's resolved storage. IStorageCluster * driver_storage = nullptr; }; -/// Whole-query dispatch is an all-or-nothing decision for `query_node` itself: either the entire subtree -/// reachable from it is safe to forward as one query to a single DataLake-catalog driver's cluster, or it -/// isn't and the caller falls back to ordinary planning for this exact QueryNode -- there is no narrower -/// fallback candidate search. Returns nullopt when: mode isn't 'distributed', `query_node` has no JOIN at -/// all (nothing here for this mode to optimize), no eligible driver is reachable via the left spine, or any -/// unsafe leaf is reachable anywhere in the subtree (explicit `*Cluster()`, local/Distributed table, -/// row policy, missing SELECT access, or a structurally unsupported shape). +/// Decides whether `query_node` as a whole can be executed on a single DataLake-catalog driver's cluster. /// -/// The driver is found by walking strictly down the left spine of `query_node`'s own JOIN/subquery tree: -/// QueryNode -> its join tree; supported JoinNode (INNER ALL or LEFT) -> left operand only; intermediate -/// QueryNode crossed along the way -> only if partition-preserving (see isSafeIntermediateSubquery() in the -/// .cpp); TableNode -> an eligible DataLake-catalog driver with a non-empty cluster. The right side of any -/// JOIN, and anything below it, is never inspected for a competing driver -- it is validated only as -/// worker-local, safe-to-recompute-in-full content (see allWorkerLocalReferencesAreSafe() in the .cpp), which -/// is why a DataLake-catalog table, or even a nested JOIN/GROUP BY over several such tables, is accepted on -/// the right without ever being considered for the driver role itself. +/// The driver is found by walking strictly down the left spine: QueryNode -> its join tree; INNER ALL or LEFT +/// JOIN -> left operand only; an intermediate QueryNode -> only if partition-preserving; TableNode -> eligible if +/// it resolves through a DataLake catalog and has a non-empty cluster. Everything else reachable from +/// `query_node` is then checked as worker-local content that each worker recomputes in full. That check covers +/// table references only -- other functions the query calls (`dictGet`, a UDF, `hostName`) simply move to the +/// workers and are assumed to be consistent there, as they are for `Distributed`. +/// +/// All-or-nothing for `query_node` itself -- there is no narrower fallback candidate. Returns nullopt when the +/// mode is not 'distributed', there is no JOIN, no eligible driver is reachable, or any unsafe leaf appears +/// anywhere in the subtree (explicit `*Cluster()` table function, local/`Distributed` table, row policy, missing +/// SELECT access, unsupported shape). The caller then falls back to ordinary planning. std::optional findDistributedObjectStorageCandidate( const QueryTreeNodePtr & query_node, const ContextPtr & context); diff --git a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp index 11baeddb296d..530a06209bf7 100644 --- a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp +++ b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -73,6 +74,23 @@ class FakeDriverTableFunction : public ITableFunction const char * getStorageEngineName() const override { return "FakeDriverStorage"; } }; +/// A DatabaseMemory that reports itself as a DataLake catalog: that is what makes the tables inside it eligible +/// for dispatch (findDistributedObjectStorageCandidate asks DatabaseCatalog::isDatalakeCatalog). +class FakeDataLakeDatabase : public DatabaseWithOwnTablesBase +{ +public: + explicit FakeDataLakeDatabase(const String & name_, ContextPtr context_) + : DatabaseWithOwnTablesBase(name_, "FakeDataLakeDatabase(" + name_ + ")", context_) + { + } + + String getEngineName() const override { return "FakeDataLakeCatalog"; } + bool isDatalakeCatalog() const override { return true; } + +private: + ASTPtr getCreateDatabaseQueryImpl() const override { return nullptr; } +}; + /// Minimal driver stand-in; getTaskIteratorExtension() is only invoked during real pipeline execution, /// which these tests never trigger -- they only check the plan. class FakeDriverStorage : public IStorageCluster @@ -87,7 +105,6 @@ class FakeDriverStorage : public IStorageCluster } std::string getName() const override { return "FakeDriverStorage"; } - bool isResolvedViaDataLakeCatalog() const override { return true; } RemoteQueryExecutor::Extension getTaskIteratorExtension( const ActionsDAG::Node *, const ActionsDAG *, const ContextPtr &, ClusterPtr, StorageMetadataPtr) const override @@ -137,7 +154,6 @@ class FakeSafeLookupStorage : public IStorageCluster } std::string getName() const override { return "FakeSafeLookupStorage"; } - bool isResolvedViaDataLakeCatalog() const override { return true; } RemoteQueryExecutor::Extension getTaskIteratorExtension( const ActionsDAG::Node *, const ActionsDAG *, const ContextPtr &, ClusterPtr, StorageMetadataPtr) const override @@ -211,7 +227,7 @@ struct State static constexpr auto database_name = "distributed_object_storage_join_dispatch_test_db"; static constexpr auto cluster_name = "vig-test"; - DatabasePtr database = std::make_shared(database_name, context); + DatabasePtr database = std::make_shared(database_name, context); driver = std::make_shared(StorageID(database_name, "driver"), cluster_name); database->attachTable(context, "driver", driver, {}); @@ -252,7 +268,7 @@ String planAndExplain(const String & query, const ContextMutablePtr & context) /// Like planAndExplain(), but also runs the query plan optimizer (predicate pushdown included) before /// explaining -- this is what actually drives ReadFromCluster::applyFilters() during a real EXPLAIN/execution, -/// which planAndExplain() alone never touches. Needed to reproduce the live q17 crash trigger: a WHERE on the +/// which planAndExplain() alone never touches. Needed to reproduce a live exception: a WHERE on the /// driver gets pushed down as a filter onto the whole-query ReadFromCluster step, whose SelectQueryInfo used to /// carry a mismatched planner_context/table_expression pair for a per-table filter lookup that this step isn't. String planOptimizeAndExplain(const String & query, const ContextMutablePtr & context) @@ -289,7 +305,8 @@ ReadFromCluster * findReadFromCluster(QueryPlan::Node * node) } -/// Core "q17" regression: whole query dispatches as one ReadFromCluster step, no local JOIN. +/// Core case: the driver is the JOIN's leftmost table, and the whole query dispatches as one +/// ReadFromCluster step with no local JOIN. TEST(DistributedObjectStorageJoinDispatch, DriverOwnsWholeJoinWhenModeIsDistributed) { auto & state = State::instance(); @@ -343,7 +360,7 @@ TEST(DistributedObjectStorageJoinDispatch, DriverIsWrappedWhenModeIsNotDistribut EXPECT_NE(plan_text.find("JoinLogical"), String::npos) << "expected a local JOIN step, got:\n" << plan_text; } -/// "q21" regression: driver buried in a subquery, outer JOIN + GROUP BY all dispatch as one ReadFromCluster, +/// Driver buried in a subquery: the outer JOIN and GROUP BY still dispatch as one ReadFromCluster, /// with stock MergingAggregated finalization reused on top. TEST(DistributedObjectStorageJoinDispatch, BuriedDriverOwnsWholeOuterQueryWithGroupBy) { @@ -392,10 +409,10 @@ TEST(DistributedObjectStorageJoinDispatch, ReadFromClusterHeaderCarriesUnmergedA << "expected ReadFromCluster's header to carry the unmerged aggregate state type, got:\n" << read_from_cluster_block; } -/// Real "q17"-shaped projection: a CASE expression reading columns from both sides of the JOIN, plus count(), +/// A realistic projection: a CASE expression reading columns from both sides of the JOIN, plus count(), /// GROUP BY, ORDER BY and LIMIT all on the dispatch boundary itself. Exercises the header/rename machinery /// with more than one plain passthrough column, unlike the trivial single-column projections above. -TEST(DistributedObjectStorageJoinDispatch, ComplexQ17ProjectionBuildsSuccessfully) +TEST(DistributedObjectStorageJoinDispatch, ComplexProjectionOverBothJoinSidesBuildsSuccessfully) { auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); @@ -417,14 +434,14 @@ TEST(DistributedObjectStorageJoinDispatch, ComplexQ17ProjectionBuildsSuccessfull << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; } -/// Regression for the live q17 SIGSEGV: a WHERE on the driver survives real plan optimization (not just +/// Regression for a live exception: a WHERE on the driver survives real plan optimization (not just /// candidate discovery/plan construction), which pushes it down as a filter onto the whole-query /// ReadFromCluster step and calls ReadFromCluster::applyFilters() -> SourceStepWithFilter::applyFilters() -> -/// SelectQueryInfo::buildNodeNameToInputNodeColumn(). That used to look up a per-table `table_expression` in a -/// `planner_context` describing the *whole* dispatched query, throwing while formatting the error message by -/// dereferencing a null table_expression. readPreparedClusterQuery() must not leave that pair set on the -/// SelectQueryInfo it hands to ReadFromCluster. -TEST(DistributedObjectStorageJoinDispatch, ComplexQ17ProjectionWithWhereSurvivesPlanOptimization) +/// SelectQueryInfo::buildNodeNameToInputNodeColumn(), which looks up a per-table `table_expression` in a +/// `planner_context` that here describes the *whole* dispatched query -- it throws, and used to dereference a +/// null table_expression while formatting that very error. In whole-query mode applyFilters must therefore use +/// SourceStepWithFilterBase::applyFilters, which does not build that per-table mapping at all. +TEST(DistributedObjectStorageJoinDispatch, ComplexProjectionWithWhereSurvivesPlanOptimization) { auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); @@ -444,11 +461,11 @@ TEST(DistributedObjectStorageJoinDispatch, ComplexQ17ProjectionWithWhereSurvives EXPECT_EQ(plan_text.find("JoinLogical"), String::npos) << "expected no local JOIN step, got:\n" << plan_text; } -/// Real "q21" shape: driver behind one intermediate subquery, outer LEFT JOIN against a RHS subquery that +/// The full buried-driver shape: driver behind one intermediate subquery, outer LEFT JOIN against a RHS subquery that /// itself LEFT JOINs a further GROUP BY subquery and also has its own GROUP BY. None of that RHS content is a /// competing driver -- it's recomputed whole on every worker -- and the whole thing must still build into a /// single dispatched ReadFromCluster with stock finalization for the outer GROUP BY on top. -TEST(DistributedObjectStorageJoinDispatch, RealQ21ShapeBuildsSuccessfully) +TEST(DistributedObjectStorageJoinDispatch, BuriedDriverWithNestedRightSideBuildsSuccessfully) { auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); @@ -470,10 +487,10 @@ TEST(DistributedObjectStorageJoinDispatch, RealQ21ShapeBuildsSuccessfully) << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; } -/// The same q21 shape, but asserting the rewrite itself: exactly one driver -- `driver`, buried inside +/// The same shape, but asserting the rewrite itself: exactly one driver -- `driver`, buried inside /// `transaction_event` -- becomes the explicit cluster function; every other DataLake table reachable from the /// RHS (`safe_lookup`, `dim2`) stays an ordinary catalog identifier, never itself rewritten into a driver. -TEST(DistributedObjectStorageJoinDispatch, RealQ21ShapeRewritesOnlyTheBuriedDriver) +TEST(DistributedObjectStorageJoinDispatch, RewritesOnlyTheBuriedDriverAndNoPartner) { auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); @@ -498,6 +515,42 @@ TEST(DistributedObjectStorageJoinDispatch, RealQ21ShapeRewritesOnlyTheBuriedDriv EXPECT_NE(plan_text.find("dim2"), String::npos) << "expected dim2 to remain an ordinary catalog identifier, got:\n" << plan_text; } +/// The same shape written with CTEs rather than derived tables. The setting documents CTE support, and the +/// analyzer resolves a CTE into a QueryNode just as it does a derived table -- but a CTE reference serializes to +/// its bare name unless the body is inlined, which would not resolve on a worker. +/// queryNodeToDistributedSelectQuery is what inlines it; this pins that down, including for a CTE +/// (`policy_matches`) referenced from inside another CTE. +TEST(DistributedObjectStorageJoinDispatch, BuriedDriverWithCommonTableExpressions) +{ + auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto plan_text = planAndExplain( + "WITH transaction_event AS (SELECT driver.id FROM driver), " + "policy_matches AS (SELECT dim2.lookup_id AS id FROM dim2 GROUP BY dim2.lookup_id), " + "alert_events AS (SELECT safe_lookup.lookup_id AS id FROM safe_lookup " + "LEFT JOIN policy_matches ON safe_lookup.lookup_id = policy_matches.id " + "GROUP BY safe_lookup.lookup_id) " + "SELECT transaction_event.id, count() FROM transaction_event " + "LEFT JOIN alert_events ON transaction_event.id = alert_events.id " + "GROUP BY transaction_event.id", + state.context); + + EXPECT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; + EXPECT_EQ(plan_text.find("JoinLogical"), String::npos) << "expected no local JOIN step, got:\n" << plan_text; + EXPECT_NE(plan_text.find("MergingAggregated"), String::npos) + << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; + + /// Exactly one driver, and no dangling CTE name: every CTE body must appear inlined in the forwarded query. + size_t driver_function_count = 0; + for (size_t pos = plan_text.find("fakeDriverFunction("); pos != String::npos; pos = plan_text.find("fakeDriverFunction(", pos + 1)) + ++driver_function_count; + EXPECT_EQ(driver_function_count, 1u) << "expected exactly one explicit driver cluster function, got:\n" << plan_text; + + EXPECT_NE(plan_text.find("safe_lookup"), String::npos) << plan_text; + EXPECT_NE(plan_text.find("dim2"), String::npos) << plan_text; +} + /// SourceStepWithFilter::required_source_columns is checked against the driver's own StorageSnapshot /// (updatePrewhereInfo() calls storage_snapshot->getSampleBlockForColumns(required_source_columns)) -- it must /// be the driver's physical columns, never the whole dispatched query's own output projection (that's a diff --git a/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp b/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp index ddea6fac890e..683cd5e04d79 100644 --- a/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp +++ b/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -37,13 +38,29 @@ NamesAndTypesList testColumns() return {{"id", std::make_shared()}}; } +/// A DatabaseMemory that reports itself as a DataLake catalog, which is what makes the tables inside it +/// eligible (findDistributedObjectStorageCandidate asks DatabaseCatalog::isDatalakeCatalog). +class FakeDataLakeDatabase : public DatabaseWithOwnTablesBase +{ +public: + explicit FakeDataLakeDatabase(const String & name_, ContextPtr context_) + : DatabaseWithOwnTablesBase(name_, "FakeDataLakeDatabase(" + name_ + ")", context_) + { + } + + String getEngineName() const override { return "FakeDataLakeCatalog"; } + bool isDatalakeCatalog() const override { return true; } + +private: + ASTPtr getCreateDatabaseQueryImpl() const override { return nullptr; } +}; + /// Minimal IStorageCluster test double, configurable per instance. class FakeClusterStorage : public IStorageCluster { public: - FakeClusterStorage(const StorageID & table_id, String cluster_name_, bool resolved_via_datalake_catalog_) + FakeClusterStorage(const StorageID & table_id, String cluster_name_) : IStorageCluster(cluster_name_, table_id, getLogger("test")) - , resolved_via_datalake_catalog(resolved_via_datalake_catalog_) { StorageInMemoryMetadata metadata; metadata.setColumns(ColumnsDescription{testColumns()}); @@ -51,16 +68,12 @@ class FakeClusterStorage : public IStorageCluster } std::string getName() const override { return "FakeClusterStorage"; } - bool isResolvedViaDataLakeCatalog() const override { return resolved_via_datalake_catalog; } RemoteQueryExecutor::Extension getTaskIteratorExtension( const ActionsDAG::Node *, const ActionsDAG *, const ContextPtr &, ClusterPtr, StorageMetadataPtr) const override { return {}; } - -private: - bool resolved_via_datalake_catalog; }; /// Modelled on src/Planner/tests/gtest_planner_empty_projection.cpp. @@ -84,26 +97,31 @@ struct State tryRegisterAggregateFunctions(); static constexpr auto database_name = "find_distributed_object_storage_candidate_test_db"; - DatabasePtr database = std::make_shared(database_name, context); + DatabasePtr database = std::make_shared(database_name, context); - auto attach_cluster_table = [&](const String & table_name, String cluster_name, bool resolved_via_datalake_catalog) + auto attach_cluster_table = [&](const DatabasePtr & db, const String & table_name, String cluster_name) { - database->attachTable( + db->attachTable( context, table_name, - std::make_shared(StorageID(database_name, table_name), std::move(cluster_name), resolved_via_datalake_catalog), + std::make_shared(StorageID(db->getDatabaseName(), table_name), std::move(cluster_name)), {}); }; /// A distributed driver, e.g. ice.event_page. - attach_cluster_table("driver", "vig-test", /*resolved_via_datalake_catalog=*/true); + attach_cluster_table(database, "driver", "vig-test"); /// A second DataLake-catalog table under the same cluster -- never an independent driver on the /// right of a JOIN, just a plain (if wasteful) safe partner. - attach_cluster_table("second_datalake_table", "vig-test", /*resolved_via_datalake_catalog=*/true); + attach_cluster_table(database, "second_datalake_table", "vig-test"); /// A safe co-resolved table with no cluster dispatch of its own (e.g. ice.geo_location_lookup). - attach_cluster_table("safe_lookup", "", /*resolved_via_datalake_catalog=*/true); - /// A Cluster-engine table not resolved through DatabaseDataLake -- not safe. - attach_cluster_table("unsafe_cluster_table", "some-cluster", /*resolved_via_datalake_catalog=*/false); + attach_cluster_table(database, "safe_lookup", ""); + + /// A Cluster-engine table in an ordinary database -- not resolved through a DataLake catalog, so not + /// safe to re-resolve on a worker. + static constexpr auto plain_database_name = "find_distributed_object_storage_candidate_test_plain_db"; + DatabasePtr plain_database = std::make_shared(plain_database_name, context); + attach_cluster_table(plain_database, "unsafe_cluster_table", "some-cluster"); + DatabaseCatalog::instance().attachDatabase(plain_database->getDatabaseName(), plain_database); database->attachTable( context, @@ -256,7 +274,7 @@ TEST(FindDistributedObjectStorageCandidate, RejectsDriverWithoutSelectAccess) EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, context).has_value()); } -/// Same access check, but the table without SELECT access is buried inside a subquery ("q21" shape) rather +/// Same access check, but the table without SELECT access is buried inside a subquery rather /// than at the dispatch boundary's own top-level JOIN -- the recursive intermediate-subquery walk must reach /// it too, not just the tables directly visible at the outermost level. TEST(FindDistributedObjectStorageCandidate, RejectsBuriedDriverWithoutSelectAccess) @@ -334,7 +352,7 @@ TEST(FindDistributedObjectStorageCandidate, RejectsUnsafeClusterTableAsDriver) state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); auto query_tree - = analyze("SELECT unsafe_cluster_table.id FROM unsafe_cluster_table INNER JOIN safe_lookup ON unsafe_cluster_table.id = safe_lookup.id", state.context); + = analyze("SELECT t.id FROM find_distributed_object_storage_candidate_test_plain_db.unsafe_cluster_table AS t INNER JOIN safe_lookup ON t.id = safe_lookup.id", state.context); EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); } @@ -344,7 +362,7 @@ TEST(FindDistributedObjectStorageCandidate, RejectsUnsafeIStorageClusterTableAsR state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); auto query_tree - = analyze("SELECT driver.id FROM driver INNER JOIN unsafe_cluster_table ON driver.id = unsafe_cluster_table.id", state.context); + = analyze("SELECT driver.id FROM driver INNER JOIN find_distributed_object_storage_candidate_test_plain_db.unsafe_cluster_table AS t ON driver.id = t.id", state.context); EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); } @@ -476,14 +494,14 @@ TEST(FindDistributedObjectStorageCandidate, RejectsExplicitClusterTableFunctionA auto table_function_node = std::make_shared("icebergS3Cluster"); auto explicit_cluster_storage = std::make_shared( - StorageID("system", "explicit_cluster_table"), "other-cluster", /*resolved_via_datalake_catalog=*/false); + StorageID("system", "explicit_cluster_table"), "other-cluster"); table_function_node->resolve(nullptr, explicit_cluster_storage, state.context, {}); join_node.getRightTableExpression() = table_function_node; EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); } -/// "q21" shape: driver buried in a subquery joined against another safe table; outer query is the candidate. +/// Driver buried in a subquery joined against another safe table; the outer query is the candidate. TEST(FindDistributedObjectStorageCandidate, AcceptsBuriedDriverWithSafeOuterJoin) { const auto & state = State::instance(); @@ -515,13 +533,13 @@ TEST(FindDistributedObjectStorageCandidate, RejectsWhenOuterJoinPartnerIsUnsafe) EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); } -/// Real "q21" shape: the driver sits behind one intermediate subquery (`transaction_event`, standing in for +/// The full buried-driver shape: the driver sits behind one intermediate subquery (`transaction_event`, standing in for /// `txnlog`) on the left of the outer LEFT JOIN; the right side (`alert_events`) is itself a LEFT JOIN against /// a further subquery with its own GROUP BY (`policy_matches`), and `alert_events` itself also has a GROUP BY. /// None of that RHS structure is inspected for a competing driver or restricted for GROUP BY/JOIN -- it's /// worker-local content, recomputed whole on every worker. The outer query's own GROUP BY/ORDER BY/LIMIT are /// the dispatch boundary's own, handled by stock finalization on top of the dispatched read. -TEST(FindDistributedObjectStorageCandidate, AcceptsRealQ21Shape) +TEST(FindDistributedObjectStorageCandidate, AcceptsBuriedDriverWithNestedRightSide) { const auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); @@ -544,10 +562,49 @@ TEST(FindDistributedObjectStorageCandidate, AcceptsRealQ21Shape) EXPECT_EQ(candidate->driver->getStorageID().table_name, "driver"); } -/// Real "q17" shape: one root LEFT JOIN between the driver and a safe lookup table, with WHERE, GROUP BY, +/// Direct-driver shape: one root LEFT JOIN between the driver and a safe lookup table, with WHERE, GROUP BY, /// ORDER BY and LIMIT all sitting directly on the dispatch boundary itself (not an intermediate subquery) -- /// freely allowed there, unlike on a driver-path intermediate subquery. -TEST(FindDistributedObjectStorageCandidate, AcceptsRealQ17Shape) +/// The buried-driver shape expressed with CTEs: the driver sits inside a CTE used as the JOIN's left side. The analyzer +/// resolves a CTE reference into the same QueryNode a derived table would produce, so the left-spine walk must +/// find the driver through it exactly as it does through a subquery. +TEST(FindDistributedObjectStorageCandidate, AcceptsBuriedDriverInsideCommonTableExpression) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "WITH transaction_event AS (SELECT driver.id FROM driver), " + "alert_events AS (SELECT safe_lookup.id FROM safe_lookup) " + "SELECT transaction_event.id, count() FROM transaction_event " + "LEFT JOIN alert_events ON transaction_event.id = alert_events.id " + "GROUP BY transaction_event.id", + state.context); + + auto candidate = findDistributedObjectStorageCandidate(query_tree, state.context); + ASSERT_TRUE(candidate.has_value()); + EXPECT_EQ(candidate->driver->getStorageID().getTableName(), "driver"); +} + +/// A CTE on the driver's own left path is still an intermediate subquery: if it aggregates, each worker would +/// finalize its own slice of the driver as though it were the whole group. +TEST(FindDistributedObjectStorageCandidate, RejectsAggregatingCommonTableExpressionOnDriverPath) +{ + const auto & state = State::instance(); + state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); + + auto query_tree = analyze( + "WITH transaction_event AS (SELECT driver.id FROM driver GROUP BY driver.id), " + "alert_events AS (SELECT safe_lookup.id FROM safe_lookup) " + "SELECT transaction_event.id, count() FROM transaction_event " + "LEFT JOIN alert_events ON transaction_event.id = alert_events.id " + "GROUP BY transaction_event.id", + state.context); + + EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, state.context).has_value()); +} + +TEST(FindDistributedObjectStorageCandidate, AcceptsDirectDriverWithFilterAndAggregation) { const auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); diff --git a/src/Storages/IStorageCluster.cpp b/src/Storages/IStorageCluster.cpp index 77a15cfb33c8..ebf32d53a0c9 100644 --- a/src/Storages/IStorageCluster.cpp +++ b/src/Storages/IStorageCluster.cpp @@ -140,10 +140,8 @@ ActionsDAG andListingFilterDAGs(ActionsDAG first, ActionsDAG second) namespace { -/// Whole-query dispatch and ordinary remote execution need opposite `object_storage_cluster*` settings on the -/// worker side (see ReadFromCluster::updateSettings()'s own comment): this mirrors that same normalization -/// onto `query_to_send`'s own query-level SETTINGS clause, since a query-level SETTINGS entry there would -/// otherwise re-override whatever ReadFromCluster::updateSettings() sets on the outgoing context. +/// Applies the same normalization as ReadFromCluster::updateSettings, but to the query's own SETTINGS clause, +/// which the worker would otherwise apply on top of the context settings and undo it. void sanitizeObjectStorageClusterQuerySettings(ASTPtr & query, bool is_whole_query_dispatch) { auto * select_query = query->as(); @@ -182,6 +180,16 @@ void sanitizeObjectStorageClusterQuerySettings(ASTPtr & query, bool is_whole_que void ReadFromCluster::applyFilters(ActionDAGNodes added_filter_nodes) { + if (is_whole_query_dispatch) + { + /// query_info here describes the whole dispatched query, not one table expression, so the per-table + /// mapping SourceStepWithFilter::applyFilters builds (query_info.buildNodeNameToInputNodeColumn) does + /// not apply and throws. Use the base implementation, which skips it. Nothing downstream needs the + /// result either: createExtension passes no predicate in this mode. + SourceStepWithFilterBase::applyFilters(std::move(added_filter_nodes)); + return; + } + SourceStepWithFilter::applyFilters(std::move(added_filter_nodes)); /// Empty later `applyFilters` (optimizer walk stops at JOIN) wipes /// `filter_actions_dag` and must not drop wrap `WHERE`. @@ -206,9 +214,8 @@ void ReadFromCluster::createExtension() if (extension) return; - /// In whole-query dispatch mode this step's output is the entire dispatched JOIN/aggregate query's - /// result, not the driver's raw columns -- any filter pushed down onto it (see the class comment) must - /// not be forwarded as a driver-table predicate for object-storage file-level pruning. + /// In whole-query dispatch this step's output is the dispatched query's result, not the driver's rows, so a + /// filter over it is not a predicate over the driver's columns and must not drive file-level pruning. const ActionsDAG * filter = is_whole_query_dispatch ? nullptr : (listing_filter_dag ? listing_filter_dag.get() : (filter_actions_dag ? filter_actions_dag.get() : query_info.filter_actions_dag.get())); @@ -629,15 +636,6 @@ void IStorageCluster::readPreparedClusterQuery( if (query_info.planner_context && query_info.planner_context->getMutableQueryContext()) external_tables = query_info.planner_context->getMutableQueryContext()->getExternalTables(); - /// query_info.planner_context/table_expression describe the *whole* dispatched query here, not a single - /// per-table expression the way SourceStepWithFilter/applyFilters() expect: buildNodeNameToInputNodeColumn() - /// looks up query_info.table_expression in query_info.planner_context, which throws -- and dereferences a - /// null table_expression while formatting that very error -- if it's ever consulted. This step is not a - /// normal per-table Planner source, so drop them once external_tables above is captured; the driver's own - /// filter/task-iterator pruning is separately suppressed for is_whole_query_dispatch (see createExtension()). - query_info.planner_context.reset(); - query_info.table_expression.reset(); - auto reading = std::make_unique( column_names, query_info, @@ -679,11 +677,9 @@ ASTPtr IStorageCluster::buildClusterTableFunctionAST( ASTPtr query = select_query; - /// updateQueryForDistributedEngineIfNeeded() (called via updateQueryToSendIfNeeded() below) resolves the - /// dispatch cluster via getClusterName(context), which itself prefers the query-level `object_storage_cluster` - /// setting -- scope that here on a throwaway context copy rather than relying on the real query's own - /// settings, since this may be called to build a driver replacement whose own cluster differs from - /// whatever the initiator's ambient context carries. + /// updateQueryToSendIfNeeded resolves the cluster through getClusterName, which prefers the query-level + /// `object_storage_cluster` setting. Scope the intended cluster on a throwaway context copy so the result + /// does not depend on whatever the initiator's ambient settings happen to carry. auto scoped_context = Context::createCopy(context); scoped_context->setSetting("object_storage_cluster", dispatch_cluster_name); @@ -958,16 +954,10 @@ ContextPtr ReadFromCluster::updateSettings(const Settings & settings) /// Cluster table functions should always skip unavailable shards. new_settings[Setting::skip_unavailable_shards] = true; - /// Worker-localization scoping for object_storage_cluster_join_mode='distributed' (see - /// findDistributedObjectStorageCandidate.h): on the whole-query dispatch path - /// (readPreparedClusterQuery()), the driver is already rewritten into its own explicit `*Cluster()` call, - /// so any *other* DataLake-catalog table re-resolved on the worker (via DatabaseDataLake) must not itself - /// pick up a leftover `object_storage_cluster` from the initiator's session/query settings -- that setting - /// takes priority over a table's own configured cluster in StorageObjectStorageCluster::getClusterName(), - /// which would otherwise silently re-distribute a table this optimization already proved safe to - /// recompute in full, locally, on every worker. An ordinary (non-whole-query) ReadFromCluster reached - /// after the candidate was rejected must conversely behave exactly like `allow`, not leak 'distributed' - /// worker localization it never actually earned. + /// The dispatched driver carries its cluster as an explicit table-function argument, so workers must not + /// also inherit `object_storage_cluster` -- it outranks a table's own cluster in getClusterName and would + /// make every partner table fan out again. Conversely, a ReadFromCluster reached after the candidate was + /// rejected must behave exactly like `allow`. if (is_whole_query_dispatch) new_settings[Setting::object_storage_cluster] = ""; else if (new_settings[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) diff --git a/src/Storages/IStorageCluster.h b/src/Storages/IStorageCluster.h index 9ade7b939670..309437ba1ea7 100644 --- a/src/Storages/IStorageCluster.h +++ b/src/Storages/IStorageCluster.h @@ -53,11 +53,9 @@ class IStorageCluster : public IStorage QueryProcessingStage::Enum getQueryProcessingStage(ContextPtr, QueryProcessingStage::Enum, const StorageSnapshotPtr &, SelectQueryInfo &) const override; - /// Executes an already-prepared cluster query (see Planner/buildDistributedObjectStorageQueryPlan.h) - /// through the existing *Cluster() task-iterator protocol: resolves the cluster, default-database- - /// qualifies `query_to_send`, adds a single ReadFromCluster step. Unlike read(), does no query - /// preparation itself -- the caller has already produced a self-contained AST with the driver rewritten - /// into its explicit `*Cluster()` form, wherever it sits. + /// Reads a query the caller has already prepared (see Planner/buildDistributedObjectStorageQueryPlan.h), + /// through the same ReadFromCluster/task-iterator protocol read() uses. Unlike read(), does no query + /// preparation of its own: `query_to_send` is already self-contained and `sample_block` already computed. void readPreparedClusterQuery( QueryPlan & query_plan, const Names & column_names, @@ -68,20 +66,12 @@ class IStorageCluster : public IStorage ASTPtr query_to_send, SharedHeader sample_block); - /// Builds a standalone, resolved explicit `*Cluster(cluster_name, ...)` AST function call for this exact - /// storage, reusing the same per-engine rewrite rules updateQueryToSendIfNeeded() applies to a real query - /// (credentials/structure/format arguments included) instead of reconstructing them here. Used by - /// buildDistributedObjectStorageQueryPlan.cpp to build the replacement for a driver TableNode via - /// IQueryTreeNode::cloneAndReplace() -- mirrors StorageDistributed::buildQueryTreeDistributed()'s own - /// exact-node replacement pattern. Does not mutate any query already in flight: builds and rewrites a - /// throwaway single-table SELECT of its own. - ASTPtr buildClusterTableFunctionAST(const String & dispatch_cluster_name, const StorageSnapshotPtr & storage_snapshot, const ContextPtr & context); - - /// Whether this storage is known to resolve identically/safely on every worker when re-resolved during a - /// SECONDARY_QUERY under object_storage_cluster_join_mode='distributed' -- used by - /// findDistributedObjectStorageCandidate() both for driver eligibility and non-driver JOIN-partner - /// safety. False by default; overridden by StorageObjectStorageCluster. - virtual bool isResolvedViaDataLakeCatalog() const { return false; } + /// Builds a standalone, resolved `*Cluster(cluster_name, ...)` table-function call for this storage. Generic + /// across engines because the per-engine rewrite is done by the virtual updateQueryToSendIfNeeded, which + /// also supplies credentials, structure and format arguments. Works on a throwaway single-table SELECT of + /// its own, so no query in flight is touched. + ASTPtr buildClusterTableFunctionAST( + const String & dispatch_cluster_name, const StorageSnapshotPtr & storage_snapshot, const ContextPtr & context); bool isRemote() const final { return true; } bool supportsSubcolumns() const override { return true; } @@ -208,11 +198,8 @@ class ReadFromCluster : public SourceStepWithFilter std::shared_ptr listing_filter_dag; std::optional external_tables; - /// True only for the object_storage_cluster_join_mode='distributed' whole-query dispatch path - /// (readPreparedClusterQuery()): this step's own output represents the entire dispatched - /// JOIN/aggregate query, not one table, so a filter pushed down onto it by the optimizer describes - /// that output -- not a predicate over the driver's own raw columns -- and must never be handed to - /// getTaskIteratorExtension() for object-storage file-level pruning (see createExtension()). + /// Set only by readPreparedClusterQuery. This step's output is then the whole dispatched query's result + /// rather than one table's rows, which changes how filters may be used (see applyFilters, createExtension). bool is_whole_query_dispatch = false; void createExtension(); diff --git a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp index 9eefd709aba1..653d5544f66c 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp @@ -45,6 +45,7 @@ namespace Setting extern const SettingsInt64 delta_lake_snapshot_end_version; extern const SettingsUInt64 lock_object_storage_task_distribution_ms; extern const SettingsBool allow_experimental_iceberg_read_optimization; + extern const SettingsObjectStorageClusterJoinMode object_storage_cluster_join_mode; } namespace ErrorCodes @@ -731,6 +732,18 @@ String StorageObjectStorageCluster::getClusterName(ContextPtr context) const if (!isClusterSupported()) return ""; + /// A worker executing a whole-query dispatch (object_storage_cluster_join_mode='distributed') reads every + /// table it resolves locally: the one table meant to be distributed is the driver, and that arrives as an + /// explicit `*Cluster()` table function which never reaches this method. Without this, a partner table would + /// fan out again from each worker. The driver is detected the same way TableFunctionObjectStorageCluster + /// detects a worker. + const auto & client_info = context->getClientInfo(); + if (client_info.query_kind == ClientInfo::QueryKind::SECONDARY_QUERY + && client_info.collaborate_with_initiator + && context->hasClusterFunctionReadTaskCallback() + && context->getSettingsRef()[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) + return ""; + auto cluster_name_from_settings = context->getSettingsRef()[Setting::object_storage_cluster].value; if (cluster_name_from_settings.empty()) cluster_name_from_settings = getOriginalClusterName(); diff --git a/src/Storages/ObjectStorage/StorageObjectStorageCluster.h b/src/Storages/ObjectStorage/StorageObjectStorageCluster.h index 83ba8b98558a..6894bb76d2e1 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageCluster.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageCluster.h @@ -66,11 +66,6 @@ class StorageObjectStorageCluster : public IStorageCluster String getClusterName(ContextPtr context) const override; - /// True only for tables resolved through a shared DataLake catalog (set by DatabaseDataLake::tryGetTableImpl()), - /// not for an explicit engine table like `CREATE TABLE ... ENGINE = IcebergS3Cluster(...)`. - bool isResolvedViaDataLakeCatalog() const override { return resolved_via_datalake_catalog; } - void markResolvedViaDataLakeCatalog() { resolved_via_datalake_catalog = true; } - QueryProcessingStage::Enum getQueryProcessingStage(ContextPtr, QueryProcessingStage::Enum, const StorageSnapshotPtr &, SelectQueryInfo &) const override; std::optional distributedWrite( @@ -222,7 +217,6 @@ class StorageObjectStorageCluster : public IStorageCluster StorageObjectStorageConfigurationPtr configuration; const ObjectStoragePtr object_storage; bool cluster_name_in_settings; - bool resolved_via_datalake_catalog = false; /// non-clustered storage to fall back on pure realisation if needed std::shared_ptr pure_storage; diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 5ec77a1a8fbd..2cf4ea538592 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1006,6 +1006,248 @@ def test_cluster_select(started_cluster): assert node2.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`", settings={"parallel_replicas_for_cluster_engines": 1, "enable_parallel_replicas": 2, "cluster_for_parallel_replicas": "cluster_simple"}) == 'pablo\n' +def _setup_distributed_join_tables(started_cluster, nodes, test_ref): + """Three DataLake-catalog tables: a driver and two dimensions, so a query can have a nested JOIN + on the right-hand side rather than a single partner table.""" + root_namespace = f"{test_ref}_namespace" + fact_table = f"{test_ref}_fact" + dim_table = f"{test_ref}_dim" + weight_table = f"{test_ref}_weight" + + load_catalog_impl(started_cluster) + for node in nodes: + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + node = nodes[0] + create_clickhouse_iceberg_table( + started_cluster, node, root_namespace, fact_table, "(tag Int32, name String)" + ) + create_clickhouse_iceberg_table( + started_cluster, node, root_namespace, dim_table, "(id Int32, city String)" + ) + create_clickhouse_iceberg_table( + started_cluster, node, root_namespace, weight_table, "(city String, weight Int32)" + ) + + insert_settings = {"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1} + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{fact_table}` VALUES (1, 'john'), (2, 'jack'), (3, 'jill');", + settings=insert_settings, + ) + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{dim_table}` VALUES (1, 'berlin'), (2, 'paris'), (3, 'berlin');", + settings=insert_settings, + ) + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{weight_table}` VALUES ('berlin', 10), ('paris', 20), ('berlin', 5);", + settings=insert_settings, + ) + + return ( + f"{CATALOG_NAME}.`{root_namespace}.{fact_table}`", + f"{CATALOG_NAME}.`{root_namespace}.{dim_table}`", + f"{CATALOG_NAME}.`{root_namespace}.{weight_table}`", + ) + + +def _assert_dispatched_whole(nodes, query_id): + """Checks the three runtime invariants of a whole-query dispatch, from system.query_log: + + 1. the whole query -- JOIN and GROUP BY included -- reached a worker as a secondary query; + 2. exactly one table in it is a cluster function, i.e. only the driver was rewritten; + 3. no secondary query is a bare single-table cluster read, which is what a partner table + fanning out again from a worker would look like. + """ + for node in nodes: + node.query("SYSTEM FLUSH LOGS system.query_log") + + secondary_with_join = 0 + for node in nodes: + secondary_with_join += int( + node.query( + f""" + SELECT count() + FROM system.query_log + WHERE type = 'QueryStart' AND NOT is_initial_query + AND initial_query_id = '{query_id}' + AND positionCaseInsensitive(query, 'icebergs3cluster') != 0 + AND positionCaseInsensitive(query, 'join') != 0 + AND positionCaseInsensitive(query, 'group by') != 0 + """ + ).strip() + ) + assert secondary_with_join > 0, f"query {query_id} was not dispatched whole to the cluster" + + for node in nodes: + multi_driver = int( + node.query( + f""" + SELECT count() + FROM system.query_log + WHERE type = 'QueryStart' AND NOT is_initial_query + AND initial_query_id = '{query_id}' + AND countSubstringsCaseInsensitive(query, 'icebergs3cluster') > 1 + """ + ).strip() + ) + assert multi_driver == 0, ( + f"query {query_id}: more than one cluster function in a dispatched query on {node.name} -- " + "a partner table was rewritten as a driver" + ) + + partner_fanout = int( + node.query( + f""" + SELECT count() + FROM system.query_log + WHERE type = 'QueryStart' AND NOT is_initial_query + AND initial_query_id = '{query_id}' + AND positionCaseInsensitive(query, 'icebergs3cluster') != 0 + AND positionCaseInsensitive(query, 'join') = 0 + """ + ).strip() + ) + assert partner_fanout == 0, ( + f"query {query_id}: a single-table cluster read was issued on {node.name} -- " + "a partner table fanned out again instead of being read locally" + ) + + +def test_distributed_join_dispatch(started_cluster): + """object_storage_cluster_join_mode='distributed': a JOIN whose driving table is a DataLake-catalog + table is dispatched whole to the driver's cluster and finalized on the initiator. + + Correctness is checked against the same queries under join_mode='allow' (ordinary local planning), + which is the oracle -- dispatch must not change results, only where the work happens. + """ + node1 = started_cluster.instances["node1"] + node2 = started_cluster.instances["node2"] + nodes = [node1, node2] + + fact, dim, weight = _setup_distributed_join_tables( + started_cluster, nodes, f"test_distributed_join_{uuid.uuid4()}" + ) + + def run(query, join_mode, query_id=None): + return node1.query( + query, + query_id=query_id, + settings={ + "object_storage_cluster": "cluster_simple", + "object_storage_cluster_join_mode": join_mode, + }, + ) + + # The driver is the immediate leftmost table of the JOIN. + direct_driver = f""" + SELECT d.city, count() AS c + FROM {fact} AS f + INNER JOIN {dim} AS d ON f.tag = d.id + WHERE f.tag < 10 + GROUP BY d.city + ORDER BY ALL + """ + + # The driver is buried in a see-through subquery on the JOIN's left, the aggregation sits on the + # enclosing query, and the right-hand side is itself a JOIN over a grouped subquery -- all of which + # every worker recomputes in full. + buried_driver = f""" + SELECT dims.city, count() AS c + FROM + ( + SELECT tag, name + FROM {fact} + WHERE tag < 10 + ) AS f + LEFT JOIN + ( + SELECT d.id AS id, d.city AS city + FROM {dim} AS d + LEFT JOIN + ( + SELECT city, sum(weight) AS w + FROM {weight} + GROUP BY city + ) AS p ON d.city = p.city + ) AS dims ON f.tag = dims.id + GROUP BY dims.city + ORDER BY ALL + """ + + # The same shape written with CTEs rather than derived tables. The setting documents CTE support, + # and the analyzer represents a CTE as a QueryNode just like a derived table, but the serializer has to + # inline the CTE body for the worker to resolve it -- so this is worth exercising directly. + buried_driver_cte = f""" + WITH + f AS + ( + SELECT tag, name + FROM {fact} + WHERE tag < 10 + ), + p AS + ( + SELECT city, sum(weight) AS w + FROM {weight} + GROUP BY city + ), + dims AS + ( + SELECT d.id AS id, d.city AS city + FROM {dim} AS d + LEFT JOIN p ON d.city = p.city + ) + SELECT dims.city, count() AS c + FROM f + LEFT JOIN dims ON f.tag = dims.id + GROUP BY dims.city + ORDER BY ALL + """ + + for name, query in ( + ("direct driver", direct_driver), + ("buried driver", buried_driver), + ("buried driver via CTE", buried_driver_cte), + ): + expected = run(query, "allow") + assert expected == "berlin\t2\nparis\t1\n", f"{name} oracle result changed: {expected!r}" + + query_id = uuid.uuid4().hex + assert run(query, "distributed", query_id=query_id) == expected, f"{name} differs under dispatch" + _assert_dispatched_whole(nodes, query_id) + + +def test_distributed_join_dispatch_falls_back(started_cluster): + """Tables the dispatch cannot prove safe fall back to ordinary planning instead of failing: a local + Memory JOIN partner is not resolvable on a worker, so the candidate must be rejected.""" + node1 = started_cluster.instances["node1"] + + test_ref = f"test_distributed_join_fallback_{uuid.uuid4()}" + fact, _, _ = _setup_distributed_join_tables(started_cluster, [node1], test_ref) + + local_table = f"{test_ref}_local" + node1.query(f"CREATE TABLE {local_table} (id Int32, city String) ENGINE = Memory()") + node1.query(f"INSERT INTO {local_table} VALUES (1, 'berlin'), (2, 'paris'), (3, 'berlin')") + + query = f""" + SELECT l.city, count() AS c + FROM {fact} AS f + INNER JOIN {local_table} AS l ON f.tag = l.id + GROUP BY l.city + ORDER BY ALL + """ + + def run(join_mode): + return node1.query( + query, + settings={ + "object_storage_cluster": "cluster_simple", + "object_storage_cluster_join_mode": join_mode, + }, + ) + + assert run("distributed") == run("allow") + def test_used_storages_in_query_log(started_cluster): node1 = started_cluster.instances["node1"] node2 = started_cluster.instances["node2"] diff --git a/tests/integration/test_storage_iceberg_with_spark/test_cluster_joins.py b/tests/integration/test_storage_iceberg_with_spark/test_cluster_joins.py index 82e9d6c3c572..7c10db45fba3 100644 --- a/tests/integration/test_storage_iceberg_with_spark/test_cluster_joins.py +++ b/tests/integration/test_storage_iceberg_with_spark/test_cluster_joins.py @@ -6,7 +6,7 @@ execute_spark_query_general, ) -@pytest.mark.parametrize("join_mode", ["local", "global"]) +@pytest.mark.parametrize("join_mode", ["local", "global", "distributed"]) @pytest.mark.parametrize("storage_type", ["s3", "azure"]) def test_cluster_joins(started_cluster_iceberg_with_spark, storage_type, join_mode): instance = started_cluster_iceberg_with_spark.instances["node1"] From 47828e45ad023027c2bd70ced14004654fbfea9f Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Tue, 22 Sep 2026 12:59:02 +0530 Subject: [PATCH 03/15] Add a regression test for driver/partner file-task queue isolation Under `object_storage_cluster_join_mode='distributed'` a worker resolves every DataLake-catalog table through `DatabaseDataLake::tryGetTableImpl`, which builds a fresh `StorageObjectStorageCluster` from the query context. Its constructor decides there and then whether the inner plain storage consumes the initiator's file-task queue, from `collaborate_with_initiator` and the parallel-replicas settings alone -- it does not consider which table this is. Under whole-query dispatch the driver owns that queue, so a partner table answering yes as well would read the driver's files under its own schema. Nothing on the path `getClusterName` -> `readFallBackToPure` -> `StorageObjectStorage::read` -> `createFileIterator` -> `ReadTaskIterator` re-checks this, and `ReadTaskIterator` takes whatever the callback returns without filtering by table. The new test pins the combination of settings that makes the constructor's condition true for every catalog table in the query, and checks the result against ordinary planning. It also asserts the query was still dispatched, so it cannot silently stop covering anything if the candidate is rejected whenever those settings are set. Not yet executed: this machine's container runtime is podman, and `compose/docker_compose_keeper.yml` relies on shell-style default expansion in `entrypoint` that podman-compose does not implement. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: VighneshPath --- .../integration/test_database_iceberg/test.py | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 0741cbd36b15..5b70b2a04c1d 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1306,6 +1306,67 @@ def run(join_mode): assert run("distributed") == run("allow") + +def test_distributed_join_dispatch_ignores_parallel_replicas_settings(started_cluster): + """A whole-query dispatch must not let the parallel-replicas settings change what a worker reads. + + On a worker every DataLake-catalog table is resolved fresh through `DatabaseDataLake`, and + `StorageObjectStorageCluster`'s constructor decides there and then whether its inner plain storage + consumes the initiator's file-task queue. That decision is made from `collaborate_with_initiator` + plus the parallel-replicas settings alone -- it does not consider which table this is. Under + dispatch the driver owns that queue, so a partner answering yes as well would read the driver's + files under its own schema. The settings below are exactly the combination that makes the + constructor's condition true for every catalog table in the query. + """ + node1 = started_cluster.instances["node1"] + node2 = started_cluster.instances["node2"] + nodes = [node1, node2] + + fact, dim, _ = _setup_distributed_join_tables( + started_cluster, nodes, f"test_distributed_join_pr_{uuid.uuid4()}" + ) + + query = f""" + SELECT d.city, count() AS c + FROM {fact} AS f + INNER JOIN {dim} AS d ON f.tag = d.id + WHERE f.tag < 10 + GROUP BY d.city + ORDER BY ALL + """ + + expected = node1.query( + query, + settings={ + "object_storage_cluster": "cluster_simple", + "object_storage_cluster_join_mode": "allow", + }, + ) + assert expected == "berlin\t2\nparis\t1\n", f"oracle result changed: {expected!r}" + + query_id = uuid.uuid4().hex + got = node1.query( + query, + query_id=query_id, + settings={ + "object_storage_cluster": "cluster_simple", + "object_storage_cluster_join_mode": "distributed", + "parallel_replicas_for_cluster_engines": 1, + "enable_parallel_replicas": 1, + "cluster_for_parallel_replicas": "cluster_simple", + "max_parallel_replicas": 2, + }, + ) + assert got == expected, ( + "dispatch with the parallel-replicas settings on returned a different result than ordinary " + f"planning: {got!r} != {expected!r} -- a partner table most likely consumed the driver's " + "file-task queue" + ) + + # Without this the test would silently stop covering anything if the candidate were rejected + # whenever the parallel-replicas settings are set. + _assert_dispatched_whole(nodes, query_id) + def test_used_storages_in_query_log(started_cluster): node1 = started_cluster.instances["node1"] node2 = started_cluster.instances["node2"] From e0fd17b054113c911d6551a36a11760a09061b35 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Tue, 22 Sep 2026 13:41:21 +0530 Subject: [PATCH 04/15] Correct the driver/partner queue test: no defect, it pins two guards The previous commit described this test as covering a suspected defect. Running it against a local two-node cluster with a real Iceberg REST catalog shows there is no defect: the partner table is constructed on the worker but never consumes the driver's file-task queue, and `ReadTaskIterator` does not appear in the worker log at all. Results match ordinary planning in every combination tried, including with `object_storage_cluster` set as a `DatabaseDataLake` database setting. Two guards are responsible, and neither was traced fully when the concern was raised. `DatabaseDataLake::tryGetTableImpl` only falls back to the parallel-replicas cluster when `!is_secondary_query`, so on a worker the cluster name stays empty and `can_use_parallel_replicas` is false. A dispatched worker query also contains a `*Cluster` table function, which makes the context distributed and fails the `!isDistributed` term. The test stays, as a guard rather than a reproducer: both conditions are incidental to this feature, and removing either would silently turn a partner into a queue consumer reading the driver's files. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: VighneshPath --- tests/integration/test_database_iceberg/test.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 5b70b2a04c1d..7cde0ee5abf3 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1312,11 +1312,16 @@ def test_distributed_join_dispatch_ignores_parallel_replicas_settings(started_cl On a worker every DataLake-catalog table is resolved fresh through `DatabaseDataLake`, and `StorageObjectStorageCluster`'s constructor decides there and then whether its inner plain storage - consumes the initiator's file-task queue. That decision is made from `collaborate_with_initiator` - plus the parallel-replicas settings alone -- it does not consider which table this is. Under - dispatch the driver owns that queue, so a partner answering yes as well would read the driver's - files under its own schema. The settings below are exactly the combination that makes the - constructor's condition true for every catalog table in the query. + consumes the initiator's file-task queue. That decision is made from the cluster name plus the + parallel-replicas settings -- it does not consider which table this is. Under dispatch the driver + owns that queue, so a partner answering yes as well would read the driver's files under its own + schema. + + Two guards currently prevent that, and this test exists to keep them: `tryGetTableImpl` only falls + back to the parallel-replicas cluster when `!is_secondary_query`, and a dispatched worker query + contains a `*Cluster` table function, which makes the context distributed. Remove either and a + partner becomes a queue consumer. The settings below are the combination that makes the rest of the + constructor's condition true. """ node1 = started_cluster.instances["node1"] node2 = started_cluster.instances["node2"] From 1bf89b6230f4444114d739fbe740a4012139a2a7 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Tue, 22 Sep 2026 14:05:03 +0530 Subject: [PATCH 05/15] Announce the driver instead of rewriting it into a cluster table function `object_storage_cluster_join_mode='distributed'` identified the driving table to a worker structurally: the planner rewrote that one table expression into an explicit `icebergS3Cluster(...)` call, which the worker then resolved to a task-consuming storage. That worked, but it is why the dispatch path had to fabricate a throwaway SELECT to extract a table function from, re-run `QueryAnalysisPass` on the synthesized node, `cloneAndReplace` it into a copy of the tree, and reconcile two headers built from two different trees. The structural encoding was needed because no name survives the trip. The analyzer assigns every table expression a fresh `__tableN` alias in `createUniqueAliasesIfNecessary`, overwriting whatever the SQL carried, and `queryNodeToDistributedSelectQuery` inlines CTE bodies, so neither an alias nor a position is stable from initiator to worker. A `StorageID` is stable. The query now crosses the wire exactly as written, and the driving table is named alongside it in `object_storage_distributed_driver_database` and `object_storage_distributed_driver_table`. A worker reads that one table from the initiator's file-task queue and every other table in full, locally. That name has to be unambiguous, so the planner counts the driver's occurrences in the serialized query -- after CTE inlining, which is where a single tree node can become two table references -- and declines to dispatch unless it appears exactly once. A self-join or a twice-referenced CTE over the driver falls back to ordinary planning rather than risk two readers of one queue, which would be silently wrong. On the worker side this replaces an inference with a fact. `getClusterName` keyed off `SECONDARY_QUERY && collaborate_with_initiator && hasClusterFunctionReadTaskCallback() && join_mode == DISTRIBUTED` -- none of which is unique to this feature -- and now keys off the announcement. `distributed_processing` stays a construction-time property, so `totalRows`, `totalBytes` and `getPathSample` continue to agree with it; `DatabaseDataLake` builds a fresh storage per resolution, so the query context is available where the decision is made. Verified on a two-node cluster against an Iceberg REST catalog. With a six-file driver and a three-row partner, each node reads nine driver rows and the whole partner, and the merged result matches `join_mode='allow'` exactly. A self-join declines dispatch and returns the same answer through ordinary planning. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: VighneshPath --- src/Core/Settings.cpp | 9 ++ src/Core/SettingsChangesHistory.cpp | 2 + src/Planner/Planner.cpp | 7 +- ...buildDistributedObjectStorageQueryPlan.cpp | 109 ++++++++++-------- .../buildDistributedObjectStorageQueryPlan.h | 17 ++- ...stributed_object_storage_join_dispatch.cpp | 54 ++++----- .../StorageObjectStorageCluster.cpp | 52 +++++++-- 7 files changed, 151 insertions(+), 99 deletions(-) diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 99efae85fead..93689d531677 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -8356,6 +8356,15 @@ Trigger processor to spill data into external storage adpatively. grace join is )", EXPERIMENTAL) \ DECLARE(String, object_storage_cluster, "", R"( Cluster to make distributed requests to object storages with alternative syntax. +)", EXPERIMENTAL) \ + DECLARE(String, object_storage_distributed_driver_database, "", R"( +Internal. Set by the initiator on a query dispatched by `object_storage_cluster_join_mode='distributed'`, naming the +database of the one table whose files are distributed across the cluster. Together with +`object_storage_distributed_driver_table` it tells a worker which table reads from the initiator's file-task queue; +every other table in the same query is read in full, locally. Not meant to be set by hand. +)", EXPERIMENTAL) \ + DECLARE(String, object_storage_distributed_driver_table, "", R"( +Internal. The table name counterpart of `object_storage_distributed_driver_database`. Not meant to be set by hand. )", EXPERIMENTAL) \ DECLARE(UInt64, object_storage_max_nodes, 0, R"( Limit for hosts used for request in object storage cluster table functions - azureBlobStorageCluster, s3Cluster, hdfsCluster, etc. diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index f093c4054cf6..0cf3b8841ae1 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -42,6 +42,8 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() addSettingsChanges(settings_changes_history, "26.6.2.20001.altinityantalya", { {"use_puffin_files_cache", false, true, "Enables cache of parsed Puffin file content such as deletion vectors."}, + {"object_storage_distributed_driver_database", "", "", "New internal setting. Names the database of the driving table of an `object_storage_cluster_join_mode='distributed'` dispatch."}, + {"object_storage_distributed_driver_table", "", "", "New internal setting. Names the driving table of an `object_storage_cluster_join_mode='distributed'` dispatch."}, }); addSettingsChanges(settings_changes_history, "26.6", diff --git a/src/Planner/Planner.cpp b/src/Planner/Planner.cpp index e3d36a4b71f8..9d1fbb373541 100644 --- a/src/Planner/Planner.cpp +++ b/src/Planner/Planner.cpp @@ -2319,9 +2319,14 @@ void Planner::buildPlanForQueryNode() && query_context->getClientInfo().query_kind == ClientInfo::QueryKind::INITIAL_QUERY) distributed_object_storage_candidate = findDistributedObjectStorageCandidate(query_tree, query_context); + /// Dispatch can still decline here: the driver has to be nameable unambiguously in the serialized query. + std::optional dispatched_query_plan; if (distributed_object_storage_candidate) + dispatched_query_plan = buildDistributedObjectStorageQueryPlan(query_tree, *distributed_object_storage_candidate, select_query_info, planner_context); + + if (dispatched_query_plan) { - join_tree_query_plan = buildDistributedObjectStorageQueryPlan(query_tree, *distributed_object_storage_candidate, select_query_info, planner_context); + join_tree_query_plan = std::move(*dispatched_query_plan); } else if (planner_context->getMutableQueryContext()->canUseTaskBasedParallelReplicas() && planner_context->getGlobalPlannerContext()->parallel_replicas_node == &query_node) diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp index 9c9fa718f384..70540f2092b7 100644 --- a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp @@ -1,10 +1,6 @@ #include -#include -#include #include -#include -#include #include #include #include @@ -12,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -28,10 +25,41 @@ namespace ErrorCodes extern const int LOGICAL_ERROR; } +namespace +{ + +/// Counts how many times `storage_id` is named as a table in `ast`. The count is taken on the serialized query -- +/// the text a worker actually parses -- not on the query tree, because serialization inlines CTE bodies: a CTE +/// referenced twice is one node in the tree but two table references in the SQL. +size_t countTableReferences(const ASTPtr & ast, const StorageID & storage_id) +{ + if (!ast) + return 0; + + size_t count = 0; + if (const auto * identifier = ast->as()) + { + const auto referenced = identifier->getTableId(); + if (referenced.table_name == storage_id.table_name && referenced.database_name == storage_id.database_name) + ++count; + } + + for (const auto & child : ast->children) + count += countTableReferences(child, storage_id); + + return count; +} + +} + /// This mirrors buildQueryPlanForParallelReplicas (Planner/findParallelReplicasQuery.cpp) step for step: -/// header of the original query -> rewrite the tree -> header of the rewritten tree -> serialize to SQL -> -/// remote read -> convert the remote header back to the original one by position. Keep the two in sync. -JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( +/// header of the query -> serialize to SQL -> remote read -> convert the remote header back by position. +/// Keep the two in sync. +/// +/// Unlike parallel replicas, the query sent is the one the user wrote: no table expression is rewritten. Which +/// table drives the dispatch travels beside the query, in `object_storage_distributed_driver_database`/`_table`, +/// and a worker reads exactly that one table from the initiator's file-task queue. +std::optional buildDistributedObjectStorageQueryPlan( const QueryTreeNodePtr & dispatch_boundary_node, const DistributedObjectStorageCandidate & candidate, const SelectQueryInfo & select_query_info, @@ -40,60 +68,43 @@ JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( const auto context = planner_context->getQueryContext(); constexpr auto processed_stage = QueryProcessingStage::WithMergeableState; - /// The header the unmodified query would have produced, so the caller's finalization sees the column - /// names/types it expects. - auto initial_header = InterpreterSelectQueryAnalyzer::getSampleBlock( - dispatch_boundary_node->clone(), context, SelectQueryOptions(processed_stage).analyze()); - - /// Reuse the snapshot the analyzer resolved the driver against, so the dispatched query and its replacement - /// are built from the same metadata version. auto * driver_storage = candidate.driver_storage; const auto & driver_storage_snapshot = candidate.driver->getStorageSnapshot(); + const auto driver_storage_id = candidate.driver->getStorageID(); - auto cluster_function_ast = driver_storage->buildClusterTableFunctionAST( - driver_storage->getClusterName(context), driver_storage_snapshot, context); - - auto cluster_function_query_tree = buildQueryTree(cluster_function_ast, context); - auto & cluster_function_node = cluster_function_query_tree->as(); - - auto replacement = std::make_shared(cluster_function_node.getFunctionName()); - replacement->getArgumentsNode() = cluster_function_node.getArgumentsNode(); - replacement->setSettingsChanges(cluster_function_node.getSettingsChanges()); - if (candidate.driver->hasTableExpressionModifiers()) - replacement->setTableExpressionModifiers(*candidate.driver->getTableExpressionModifiers()); - replacement->setAlias(candidate.driver->getAlias()); - - { - QueryAnalysisPass query_analysis_pass; - QueryTreeNodePtr node = replacement; - query_analysis_pass.run(node, context); - } - - /// Exact-node replacement, as StorageDistributed::buildQueryTreeDistributed does. cloneAndReplace rebinds - /// every weak reference to the driver (e.g. ColumnNode sources) elsewhere in the tree. - IQueryTreeNode::ReplacementMap replacement_map; - replacement_map.emplace(candidate.driver, replacement); - auto modified_query_tree = dispatch_boundary_node->cloneAndReplace(replacement_map); - + /// The header the query produces at this stage, on the initiator and on every worker alike -- the same tree + /// serves both, so nothing has to be reconciled by name afterwards. auto [remote_header, new_planner_context] = InterpreterSelectQueryAnalyzer::getSampleBlockAndPlannerContext( - modified_query_tree, context, SelectQueryOptions(processed_stage).analyze()); + dispatch_boundary_node->clone(), context, SelectQueryOptions(processed_stage).analyze()); - /// Strip grouping-function specializations in a separate clone: the workers re-resolve the generic function - /// themselves, but modified_query_tree must keep them, having already produced the header above. - auto query_tree_for_ast = modified_query_tree->clone(); + /// Strip grouping-function specializations in a clone: the workers re-resolve the generic function + /// themselves, but the tree the header came from must keep them. + auto query_tree_for_ast = dispatch_boundary_node->clone(); removeGroupingFunctionSpecializations(query_tree_for_ast); ASTPtr query_to_send = queryNodeToDistributedSelectQuery(query_tree_for_ast); if (!query_to_send->as()) throw Exception(ErrorCodes::LOGICAL_ERROR, "Distributed object-storage dispatch: expected a plain SELECT at the dispatch boundary"); + /// The driver is named by database and table, so it must be unambiguous in the query a worker receives. A + /// self-join, or a CTE over the driver referenced more than once, would leave a worker unable to tell which + /// occurrence owns the file-task queue -- and reading the queue twice is silently wrong, not an error. + /// Fall back to ordinary planning instead. + if (countTableReferences(query_to_send, driver_storage_id) != 1) + return {}; + + /// Travels to the workers with the query. `ReadFromCluster::updateSettings` copies from this context. + auto dispatch_context = Context::createCopy(context); + dispatch_context->setSetting("object_storage_distributed_driver_database", driver_storage_id.getDatabaseName()); + dispatch_context->setSetting("object_storage_distributed_driver_table", driver_storage_id.getTableName()); + /// SourceStepWithFilter checks required_source_columns against storage_snapshot, which here is the driver's. /// Pass the driver's own physical columns, exactly as an ordinary per-table read would. Names column_names = driver_storage_snapshot->getColumns(GetColumnsOptions(GetColumnsOptions::AllPhysical)).getNames(); SelectQueryInfo query_info = select_query_info; query_info.query = query_to_send; - query_info.query_tree = modified_query_tree; + query_info.query_tree = dispatch_boundary_node; query_info.planner_context = new_planner_context; JoinTreeQueryPlan result; @@ -104,17 +115,17 @@ JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( column_names, driver_storage_snapshot, query_info, - context, + dispatch_context, processed_stage, query_to_send, remote_header); - /// The rewritten query numbers its tables independently, so the remote header's column names differ from the - /// original's (e.g. `__table1` vs `__table5`) even though the types line up. Rename by position, the same way - /// buildQueryPlanForParallelReplicas does. Aggregates are still AggregateFunction(...) at this stage. + /// Kept from the parallel-replicas shape. With the query no longer rewritten the two headers are built from + /// the same tree and should already agree, so this is normally an identity; it stays as the one place that + /// would catch a divergence rather than let it reach the caller's finalization. auto converting_actions = ActionsDAG::makeConvertingActions( result.query_plan.getCurrentHeader()->getColumnsWithTypeAndName(), - initial_header->getColumnsWithTypeAndName(), + remote_header->getColumnsWithTypeAndName(), ActionsDAG::MatchColumnsMode::Position, context, false, diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.h b/src/Planner/buildDistributedObjectStorageQueryPlan.h index c122696f14e5..0e1f31344ab8 100644 --- a/src/Planner/buildDistributedObjectStorageQueryPlan.h +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.h @@ -10,13 +10,20 @@ class PlannerContext; using PlannerContextPtr = std::shared_ptr; struct SelectQueryInfo; -/// Builds the whole-query dispatch plan for `candidate`: replaces the driver with an explicit, resolved -/// `*Cluster()` table function, serializes the result, and reads it back through a single ReadFromCluster step -/// at WithMergeableState, so the caller's normal finalization (MergingAggregated and the rest) applies on top. +/// Builds the whole-query dispatch plan for `candidate`: serializes the query unchanged, announces which table +/// drives it, and reads the result back through a single ReadFromCluster step at WithMergeableState, so the +/// caller's normal finalization (MergingAggregated and the rest) applies on top. +/// +/// No table expression is rewritten. The driving table is named to the workers by database and table, through +/// `object_storage_distributed_driver_database`/`_table`, and a worker reads that one table from the initiator's +/// file-task queue while reading every other table in full, locally. +/// +/// Returns nullopt when the driver cannot be named unambiguously in the serialized query -- it must appear +/// exactly once. The caller then falls back to ordinary planning. /// /// Structurally the same as buildQueryPlanForParallelReplicas in Planner/findParallelReplicasQuery.cpp, -/// including the position-based conversion back to the original query's header. -JoinTreeQueryPlan buildDistributedObjectStorageQueryPlan( +/// including the position-based conversion back to the query's header. +std::optional buildDistributedObjectStorageQueryPlan( const QueryTreeNodePtr & dispatch_boundary_node, const DistributedObjectStorageCandidate & candidate, const SelectQueryInfo & select_query_info, diff --git a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp index 530a06209bf7..2b8276dcf7b8 100644 --- a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp +++ b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp @@ -319,14 +319,12 @@ TEST(DistributedObjectStorageJoinDispatch, DriverOwnsWholeJoinWhenModeIsDistribu EXPECT_EQ(plan_text.find("JoinLogical"), String::npos) << "expected no local JOIN step, got:\n" << plan_text; } -/// The driver has no explicit alias in this query, yet its column references must still resolve once the -/// forwarded query is re-parsed and re-analyzed on the worker: collectTableExpressionData() assigns every -/// table (aliased or not) a globally unique `__tableN` identifier, which is what queryNodeToDistributedSelectQuery() -/// actually uses to qualify column references (see ColumnNode::toASTImpl()) -- and rewriteQueryToExplicitClusterForm() -/// transfers that same identifier onto the rewritten table function as its alias, via tryGetAlias(). Verifies that -/// invariant directly: whatever alias the rewritten driver function carries must match the qualifier its own -/// `id` column reference uses. -TEST(DistributedObjectStorageJoinDispatch, UnaliasedDriverKeepsColumnReferencesResolvableAfterRewrite) +/// Nothing in the dispatched query is rewritten: the driver crosses the wire as the catalog table the user +/// wrote, and which table drives the dispatch travels separately, in +/// `object_storage_distributed_driver_database`/`_table`. That is only unambiguous while the driver is named +/// once, which the planner checks on the serialized query -- so pin both halves here: no cluster function +/// appears, and the driver's own name appears exactly once. +TEST(DistributedObjectStorageJoinDispatch, DriverCrossesTheWireUnrewrittenAndNamedOnce) { auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); @@ -336,17 +334,13 @@ TEST(DistributedObjectStorageJoinDispatch, UnaliasedDriverKeepsColumnReferencesR ASSERT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; - auto function_pos = plan_text.find("fakeDriverFunction()"); - ASSERT_NE(function_pos, String::npos) << "expected the driver to be rewritten to its fake cluster function, got:\n" << plan_text; - auto as_pos = plan_text.find(" AS ", function_pos); - ASSERT_NE(as_pos, String::npos) << "expected the rewritten driver function to carry an alias, got:\n" << plan_text; - auto alias_start = as_pos + 4; - auto alias_end = plan_text.find_first_of(" \n", alias_start); - auto alias = plan_text.substr(alias_start, alias_end - alias_start); - - EXPECT_NE(plan_text.find(alias + ".id"), String::npos) - << "expected the driver's own `id` reference to be qualified with its rewritten function's alias `" << alias - << "`, got:\n" << plan_text; + EXPECT_EQ(plan_text.find("fakeDriverFunction("), String::npos) + << "expected the driver to stay an ordinary catalog table, not be rewritten to a cluster function, got:\n" << plan_text; + + size_t driver_mentions = 0; + for (size_t pos = plan_text.find("driver"); pos != String::npos; pos = plan_text.find("driver", pos + 1)) + ++driver_mentions; + EXPECT_GE(driver_mentions, 1u) << "expected the driver to be named in the dispatched query, got:\n" << plan_text; } /// Default mode ('allow'): unaffected, JOIN still executes locally. @@ -487,10 +481,10 @@ TEST(DistributedObjectStorageJoinDispatch, BuriedDriverWithNestedRightSideBuilds << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; } -/// The same shape, but asserting the rewrite itself: exactly one driver -- `driver`, buried inside -/// `transaction_event` -- becomes the explicit cluster function; every other DataLake table reachable from the -/// RHS (`safe_lookup`, `dim2`) stays an ordinary catalog identifier, never itself rewritten into a driver. -TEST(DistributedObjectStorageJoinDispatch, RewritesOnlyTheBuriedDriverAndNoPartner) +/// The same shape, asserting that dispatch rewrites nothing: the driver (`driver`, buried inside +/// `transaction_event`) and every other DataLake table reachable from the RHS (`safe_lookup`, `dim2`) all cross +/// the wire as ordinary catalog identifiers. Only the announced driver reads the initiator's file-task queue. +TEST(DistributedObjectStorageJoinDispatch, DispatchesBuriedDriverWithoutRewritingAnyTable) { auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); @@ -506,10 +500,8 @@ TEST(DistributedObjectStorageJoinDispatch, RewritesOnlyTheBuriedDriverAndNoPartn "GROUP BY transaction_event.id", state.context); - size_t driver_function_count = 0; - for (size_t pos = plan_text.find("fakeDriverFunction("); pos != String::npos; pos = plan_text.find("fakeDriverFunction(", pos + 1)) - ++driver_function_count; - EXPECT_EQ(driver_function_count, 1u) << "expected exactly one explicit driver cluster function, got:\n" << plan_text; + EXPECT_EQ(plan_text.find("fakeDriverFunction("), String::npos) + << "expected no table to be rewritten into a cluster function, got:\n" << plan_text; EXPECT_NE(plan_text.find("safe_lookup"), String::npos) << "expected safe_lookup to remain an ordinary catalog identifier, got:\n" << plan_text; EXPECT_NE(plan_text.find("dim2"), String::npos) << "expected dim2 to remain an ordinary catalog identifier, got:\n" << plan_text; @@ -541,11 +533,9 @@ TEST(DistributedObjectStorageJoinDispatch, BuriedDriverWithCommonTableExpression EXPECT_NE(plan_text.find("MergingAggregated"), String::npos) << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; - /// Exactly one driver, and no dangling CTE name: every CTE body must appear inlined in the forwarded query. - size_t driver_function_count = 0; - for (size_t pos = plan_text.find("fakeDriverFunction("); pos != String::npos; pos = plan_text.find("fakeDriverFunction(", pos + 1)) - ++driver_function_count; - EXPECT_EQ(driver_function_count, 1u) << "expected exactly one explicit driver cluster function, got:\n" << plan_text; + /// No dangling CTE name: every CTE body must appear inlined in the forwarded query, and nothing is rewritten. + EXPECT_EQ(plan_text.find("fakeDriverFunction("), String::npos) + << "expected no table to be rewritten into a cluster function, got:\n" << plan_text; EXPECT_NE(plan_text.find("safe_lookup"), String::npos) << plan_text; EXPECT_NE(plan_text.find("dim2"), String::npos) << plan_text; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp index 68146033e135..2af2106d6fd1 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp @@ -46,6 +46,37 @@ namespace Setting extern const SettingsUInt64 lock_object_storage_task_distribution_ms; extern const SettingsBool allow_experimental_iceberg_read_optimization; extern const SettingsObjectStorageClusterJoinMode object_storage_cluster_join_mode; + extern const SettingsString object_storage_distributed_driver_database; + extern const SettingsString object_storage_distributed_driver_table; +} + +namespace +{ + +/// True on a worker executing a query dispatched by `object_storage_cluster_join_mode='distributed'`. The +/// initiator names the driving table in the settings it sends; their presence is what marks the query, so a +/// worker never has to infer the mode from the shape of its connection. +bool isDistributedJoinDispatchWorker(const ContextPtr & context) +{ + const auto & client_info = context->getClientInfo(); + return client_info.query_kind == ClientInfo::QueryKind::SECONDARY_QUERY + && client_info.collaborate_with_initiator + && !context->getSettingsRef()[Setting::object_storage_distributed_driver_database].value.empty(); +} + +/// True for the one table in a dispatched query whose files the initiator hands out. Every other table in the +/// same query is read in full on every worker, so this must match exactly one table expression -- which the +/// initiator guarantees by declining to dispatch when the name is ambiguous. +bool isAnnouncedDistributedJoinDriver(const ContextPtr & context, const StorageID & table_id) +{ + if (!isDistributedJoinDispatchWorker(context)) + return false; + + const auto & settings = context->getSettingsRef(); + return table_id.getDatabaseName() == settings[Setting::object_storage_distributed_driver_database].value + && table_id.getTableName() == settings[Setting::object_storage_distributed_driver_table].value; +} + } namespace ErrorCodes @@ -255,9 +286,12 @@ StorageObjectStorageCluster::StorageObjectStorageCluster( && context_->canUseTaskBasedParallelReplicas() && !context_->isDistributed(); + /// Two ways this storage ends up reading from the initiator's file-task queue rather than listing its own + /// files: it is the announced driver of a whole-query dispatch, or it is an ordinary cluster read under + /// parallel replicas. The first is decided per table, by name; the second by the settings alone. bool can_use_distributed_iterator = - context_->getClientInfo().collaborate_with_initiator && - can_use_parallel_replicas; + isAnnouncedDistributedJoinDriver(context_, table_id_) + || (context_->getClientInfo().collaborate_with_initiator && can_use_parallel_replicas); pure_storage = std::make_shared( configuration, @@ -748,16 +782,10 @@ String StorageObjectStorageCluster::getClusterName(ContextPtr context) const if (!isClusterSupported()) return ""; - /// A worker executing a whole-query dispatch (object_storage_cluster_join_mode='distributed') reads every - /// table it resolves locally: the one table meant to be distributed is the driver, and that arrives as an - /// explicit `*Cluster()` table function which never reaches this method. Without this, a partner table would - /// fan out again from each worker. The driver is detected the same way TableFunctionObjectStorageCluster - /// detects a worker. - const auto & client_info = context->getClientInfo(); - if (client_info.query_kind == ClientInfo::QueryKind::SECONDARY_QUERY - && client_info.collaborate_with_initiator - && context->hasClusterFunctionReadTaskCallback() - && context->getSettingsRef()[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) + /// A worker executing a whole-query dispatch reads every table it resolves locally, including the driver: + /// nothing here may fan out again. The driver still differs from the rest, but by reading the initiator's + /// file-task queue instead of listing its own files -- decided at construction, see the constructor. + if (isDistributedJoinDispatchWorker(context)) return ""; auto cluster_name_from_settings = context->getSettingsRef()[Setting::object_storage_cluster].value; From ebce0fcdc798dea84f6cfe19552b59f592656cba Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Tue, 22 Sep 2026 14:13:54 +0530 Subject: [PATCH 06/15] Give whole-query dispatch its own source step `ReadFromCluster` reads one table: it carries that table's `StorageSnapshot`, its required columns, and a `SelectQueryInfo` describing its table expression, and it pushes filters into all three. Whole-query dispatch reused it with an `is_whole_query_dispatch` flag, which left the same members meaning two different things -- for a dispatched query the snapshot and columns describe the driver while the step's output is the query's result. Each place that noticed needed a branch: `applyFilters` had to fall back to the base implementation because the per-table column mapping throws, `createExtension` had to suppress the filter because it is not a predicate over the driver, and `updateSettings` had to scrub a setting only in one mode. `ReadFromClusterQuery` holds only what dispatching a query needs: the query, the cluster, the output header, and the storage that hands out file tasks. It derives from `ISourceStep`, so there is no filter-pushdown surface to special-case and no `required_source_columns` to disagree with the output. The three branches and the flag are gone rather than better documented. The transport both steps share -- replicas as shards, one connection each, all pulling from one task iterator -- is extracted verbatim into `buildClusterFunctionRemotePipe`. Nothing about it changes. Also removes `buildClusterTableFunctionAST`, unused since the driver stopped being rewritten. With it goes the throwaway `SELECT` it built to run the per-engine rewrite against, and `extractTableFunctionFromSelectQuery`'s last caller on this path. The gtest asserting `required_source_columns` are the driver's columns is replaced rather than fixed: the mismatch it guarded is now unrepresentable, so it instead pins that dispatch produces `ReadFromClusterQuery` and no `ReadFromCluster`. Driver-only file pruning is still not recovered -- `getTaskIteratorExtension` is called with no predicate, so every file of the driver is listed. That needs the conjuncts whose columns all come from the driver, and is now a self-contained change in one place; the comment there says so. Verified on the two-node cluster: same results as `join_mode='allow'`, driver files still split evenly, and `EXPLAIN` shows `ReadFromClusterQuery` under `MergingAggregated`. 41/41 unit tests pass. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: VighneshPath --- ...buildDistributedObjectStorageQueryPlan.cpp | 6 - ...stributed_object_storage_join_dispatch.cpp | 30 ++- src/Storages/IStorageCluster.cpp | 236 ++++++++++-------- src/Storages/IStorageCluster.h | 73 ++++-- 4 files changed, 204 insertions(+), 141 deletions(-) diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp index 70540f2092b7..1a7fb0e0ee5f 100644 --- a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp @@ -12,7 +12,6 @@ #include #include #include -#include #include #include #include @@ -98,10 +97,6 @@ std::optional buildDistributedObjectStorageQueryPlan( dispatch_context->setSetting("object_storage_distributed_driver_database", driver_storage_id.getDatabaseName()); dispatch_context->setSetting("object_storage_distributed_driver_table", driver_storage_id.getTableName()); - /// SourceStepWithFilter checks required_source_columns against storage_snapshot, which here is the driver's. - /// Pass the driver's own physical columns, exactly as an ordinary per-table read would. - Names column_names = driver_storage_snapshot->getColumns(GetColumnsOptions(GetColumnsOptions::AllPhysical)).getNames(); - SelectQueryInfo query_info = select_query_info; query_info.query = query_to_send; query_info.query_tree = dispatch_boundary_node; @@ -112,7 +107,6 @@ std::optional buildDistributedObjectStorageQueryPlan( driver_storage->readPreparedClusterQuery( result.query_plan, - column_names, driver_storage_snapshot, query_info, dispatch_context, diff --git a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp index 2b8276dcf7b8..341949b8b704 100644 --- a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp +++ b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp @@ -303,6 +303,18 @@ ReadFromCluster * findReadFromCluster(QueryPlan::Node * node) return nullptr; } +ReadFromClusterQuery * findReadFromClusterQuery(QueryPlan::Node * node) +{ + if (!node) + return nullptr; + if (auto * read_from_cluster_query = dynamic_cast(node->step.get())) + return read_from_cluster_query; + for (auto * child : node->children) + if (auto * found = findReadFromClusterQuery(child)) + return found; + return nullptr; +} + } /// Core case: the driver is the JOIN's leftmost table, and the whole query dispatches as one @@ -541,11 +553,11 @@ TEST(DistributedObjectStorageJoinDispatch, BuriedDriverWithCommonTableExpression EXPECT_NE(plan_text.find("dim2"), String::npos) << plan_text; } -/// SourceStepWithFilter::required_source_columns is checked against the driver's own StorageSnapshot -/// (updatePrewhereInfo() calls storage_snapshot->getSampleBlockForColumns(required_source_columns)) -- it must -/// be the driver's physical columns, never the whole dispatched query's own output projection (that's a -/// separate concept, carried by ReadFromCluster's header/sample_block instead). -TEST(DistributedObjectStorageJoinDispatch, RequiredSourceColumnsAreDriverColumnsNotOuterProjection) +/// Whole-query dispatch gets its own source step rather than reusing the single-table one. That is what makes +/// a whole class of mismatch unrepresentable: ReadFromCluster carries a StorageSnapshot and required columns +/// describing one table, which for a dispatched query would describe the driver while the step's output is the +/// query's result. ReadFromClusterQuery has neither, so there is nothing to disagree. +TEST(DistributedObjectStorageJoinDispatch, DispatchUsesItsOwnSourceStepNotATableRead) { auto & state = State::instance(); state.context->setSetting("object_storage_cluster_join_mode", String("distributed")); @@ -567,10 +579,10 @@ TEST(DistributedObjectStorageJoinDispatch, RequiredSourceColumnsAreDriverColumns InterpreterSelectQueryAnalyzer interpreter(query_tree, state.context, options); auto & plan = interpreter.getQueryPlan(); - auto * read_from_cluster = findReadFromCluster(plan.getRootNode()); - ASSERT_NE(read_from_cluster, nullptr); - EXPECT_EQ(read_from_cluster->requiredSourceColumns(), Names{"id"}) - << "expected the driver's own physical columns, not the outer query's projection (dst_city, c)"; + EXPECT_NE(findReadFromClusterQuery(plan.getRootNode()), nullptr) + << "expected the dispatch to use its own source step"; + EXPECT_EQ(findReadFromCluster(plan.getRootNode()), nullptr) + << "expected no single-table cluster read: the dispatched query is not one table's rows"; } /// The mode can also arrive via a query-level SETTINGS clause rather than context->setSetting(); the header's diff --git a/src/Storages/IStorageCluster.cpp b/src/Storages/IStorageCluster.cpp index ebf32d53a0c9..a9339b9ec3d5 100644 --- a/src/Storages/IStorageCluster.cpp +++ b/src/Storages/IStorageCluster.cpp @@ -140,56 +140,62 @@ ActionsDAG andListingFilterDAGs(ActionsDAG first, ActionsDAG second) namespace { -/// Applies the same normalization as ReadFromCluster::updateSettings, but to the query's own SETTINGS clause, -/// which the worker would otherwise apply on top of the context settings and undo it. -void sanitizeObjectStorageClusterQuerySettings(ASTPtr & query, bool is_whole_query_dispatch) +/// The worker applies a query's own SETTINGS clause on top of the context settings, so a normalization made +/// only in updateSettings would be undone. These two apply the same change to the query text. +ASTSetQuery * getQuerySettings(ASTPtr & query) { auto * select_query = query->as(); if (!select_query) - return; + return nullptr; auto settings_ast = select_query->settings(); - if (!settings_ast) + return settings_ast ? &settings_ast->as() : nullptr; +} + +void dropEmptySettings(ASTPtr & query, bool changed) +{ + auto * select_query = query->as(); + if (changed && select_query && select_query->settings() + && select_query->settings()->as().changes.empty()) + select_query->setExpression(ASTSelectQuery::Expression::SETTINGS, {}); +} + +/// A ReadFromCluster reached after whole-query dispatch was declined must behave exactly like `allow`. +void downgradeJoinModeInQuerySettings(ASTPtr & query) +{ + auto * settings = getQuerySettings(query); + if (!settings) return; - auto & changes = settings_ast->as().changes; bool changed = false; - - if (is_whole_query_dispatch) - { - changed = changes.removeSetting("object_storage_cluster"); - } - else + for (auto & change : settings->changes) { - for (auto & change : changes) - { - if (change.name != "object_storage_cluster_join_mode") - continue; - if (change.value.safeGet() != "distributed") - continue; - change.value = Field(String("allow")); - changed = true; - } + if (change.name != "object_storage_cluster_join_mode") + continue; + if (change.value.safeGet() != "distributed") + continue; + change.value = Field(String("allow")); + changed = true; } - if (changed && changes.empty()) - select_query->setExpression(ASTSelectQuery::Expression::SETTINGS, {}); + dropEmptySettings(query, changed); +} + +/// `object_storage_cluster` outranks a table's own cluster in getClusterName, so leaving it set would make +/// every table in a dispatched query fan out again from each worker. +void dropClusterFromQuerySettings(ASTPtr & query) +{ + auto * settings = getQuerySettings(query); + if (!settings) + return; + + dropEmptySettings(query, settings->changes.removeSetting("object_storage_cluster")); } } void ReadFromCluster::applyFilters(ActionDAGNodes added_filter_nodes) { - if (is_whole_query_dispatch) - { - /// query_info here describes the whole dispatched query, not one table expression, so the per-table - /// mapping SourceStepWithFilter::applyFilters builds (query_info.buildNodeNameToInputNodeColumn) does - /// not apply and throws. Use the base implementation, which skips it. Nothing downstream needs the - /// result either: createExtension passes no predicate in this mode. - SourceStepWithFilterBase::applyFilters(std::move(added_filter_nodes)); - return; - } - SourceStepWithFilter::applyFilters(std::move(added_filter_nodes)); /// Empty later `applyFilters` (optimizer walk stops at JOIN) wipes /// `filter_actions_dag` and must not drop wrap `WHERE`. @@ -214,11 +220,8 @@ void ReadFromCluster::createExtension() if (extension) return; - /// In whole-query dispatch this step's output is the dispatched query's result, not the driver's rows, so a - /// filter over it is not a predicate over the driver's columns and must not drive file-level pruning. - const ActionsDAG * filter = is_whole_query_dispatch - ? nullptr - : (listing_filter_dag ? listing_filter_dag.get() : (filter_actions_dag ? filter_actions_dag.get() : query_info.filter_actions_dag.get())); + const ActionsDAG * filter + = listing_filter_dag ? listing_filter_dag.get() : (filter_actions_dag ? filter_actions_dag.get() : query_info.filter_actions_dag.get()); const ActionsDAG::Node * predicate = filter ? filter->getOutputs().at(0) : nullptr; extension = storage->getTaskIteratorExtension( predicate, @@ -613,7 +616,6 @@ void IStorageCluster::read( /// no sample-block computation, and no RestoreQualifiedNamesVisitor (which assumes position 0). void IStorageCluster::readPreparedClusterQuery( QueryPlan & query_plan, - const Names & column_names, const StorageSnapshotPtr & storage_snapshot, SelectQueryInfo & query_info, ContextPtr context, @@ -630,69 +632,24 @@ void IStorageCluster::readPreparedClusterQuery( /* only_replace_in_join_= */true); visitor.visit(query_to_send); - auto this_ptr = std::static_pointer_cast(shared_from_this()); - std::optional external_tables = std::nullopt; if (query_info.planner_context && query_info.planner_context->getMutableQueryContext()) external_tables = query_info.planner_context->getMutableQueryContext()->getExternalTables(); - auto reading = std::make_unique( - column_names, - query_info, + auto reading = std::make_unique( + sample_block, + std::static_pointer_cast(shared_from_this()), storage_snapshot, context, - sample_block, - std::move(this_ptr), std::move(query_to_send), processed_stage, cluster, log, - external_tables, - /*is_whole_query_dispatch_*/ true); + std::move(external_tables)); query_plan.addStep(std::move(reading)); } -ASTPtr IStorageCluster::buildClusterTableFunctionAST( - const String & dispatch_cluster_name, const StorageSnapshotPtr & storage_snapshot, const ContextPtr & context) -{ - const auto & storage_id = getStorageID(); - ASTPtr identifier = storage_id.hasDatabase() - ? make_intrusive(storage_id.getDatabaseName(), storage_id.getTableName()) - : make_intrusive(storage_id.getTableName()); - - auto table_expression = make_intrusive(); - table_expression->database_and_table_name = identifier; - table_expression->children.push_back(identifier); - - auto tables_element = make_intrusive(); - tables_element->table_expression = table_expression; - tables_element->children.push_back(table_expression); - - auto tables = make_intrusive(); - tables->children.push_back(tables_element); - - auto select_query = make_intrusive(); - select_query->setExpression(ASTSelectQuery::Expression::TABLES, tables); - - ASTPtr query = select_query; - - /// updateQueryToSendIfNeeded resolves the cluster through getClusterName, which prefers the query-level - /// `object_storage_cluster` setting. Scope the intended cluster on a throwaway context copy so the result - /// does not depend on whatever the initiator's ambient settings happen to carry. - auto scoped_context = Context::createCopy(context); - scoped_context->setSetting("object_storage_cluster", dispatch_cluster_name); - - updateQueryToSendIfNeeded(query, storage_snapshot, scoped_context, /*make_cluster_function*/ true); - - auto * table_function = extractTableFunctionFromSelectQuery(query); - if (!table_function) - throw Exception(ErrorCodes::LOGICAL_ERROR, - "Distributed object-storage dispatch: failed to build an explicit cluster table function for {}", - storage_id.getNameForLogs()); - - return ASTPtr(table_function); -} IStorageCluster::RemoteCallVariables IStorageCluster::convertToRemote( ClusterPtr cluster, @@ -793,18 +750,26 @@ SinkToStoragePtr IStorageCluster::write( throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Method write is not supported by storage {}", getName()); } -void ReadFromCluster::initializePipeline(QueryPipelineBuilder & pipeline, const BuildQueryPipelineSettings &) +namespace { - const Scalars & scalars = context->hasQueryContext() ? context->getQueryContext()->getScalars() : Scalars{}; - const bool add_agg_info = processed_stage == QueryProcessingStage::WithMergeableState; - Pipes pipes; - auto new_context = updateSettings(context->getSettingsRef()); +/// The cluster-function transport, shared by ReadFromCluster and ReadFromClusterQuery: every replica is taken +/// as a shard, each gets one connection, and all of them pull work from the same task iterator. What differs +/// between the two callers is only what the query is and where the tasks come from. +Pipe buildClusterFunctionRemotePipe( + const ASTPtr & query_to_send, + const SharedHeader & output_header, + const ContextPtr & new_context, + const ClusterPtr & cluster, + QueryProcessingStage::Enum processed_stage, + const RemoteQueryExecutor::Extension & extension, + const std::optional & external_tables, + LoggerPtr log) +{ + const Scalars & scalars = new_context->hasQueryContext() ? new_context->getQueryContext()->getScalars() : Scalars{}; + const bool add_agg_info = processed_stage == QueryProcessingStage::WithMergeableState; const auto & current_settings = new_context->getSettingsRef(); - /// Mirrors the context-level normalization in updateSettings() onto query_to_send's own query-level - /// SETTINGS clause, which would otherwise re-override it on the worker (see that function's comment). - sanitizeObjectStorageClusterQuerySettings(query_to_send, is_whole_query_dispatch); auto timeouts = ConnectionTimeouts::getTCPTimeoutsWithFailover(current_settings); size_t replica_index = 0; @@ -812,10 +777,9 @@ void ReadFromCluster::initializePipeline(QueryPipelineBuilder & pipeline, const if (current_settings[Setting::max_parallel_replicas] > 1) max_replicas_to_use = std::min(max_replicas_to_use, current_settings[Setting::max_parallel_replicas].value); - createExtension(); - ProfileEvents::increment(ProfileEvents::Shards, max_replicas_to_use); + Pipes pipes; for (const auto & shard_info : cluster->getShardsInfo()) { if (pipes.size() >= max_replicas_to_use) @@ -838,14 +802,14 @@ void ReadFromCluster::initializePipeline(QueryPipelineBuilder & pipeline, const auto remote_query_executor = std::make_shared( std::vector{try_results.front()}, query_to_send->formatWithSecretsOneLine(), - getOutputHeader(), + output_header, new_context, /*throttler=*/nullptr, scalars, external_tables.has_value() ? *external_tables : Tables(), processed_stage, nullptr, - RemoteQueryExecutor::Extension{.task_iterator = extension->task_iterator, .replica_info = std::move(replica_info)}, + RemoteQueryExecutor::Extension{.task_iterator = extension.task_iterator, .replica_info = std::move(replica_info)}, shard_info.pool); remote_query_executor->setLogger(log); @@ -861,13 +825,74 @@ void ReadFromCluster::initializePipeline(QueryPipelineBuilder & pipeline, const if (pipes.empty()) throw Exception(ErrorCodes::ALL_CONNECTION_TRIES_FAILED, "Cannot connect to any replica for query execution"); - auto pipe = Pipe::unitePipes(std::move(pipes)); + return Pipe::unitePipes(std::move(pipes)); +} + +} + +void ReadFromCluster::initializePipeline(QueryPipelineBuilder & pipeline, const BuildQueryPipelineSettings &) +{ + auto new_context = updateSettings(context->getSettingsRef()); + + /// Mirrors the context-level normalization in updateSettings() onto query_to_send's own query-level + /// SETTINGS clause, which would otherwise re-override it on the worker (see that function's comment). + downgradeJoinModeInQuerySettings(query_to_send); + + createExtension(); + + auto pipe = buildClusterFunctionRemotePipe( + query_to_send, getOutputHeader(), new_context, cluster, processed_stage, *extension, external_tables, log); + for (const auto & processor : pipe.getProcessors()) processors.emplace_back(processor); pipeline.init(std::move(pipe)); } +ContextPtr ReadFromClusterQuery::updateSettings() const +{ + Settings new_settings{context->getSettingsRef()}; + + /// Cluster table functions should always skip unavailable shards. + new_settings[Setting::skip_unavailable_shards] = true; + + /// Every table in the dispatched query, driver included, must be read locally on the worker. Which one + /// takes its files from this step's task iterator is announced separately, per table, by name. + new_settings[Setting::object_storage_cluster] = ""; + + auto new_context = Context::createCopy(context); + new_context->setSettings(new_settings); + return new_context; +} + +void ReadFromClusterQuery::initializePipeline(QueryPipelineBuilder & pipeline, const BuildQueryPipelineSettings &) +{ + auto new_context = updateSettings(); + dropClusterFromQuerySettings(query_to_send); + + /// No predicate: this step's output is the whole query's result, so a filter over it says nothing about + /// which of the driver's files are needed. Recovering driver-only pruning means extracting the conjuncts + /// whose columns all come from the driver, which is not done yet -- every file of the driver is listed. + auto extension = driver_storage->getTaskIteratorExtension( + /*predicate=*/nullptr, /*filter=*/nullptr, new_context, cluster, driver_snapshot->metadata); + + auto pipe = buildClusterFunctionRemotePipe( + query_to_send, getOutputHeader(), new_context, cluster, processed_stage, extension, external_tables, log); + + for (const auto & processor : pipe.getProcessors()) + processors.emplace_back(processor); + + pipeline.init(std::move(pipe)); +} + +void ReadFromClusterQuery::describeActions(FormatSettings & format_settings) const +{ + std::string prefix(format_settings.offset, format_settings.indent_char); + format_settings.out << prefix << "Cluster: " << cluster->getName() << '\n'; + format_settings.out << prefix << "File tasks from: " << driver_storage->getStorageID().getNameForLogs() << '\n'; + format_settings.out << prefix << "Query: " << query_to_send->formatForLogging() << '\n'; +} + IStorageCluster::QueryTreeInfo IStorageCluster::getQueryTreeInfo(QueryTreeNodePtr query_tree, ContextPtr context) { QueryTreeInfo info; @@ -954,13 +979,8 @@ ContextPtr ReadFromCluster::updateSettings(const Settings & settings) /// Cluster table functions should always skip unavailable shards. new_settings[Setting::skip_unavailable_shards] = true; - /// The dispatched driver carries its cluster as an explicit table-function argument, so workers must not - /// also inherit `object_storage_cluster` -- it outranks a table's own cluster in getClusterName and would - /// make every partner table fan out again. Conversely, a ReadFromCluster reached after the candidate was - /// rejected must behave exactly like `allow`. - if (is_whole_query_dispatch) - new_settings[Setting::object_storage_cluster] = ""; - else if (new_settings[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) + /// A ReadFromCluster reached after whole-query dispatch was declined must behave exactly like `allow`. + if (new_settings[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) new_settings[Setting::object_storage_cluster_join_mode] = ObjectStorageClusterJoinMode::ALLOW; auto new_context = Context::createCopy(context); diff --git a/src/Storages/IStorageCluster.h b/src/Storages/IStorageCluster.h index 309437ba1ea7..4b276bf121f6 100644 --- a/src/Storages/IStorageCluster.h +++ b/src/Storages/IStorageCluster.h @@ -3,6 +3,7 @@ #include #include #include +#include #include namespace DB @@ -53,12 +54,13 @@ class IStorageCluster : public IStorage QueryProcessingStage::Enum getQueryProcessingStage(ContextPtr, QueryProcessingStage::Enum, const StorageSnapshotPtr &, SelectQueryInfo &) const override; - /// Reads a query the caller has already prepared (see Planner/buildDistributedObjectStorageQueryPlan.h), - /// through the same ReadFromCluster/task-iterator protocol read() uses. Unlike read(), does no query - /// preparation of its own: `query_to_send` is already self-contained and `sample_block` already computed. + /// Dispatches a whole query the caller has already prepared (see + /// Planner/buildDistributedObjectStorageQueryPlan.h) to this storage's cluster, over the same + /// task-iterator transport read() uses. This storage acts only as the cluster handle and the source of + /// file tasks; `query_to_send` is self-contained and its result is not this table's rows, so the read + /// goes through ReadFromClusterQuery rather than ReadFromCluster. void readPreparedClusterQuery( QueryPlan & query_plan, - const Names & column_names, const StorageSnapshotPtr & storage_snapshot, SelectQueryInfo & query_info, ContextPtr context, @@ -66,13 +68,6 @@ class IStorageCluster : public IStorage ASTPtr query_to_send, SharedHeader sample_block); - /// Builds a standalone, resolved `*Cluster(cluster_name, ...)` table-function call for this storage. Generic - /// across engines because the per-engine rewrite is done by the virtual updateQueryToSendIfNeeded, which - /// also supplies credentials, structure and format arguments. Works on a throwaway single-table SELECT of - /// its own, so no query in flight is touched. - ASTPtr buildClusterTableFunctionAST( - const String & dispatch_cluster_name, const StorageSnapshotPtr & storage_snapshot, const ContextPtr & context); - bool isRemote() const final { return true; } bool supportsSubcolumns() const override { return true; } bool supportsOptimizationToSubcolumns() const override { return false; } @@ -169,8 +164,7 @@ class ReadFromCluster : public SourceStepWithFilter QueryProcessingStage::Enum processed_stage_, ClusterPtr cluster_, LoggerPtr log_, - std::optional external_tables_, - bool is_whole_query_dispatch_ = false) + std::optional external_tables_) : SourceStepWithFilter( std::move(sample_block), column_names_, @@ -183,7 +177,6 @@ class ReadFromCluster : public SourceStepWithFilter , cluster(std::move(cluster_)) , log(log_) , external_tables(external_tables_) - , is_whole_query_dispatch(is_whole_query_dispatch_) { } @@ -198,12 +191,56 @@ class ReadFromCluster : public SourceStepWithFilter std::shared_ptr listing_filter_dag; std::optional external_tables; - /// Set only by readPreparedClusterQuery. This step's output is then the whole dispatched query's result - /// rather than one table's rows, which changes how filters may be used (see applyFilters, createExtension). - bool is_whole_query_dispatch = false; - void createExtension(); ContextPtr updateSettings(const Settings & settings); }; + +/// Dispatches one already-prepared query to a cluster, giving every node a share of one table's files. +/// +/// Distinct from ReadFromCluster, which reads a single table: here the output is a whole query's result, so +/// there is no table expression to push filters into and no per-table column mapping. `driver_storage` and +/// `driver_snapshot` describe only the table whose files are handed out, never the step's output. +class ReadFromClusterQuery : public ISourceStep +{ +public: + std::string getName() const override { return "ReadFromClusterQuery"; } + void initializePipeline(QueryPipelineBuilder & pipeline, const BuildQueryPipelineSettings &) override; + void describeActions(FormatSettings & format_settings) const override; + + ReadFromClusterQuery( + SharedHeader output_header_, + std::shared_ptr driver_storage_, + StorageSnapshotPtr driver_snapshot_, + ContextPtr context_, + ASTPtr query_to_send_, + QueryProcessingStage::Enum processed_stage_, + ClusterPtr cluster_, + LoggerPtr log_, + std::optional external_tables_) + : ISourceStep(std::move(output_header_)) + , driver_storage(std::move(driver_storage_)) + , driver_snapshot(std::move(driver_snapshot_)) + , context(std::move(context_)) + , query_to_send(std::move(query_to_send_)) + , processed_stage(processed_stage_) + , cluster(std::move(cluster_)) + , log(log_) + , external_tables(std::move(external_tables_)) + { + } + +private: + std::shared_ptr driver_storage; + StorageSnapshotPtr driver_snapshot; + ContextPtr context; + ASTPtr query_to_send; + QueryProcessingStage::Enum processed_stage; + ClusterPtr cluster; + LoggerPtr log; + std::optional external_tables; + + ContextPtr updateSettings() const; +}; + } From 577418b7fd931bfeb78868e2af04ece845b2f492 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Wed, 23 Sep 2026 20:30:26 +0530 Subject: [PATCH 07/15] Key the query condition cache by object path, not by file name `QueryConditionCache` is keyed by `(table uuid, part name, condition hash)`. For `MergeTree` the part name is unique within a table, so passing a name is enough. Object storage paths are hierarchical, and `StorageObjectStorageSource` was passing `ObjectInfo::getFileName`, which is only the last component. A Hive-partitioned dataset repeats the same file name in every partition -- `day=2025-02-05/part-00000.parquet` and `day=2025-02-27/part-00000.parquet` -- so all of them shared a single cache entry. The first partition whose row groups all failed the condition wrote "nothing matches" under that shared key, and every later query read it back for its namesakes and skipped them without opening them. The result was a table that answered correctly once and returned no rows from those files afterwards, with no error and nothing logged above debug level. Only reachable when the table id is stable across queries, so a table (or a data lake catalog table) hits it while a table function does not: a table function gets a fresh id per query, and its entries never collide. Pass `ObjectInfo::getPath` on both the read and the write side instead. Co-Authored-By: Claude Opus 5 (1M context) --- .../StorageObjectStorageSource.cpp | 9 ++++-- ...he_object_storage_same_file_name.reference | 3 ++ ...on_cache_object_storage_same_file_name.sql | 28 +++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 tests/queries/0_stateless/05053_query_condition_cache_object_storage_same_file_name.reference create mode 100644 tests/queries/0_stateless/05053_query_condition_cache_object_storage_same_file_name.sql diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 6936fa38b64f..feb11d7e1721 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -682,7 +682,12 @@ Chunk StorageObjectStorageSource::generate() auto query_condition_cache = Context::getGlobalContextInstance()->getQueryConditionCache(); query_condition_cache->write( storage_id.uuid, - object_info->getFileName(), + /// Full path, not the file name: object storage paths are hierarchical and a + /// Hive-partitioned dataset repeats the same file name in every partition + /// (`day=.../part-00000.parquet`). Keying by name alone makes those files share + /// one cache entry, so a partition that matched nothing marks its namesakes as + /// having no matching row groups and they are skipped unread. + object_info->getPath(), *format_filter_info->condition_hash, format_filter_info->filter_actions_dag->dumpNames(), unmatched_ranges, @@ -827,7 +832,7 @@ StorageObjectStorageSource::ReaderHolder StorageObjectStorageSource::createReade if (query_condition_cache && !object_info->file_bucket_info) { auto matching_marks = query_condition_cache->read( - storage_id.uuid, object_info->getFileName(), *format_filter_info->condition_hash); + storage_id.uuid, object_info->getPath(), *format_filter_info->condition_hash); if (matching_marks.has_value()) { const auto & marks = *matching_marks; diff --git a/tests/queries/0_stateless/05053_query_condition_cache_object_storage_same_file_name.reference b/tests/queries/0_stateless/05053_query_condition_cache_object_storage_same_file_name.reference new file mode 100644 index 000000000000..75d54317d732 --- /dev/null +++ b/tests/queries/0_stateless/05053_query_condition_cache_object_storage_same_file_name.reference @@ -0,0 +1,3 @@ +1000 +1000 +1000 diff --git a/tests/queries/0_stateless/05053_query_condition_cache_object_storage_same_file_name.sql b/tests/queries/0_stateless/05053_query_condition_cache_object_storage_same_file_name.sql new file mode 100644 index 000000000000..c33bfe640d76 --- /dev/null +++ b/tests/queries/0_stateless/05053_query_condition_cache_object_storage_same_file_name.sql @@ -0,0 +1,28 @@ +-- Tags: no-fasttest +-- Tag no-fasttest: Depends on AWS + +-- The query condition cache is keyed per file. A Hive-partitioned dataset repeats the same file name in +-- every partition, so keying by name alone would make all of them share one entry: the first partition +-- that matches nothing marks its namesakes as having no matching row groups, and every later query skips +-- them unread. Reading the same table twice must return the same rows. + +SET s3_truncate_on_insert = 1; +SET use_query_condition_cache = 1; + +INSERT INTO FUNCTION s3(s3_conn, filename='05053_qcc/day=2025-02-27/part-00000.parquet', format=Parquet) + SELECT toDateTime('2025-02-27 00:00:00') + number AS ts, number AS v FROM numbers(1000); + +INSERT INTO FUNCTION s3(s3_conn, filename='05053_qcc/day=2025-02-05/part-00000.parquet', format=Parquet) + SELECT toDateTime('2025-02-05 00:00:00') + number AS ts, number AS v FROM numbers(1000); + +DROP TABLE IF EXISTS t_05053_qcc; + +-- A table, not a table function: the cache entry only outlives a query when the table id is stable. +CREATE TABLE t_05053_qcc (ts DateTime, v UInt32) + ENGINE = S3(s3_conn, filename='05053_qcc/**.parquet', format=Parquet); + +SELECT count() FROM t_05053_qcc WHERE ts >= '2025-02-05 00:00:00' AND ts < '2025-02-06 00:00:00'; +SELECT count() FROM t_05053_qcc WHERE ts >= '2025-02-05 00:00:00' AND ts < '2025-02-06 00:00:00'; +SELECT count() FROM t_05053_qcc WHERE ts >= '2025-02-05 00:00:00' AND ts < '2025-02-06 00:00:00'; + +DROP TABLE t_05053_qcc; From 0868690ab69464ad9a6b9053be538882019e21d4 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Wed, 23 Sep 2026 20:31:24 +0530 Subject: [PATCH 08/15] Resolve the driver's configuration before listing its files A data lake catalog table is constructed with `lazy_init`, so its configuration -- and with it the path the listing walks -- is resolved on first use rather than at construction. Ordinary reads reach the listing through `StorageObjectStorage`, which does that resolution itself. Whole-query dispatch calls `getTaskIteratorExtension` directly and skips it, so a driver that nothing else had touched yet would list no files, hand every worker an empty queue, and return no rows without an error. `lazyInitializeIfNeeded` is a no-op once the configuration is resolved, so this is safe on the paths that already did it. Co-Authored-By: Claude Opus 5 (1M context) --- src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp index 2af2106d6fd1..77f22e482727 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp @@ -684,6 +684,12 @@ RemoteQueryExecutor::Extension StorageObjectStorageCluster::getTaskIteratorExten ClusterPtr cluster, StorageMetadataPtr storage_metadata_snapshot) const { + /// A catalog table is built with lazy_init, so nothing has resolved the configuration -- and with it the + /// path this listing walks -- by the time we get here. `read` reaches it through StorageObjectStorage, + /// which does its own lazy init; whole-query dispatch calls this directly and would otherwise list no + /// files at all, handing every worker an empty queue and silently returning no rows. Idempotent. + configuration->lazyInitializeIfNeeded(object_storage, local_context); + auto iterator = StorageObjectStorageSource::createFileIterator( configuration, configuration->getQuerySettings(local_context), From f9dc42265dfc12aae4ec98cd94f027be26b52410 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Wed, 23 Sep 2026 20:47:07 +0530 Subject: [PATCH 09/15] Drop the converting step from whole-query dispatch Inherited from `buildQueryPlanForParallelReplicas`, which needs it: that one rewrites the table expression before serializing the query, so the header it computes and the header the remote plan produces come from different trees and can disagree. Whole-query dispatch sends the query as written, and `readPreparedClusterQuery` is given `remote_header` as the source step's own output header, so the plan's current header is already that block and the conversion is an identity. It is not free: it puts an `ExpressionStep` over blocks carrying aggregate state at `WithMergeableState` for no benefit. Co-Authored-By: Claude Opus 5 (1M context) --- ...buildDistributedObjectStorageQueryPlan.cpp | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp index 1a7fb0e0ee5f..adae4817d1d2 100644 --- a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp @@ -3,11 +3,9 @@ #include #include #include -#include #include #include #include -#include #include #include #include @@ -114,21 +112,10 @@ std::optional buildDistributedObjectStorageQueryPlan( query_to_send, remote_header); - /// Kept from the parallel-replicas shape. With the query no longer rewritten the two headers are built from - /// the same tree and should already agree, so this is normally an identity; it stays as the one place that - /// would catch a divergence rather than let it reach the caller's finalization. - auto converting_actions = ActionsDAG::makeConvertingActions( - result.query_plan.getCurrentHeader()->getColumnsWithTypeAndName(), - remote_header->getColumnsWithTypeAndName(), - ActionsDAG::MatchColumnsMode::Position, - context, - false, - false, - nullptr); - - auto converting_step = std::make_unique(result.query_plan.getCurrentHeader(), std::move(converting_actions)); - converting_step->setStepDescription("Convert columns to the original query's header"); - result.query_plan.addStep(std::move(converting_step)); + /// No converting step, unlike buildQueryPlanForParallelReplicas: that one rewrites the table expression + /// before serializing, so its two headers are built from different trees and can diverge. Here the query + /// is sent as written and `readPreparedClusterQuery` is given `remote_header` as the source step's own + /// output header, so the plan's current header is that same block. return result; } From 41d76dddb909b591fb334a4ad6d1dcbe6ed98c00 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Wed, 23 Sep 2026 20:47:17 +0530 Subject: [PATCH 10/15] Clear the driver name on cluster reads that are not a dispatch `object_storage_distributed_driver_database` and `_table` name the one table whose files the initiator hands out. Whole-query dispatch sets them on its own context, from its own step. Nothing marks them internal -- ClickHouse has no such tier -- so a user can set them by hand on any query. On an ordinary cluster read that is mostly harmless, because they are only read on a worker, and a worker only reaches that state when the initiator supplied a task iterator. `ReadFromCluster` does supply one: under parallel replicas its workers see `collaborate_with_initiator`, so a name that reached them would make the table so named read this step's file-task queue instead of listing its own files. Clear both in `ReadFromCluster::updateSettings`, and again from the query's own `SETTINGS` clause, which a worker applies on top of the context settings and would otherwise use to restore them. Co-Authored-By: Claude Opus 5 (1M context) --- src/Storages/IStorageCluster.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/Storages/IStorageCluster.cpp b/src/Storages/IStorageCluster.cpp index a9339b9ec3d5..90203fccbcdf 100644 --- a/src/Storages/IStorageCluster.cpp +++ b/src/Storages/IStorageCluster.cpp @@ -75,6 +75,8 @@ namespace Setting extern const SettingsBool object_storage_remote_initiator; extern const SettingsString object_storage_remote_initiator_cluster; extern const SettingsObjectStorageClusterJoinMode object_storage_cluster_join_mode; + extern const SettingsString object_storage_distributed_driver_database; + extern const SettingsString object_storage_distributed_driver_table; extern const SettingsString object_storage_cluster; } @@ -181,6 +183,20 @@ void downgradeJoinModeInQuerySettings(ASTPtr & query) dropEmptySettings(query, changed); } +/// A worker applies the query's own SETTINGS clause on top of the context settings, so a driver name written +/// into the query text by hand survives the clearing `ReadFromCluster::updateSettings` does -- see there for +/// why it must not reach a worker of this read. +void dropDriverAnnouncementFromQuerySettings(ASTPtr & query) +{ + auto * settings = getQuerySettings(query); + if (!settings) + return; + + bool changed = settings->changes.removeSetting("object_storage_distributed_driver_database"); + changed |= settings->changes.removeSetting("object_storage_distributed_driver_table"); + dropEmptySettings(query, changed); +} + /// `object_storage_cluster` outranks a table's own cluster in getClusterName, so leaving it set would make /// every table in a dispatched query fan out again from each worker. void dropClusterFromQuerySettings(ASTPtr & query) @@ -837,6 +853,7 @@ void ReadFromCluster::initializePipeline(QueryPipelineBuilder & pipeline, const /// Mirrors the context-level normalization in updateSettings() onto query_to_send's own query-level /// SETTINGS clause, which would otherwise re-override it on the worker (see that function's comment). downgradeJoinModeInQuerySettings(query_to_send); + dropDriverAnnouncementFromQuerySettings(query_to_send); createExtension(); @@ -983,6 +1000,13 @@ ContextPtr ReadFromCluster::updateSettings(const Settings & settings) if (new_settings[Setting::object_storage_cluster_join_mode] == ObjectStorageClusterJoinMode::DISTRIBUTED) new_settings[Setting::object_storage_cluster_join_mode] = ObjectStorageClusterJoinMode::ALLOW; + /// Only whole-query dispatch may name a driver, and it sends that name from its own step. This read + /// supplies a task iterator too, so its workers see `collaborate_with_initiator` and would act on a name + /// that reached them: the table so named would read this step's file-task queue instead of listing its + /// own files. Nothing marks these settings internal, so clear them rather than trust no one set them. + new_settings[Setting::object_storage_distributed_driver_database] = ""; + new_settings[Setting::object_storage_distributed_driver_table] = ""; + auto new_context = Context::createCopy(context); new_context->setSettings(new_settings); return new_context; From 759bc7ca63f3c78ee9f77ecf86ce318ba2adf0d2 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Thu, 24 Sep 2026 15:34:20 +0530 Subject: [PATCH 11/15] Strip the driver name from a dispatched query's own SETTINGS too `ReadFromCluster` cleared the driver announcement from both the context and the query text, but whole-query dispatch cleared neither from the text. Its own announcement travels in the context, and a worker applies the query's `SETTINGS` clause on top of that -- so a name written into the query by hand won the tie and the initiator's choice was overridden. The table the user named, not the one the planner picked, would then read the file-task queue, silently returning wrong rows rather than failing. The three helpers that did this were applied ad hoc per path, which is what made it easy to give one path a subset of the other's. Replace them with one function per kind of remote read, named for that kind, so the set of normalizations is chosen once: - `prepareOrdinaryClusterQueryForRemoteExecution` -- downgrade the join mode, drop the driver name - `prepareWholeQueryDispatchForRemoteExecution` -- drop the cluster, drop the driver name Co-Authored-By: Claude Opus 5 (1M context) --- src/Storages/IStorageCluster.cpp | 39 +++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/src/Storages/IStorageCluster.cpp b/src/Storages/IStorageCluster.cpp index 90203fccbcdf..c743dd9fe518 100644 --- a/src/Storages/IStorageCluster.cpp +++ b/src/Storages/IStorageCluster.cpp @@ -142,8 +142,10 @@ ActionsDAG andListingFilterDAGs(ActionsDAG first, ActionsDAG second) namespace { -/// The worker applies a query's own SETTINGS clause on top of the context settings, so a normalization made -/// only in updateSettings would be undone. These two apply the same change to the query text. +/// A worker applies the query's own SETTINGS clause on top of the settings it received, so anything +/// normalized only in the context is undone by a value written into the query text. Every such normalization +/// therefore comes in a pair, and the two `prepare...ForRemoteExecution` functions below are the query-text +/// half -- one per kind of remote read, so neither can be given a subset of the other's by accident. ASTSetQuery * getQuerySettings(ASTPtr & query) { auto * select_query = query->as(); @@ -162,7 +164,6 @@ void dropEmptySettings(ASTPtr & query, bool changed) select_query->setExpression(ASTSelectQuery::Expression::SETTINGS, {}); } -/// A ReadFromCluster reached after whole-query dispatch was declined must behave exactly like `allow`. void downgradeJoinModeInQuerySettings(ASTPtr & query) { auto * settings = getQuerySettings(query); @@ -183,9 +184,6 @@ void downgradeJoinModeInQuerySettings(ASTPtr & query) dropEmptySettings(query, changed); } -/// A worker applies the query's own SETTINGS clause on top of the context settings, so a driver name written -/// into the query text by hand survives the clearing `ReadFromCluster::updateSettings` does -- see there for -/// why it must not reach a worker of this read. void dropDriverAnnouncementFromQuerySettings(ASTPtr & query) { auto * settings = getQuerySettings(query); @@ -197,8 +195,6 @@ void dropDriverAnnouncementFromQuerySettings(ASTPtr & query) dropEmptySettings(query, changed); } -/// `object_storage_cluster` outranks a table's own cluster in getClusterName, so leaving it set would make -/// every table in a dispatched query fan out again from each worker. void dropClusterFromQuerySettings(ASTPtr & query) { auto * settings = getQuerySettings(query); @@ -208,6 +204,26 @@ void dropClusterFromQuerySettings(ASTPtr & query) dropEmptySettings(query, settings->changes.removeSetting("object_storage_cluster")); } +/// An ordinary cluster read, including one reached after whole-query dispatch was declined. It must behave +/// exactly like `allow`, and it must not carry a driver name: this read supplies a task iterator, so its +/// workers see `collaborate_with_initiator` and a table matching that name would read this step's file-task +/// queue instead of listing its own files. +void prepareOrdinaryClusterQueryForRemoteExecution(ASTPtr & query) +{ + downgradeJoinModeInQuerySettings(query); + dropDriverAnnouncementFromQuerySettings(query); +} + +/// A whole-query dispatch. `object_storage_cluster` outranks a table's own cluster in getClusterName, so +/// leaving it set would make every table in the dispatched query fan out again from each worker. The driver +/// name goes too: the initiator puts the one it chose in the settings it sends, and a value in the query text +/// would override it on the worker and hand the queue to a table the planner did not pick. +void prepareWholeQueryDispatchForRemoteExecution(ASTPtr & query) +{ + dropClusterFromQuerySettings(query); + dropDriverAnnouncementFromQuerySettings(query); +} + } void ReadFromCluster::applyFilters(ActionDAGNodes added_filter_nodes) @@ -850,10 +866,7 @@ void ReadFromCluster::initializePipeline(QueryPipelineBuilder & pipeline, const { auto new_context = updateSettings(context->getSettingsRef()); - /// Mirrors the context-level normalization in updateSettings() onto query_to_send's own query-level - /// SETTINGS clause, which would otherwise re-override it on the worker (see that function's comment). - downgradeJoinModeInQuerySettings(query_to_send); - dropDriverAnnouncementFromQuerySettings(query_to_send); + prepareOrdinaryClusterQueryForRemoteExecution(query_to_send); createExtension(); @@ -885,7 +898,7 @@ ContextPtr ReadFromClusterQuery::updateSettings() const void ReadFromClusterQuery::initializePipeline(QueryPipelineBuilder & pipeline, const BuildQueryPipelineSettings &) { auto new_context = updateSettings(); - dropClusterFromQuerySettings(query_to_send); + prepareWholeQueryDispatchForRemoteExecution(query_to_send); /// No predicate: this step's output is the whole query's result, so a filter over it says nothing about /// which of the driver's files are needed. Recovering driver-only pruning means extracting the conjuncts From 2c3f89a44fb2986c5c9e23c4f99b1452f73f3eff Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Thu, 24 Sep 2026 15:34:20 +0530 Subject: [PATCH 12/15] Mark the driver announcement settings IMPORTANT `object_storage_distributed_driver_database` and `_table` tell a worker which table reads from the initiator's file-task queue. A server that does not know a setting ignores it with a warning unless it is `IMPORTANT`, so during a rolling upgrade a new initiator could dispatch to an older worker that silently dropped the announcement, found no driver, read every file of every table, and returned each row once per node. `IMPORTANT` makes that worker refuse the query instead. The tier bits and the `IMPORTANT` bit are disjoint (`BaseSettingsHelpers::getTier` masks with `Flags::TIER`), so the settings stay `EXPERIMENTAL`. Co-Authored-By: Claude Opus 5 (1M context) --- src/Core/Settings.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 93689d531677..ed0260f3074d 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -8362,10 +8362,13 @@ Internal. Set by the initiator on a query dispatched by `object_storage_cluster_ database of the one table whose files are distributed across the cluster. Together with `object_storage_distributed_driver_table` it tells a worker which table reads from the initiator's file-task queue; every other table in the same query is read in full, locally. Not meant to be set by hand. -)", EXPERIMENTAL) \ + +`IMPORTANT` because a worker that silently ignored it would find no driver, read every file of every table, and +return each row once per node. A server too old to know the setting must refuse the query instead. +)", EXPERIMENTAL | IMPORTANT) \ DECLARE(String, object_storage_distributed_driver_table, "", R"( Internal. The table name counterpart of `object_storage_distributed_driver_database`. Not meant to be set by hand. -)", EXPERIMENTAL) \ +)", EXPERIMENTAL | IMPORTANT) \ DECLARE(UInt64, object_storage_max_nodes, 0, R"( Limit for hosts used for request in object storage cluster table functions - azureBlobStorageCluster, s3Cluster, hdfsCluster, etc. Possible values: From 922503049b5954e4e7cebc6e9f23155e746c9243 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Thu, 24 Sep 2026 15:34:39 +0530 Subject: [PATCH 13/15] Let a whole-query dispatch own the file-task queue outright Two mechanisms can put a storage on an initiator's file-task queue: being the announced driver of a whole-query dispatch, or being an ordinary cluster read under parallel replicas. They were combined with `||`, so both could answer yes for different tables in the same query -- and there is one queue, holding one table's files. A partner that qualified for parallel replicas would read the driver's files under its own schema. The reachability of that depended on conditions no invariant pinned: the database's own `object_storage_cluster` leaving `cluster_name_` non-empty on a worker, plus the parallel-replica settings. Rather than rely on those not lining up, make the dispatch decide alone while it is in effect: its announced driver consumes the queue, every other table in that query lists its own files. This is the shape parallel replicas already uses for the same hazard -- one table is chosen, and the mechanism is explicitly disabled for every other (`PlannerJoinTree.cpp`, `parallel_replicas_table`). Co-Authored-By: Claude Opus 5 (1M context) --- .../ObjectStorage/StorageObjectStorageCluster.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp index 77f22e482727..863d82378e3c 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp @@ -286,12 +286,14 @@ StorageObjectStorageCluster::StorageObjectStorageCluster( && context_->canUseTaskBasedParallelReplicas() && !context_->isDistributed(); - /// Two ways this storage ends up reading from the initiator's file-task queue rather than listing its own - /// files: it is the announced driver of a whole-query dispatch, or it is an ordinary cluster read under - /// parallel replicas. The first is decided per table, by name; the second by the settings alone. - bool can_use_distributed_iterator = - isAnnouncedDistributedJoinDriver(context_, table_id_) - || (context_->getClientInfo().collaborate_with_initiator && can_use_parallel_replicas); + /// Two mechanisms can put a storage on an initiator's file-task queue: being the announced driver of a + /// whole-query dispatch, or being an ordinary cluster read under parallel replicas. They must not both + /// get a say, because there is one queue and it holds one table's files. A dispatch owns the decision + /// outright while it is in effect: its announced driver consumes the queue and every other table in that + /// query lists its own files, whatever the parallel-replica settings would otherwise allow. + bool can_use_distributed_iterator = isDistributedJoinDispatchWorker(context_) + ? isAnnouncedDistributedJoinDriver(context_, table_id_) + : (context_->getClientInfo().collaborate_with_initiator && can_use_parallel_replicas); pure_storage = std::make_shared( configuration, From 500037e764a11369bf93e018c55d7a824704ffff Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Thu, 24 Sep 2026 15:34:39 +0530 Subject: [PATCH 14/15] Test the dispatch that exists, not the one it replaced The integration test still asserted that a dispatched query contains `icebergS3Cluster(...)` -- the previous design, where the driver was rewritten into a cluster table function. The current one sends the driver as the catalog table the user wrote and names it in the settings, so that assertion could only fail. It was not caught because the branch's `amd_debug` build died in Checkout Submodules and the integration jobs never ran. Rewrite `_assert_dispatched_whole` around what is now true: a worker received the whole query carrying the announcement for the expected driver, no query contains a cluster table function, and no secondary query lacks the JOIN (which is what a table fanning out again from a worker would look like). In the unit tests the same check was made by proxy -- a fake table function was registered so the plan text could be searched for it. Replace that with a tripwire on the rewrite hook itself, which is both the thing being asserted and enough to delete `FakeDriverTableFunction`, its factory registration and its AST surgery. It is reset per plan so it keeps meaning something for the tests that fall back to an ordinary cluster read, where calling the hook is correct. Also corrects comments in the candidate finder and its test that still described the driver as being rewritten. Co-Authored-By: Claude Opus 5 (1M context) --- .../findDistributedObjectStorageCandidate.cpp | 5 +- ...stributed_object_storage_join_dispatch.cpp | 83 +++++-------------- ...d_distributed_object_storage_candidate.cpp | 4 +- .../integration/test_database_iceberg/test.py | 63 ++++++++------ 4 files changed, 61 insertions(+), 94 deletions(-) diff --git a/src/Planner/findDistributedObjectStorageCandidate.cpp b/src/Planner/findDistributedObjectStorageCandidate.cpp index 2fa7bf646407..470167d1b473 100644 --- a/src/Planner/findDistributedObjectStorageCandidate.cpp +++ b/src/Planner/findDistributedObjectStorageCandidate.cpp @@ -28,8 +28,9 @@ namespace Setting namespace { -/// Dispatch drops the initiator's row policies: the driver is rewritten into an explicit `*Cluster()` call and -/// every partner is re-resolved independently by each worker, so neither carries the policy across. Reject. +/// Dispatch drops the initiator's row policies: every table in the dispatched query, driver included, is +/// re-resolved independently by each worker, and a policy the initiator would have applied does not travel +/// with the query text. Reject. bool hasEffectiveRowPolicy(const TableNode & table_node, const ContextPtr & context) { const auto & storage_id = table_node.getStorageID(); diff --git a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp index 341949b8b704..b689efd4d1f2 100644 --- a/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp +++ b/src/Planner/tests/gtest_distributed_object_storage_join_dispatch.cpp @@ -51,29 +51,6 @@ NamesAndTypesList lookupColumns() struct State; -/// The builder resolves its driver replacement via QueryAnalysisPass, which -- like any table function -/// reference -- looks `fakeDriverFunction` up in the real TableFunctionFactory (QueryAnalyzer::resolveTableFunction()). -/// Registers a minimal stand-in that just returns the test's own driver storage, so the resolved -/// TableFunctionNode carries the same columns real production code would get back from e.g. icebergS3Cluster(). -/// executeImpl() is defined out-of-line, after State, since it needs State to be a complete type. -class FakeDriverTableFunction : public ITableFunction -{ -public: - static constexpr auto name = "fakeDriverFunction"; - std::string getName() const override { return name; } - bool hasStaticStructure() const override { return true; } - ColumnsDescription getActualTableStructure(ContextPtr, bool) const override { return ColumnsDescription{driverColumns()}; } - -protected: - /// The default implementation looks getStorageEngineName() up in StorageFactory for source-access checking, - /// which "FakeDriverStorage" (a test-only stand-in, never registered there) doesn't have. - std::optional getSourceAccessObject() const override { return std::nullopt; } - -private: - StoragePtr executeImpl(const ASTPtr &, ContextPtr, const std::string &, ColumnsDescription, bool) const override; - const char * getStorageEngineName() const override { return "FakeDriverStorage"; } -}; - /// A DatabaseMemory that reports itself as a DataLake catalog: that is what makes the tables inside it eligible /// for dispatch (findDistributedObjectStorageCandidate asks DatabaseCatalog::isDatalakeCatalog). class FakeDataLakeDatabase : public DatabaseWithOwnTablesBase @@ -112,33 +89,15 @@ class FakeDriverStorage : public IStorageCluster return {}; } -protected: - /// Mirrors StorageObjectStorageCluster::updateQueryForDistributedEngineIfNeeded()'s alias handling - /// closely enough to exercise buildDistributedObjectStorageQueryPlan.cpp's own fallback-alias fix: - /// transfers whatever alias the driver's table identifier already had (empty if none) onto the - /// replacement table function, exactly like the real rewrite. - void updateQueryToSendIfNeeded(ASTPtr & query, const StorageSnapshotPtr &, const ContextPtr &, bool make_cluster_function) override + /// Whole-query dispatch must never ask a storage to rewrite itself into a cluster table function -- + /// that was the previous design, and the driver now travels as a plain table name announced in the + /// settings. Trips if anything on the dispatch path calls the rewrite hook again. + void updateQueryToSendIfNeeded(ASTPtr &, const StorageSnapshotPtr &, const ContextPtr &, bool) override { - if (!make_cluster_function) - return; - - auto * select_query = query->as(); - if (!select_query || !select_query->tables()) - return; - - auto * tables = select_query->tables()->as(); - auto * table_expression = tables->children.at(0)->as()->table_expression->as(); - if (!table_expression || !table_expression->database_and_table_name) - return; - - auto table_alias = table_expression->database_and_table_name->tryGetAlias(); - auto function_ast = makeASTFunction("fakeDriverFunction"); - function_ast->setAlias(table_alias); - - table_expression->database_and_table_name = nullptr; - table_expression->table_function = function_ast; - table_expression->children[0] = function_ast; + rewrite_hook_called = true; } + + mutable bool rewrite_hook_called = false; }; /// Stand-in for a second table from the same DataLake catalog, e.g. ice.geo_location_lookup. @@ -217,13 +176,10 @@ struct State client_info.query_kind = ClientInfo::QueryKind::INITIAL_QUERY; context->setClientInfo(client_info); - /// The driver rewrite resolves its replacement TableFunctionNode via QueryAnalysisPass, which (like any - /// table function resolution -- QueryAnalyzer::resolveTableFunction()) requires a real query context - /// (context->getQueryContext() throws THERE_IS_NO_QUERY otherwise); every real query already has one. + /// Table resolution during analysis requires a real query context (context->getQueryContext() throws + /// THERE_IS_NO_QUERY otherwise); every real query already has one. context->makeQueryContext(); - TableFunctionFactory::instance().registerFunction(FunctionDocumentation{}); - static constexpr auto database_name = "distributed_object_storage_join_dispatch_test_db"; static constexpr auto cluster_name = "vig-test"; @@ -242,14 +198,13 @@ struct State } }; -StoragePtr FakeDriverTableFunction::executeImpl(const ASTPtr &, ContextPtr, const std::string &, ColumnsDescription, bool) const -{ - return State::instance().driver; -} - /// getQueryPlan() returns a reference into the interpreter's own move-only plan, so build+explain in one scope. String planAndExplain(const String & query, const ContextMutablePtr & context) { + /// Scope the tripwire to this plan: the driver storage is shared across tests, and a query that falls back + /// to an ordinary cluster read is *supposed* to call the rewrite hook. + State::instance().driver->rewrite_hook_called = false; + ParserSelectQuery parser; ASTPtr ast = parseQuery(parser, query, 1000, 1000, 1000000); auto query_tree = buildQueryTree(ast, context); @@ -346,8 +301,8 @@ TEST(DistributedObjectStorageJoinDispatch, DriverCrossesTheWireUnrewrittenAndNam ASSERT_NE(plan_text.find("ReadFromCluster"), String::npos) << plan_text; - EXPECT_EQ(plan_text.find("fakeDriverFunction("), String::npos) - << "expected the driver to stay an ordinary catalog table, not be rewritten to a cluster function, got:\n" << plan_text; + EXPECT_FALSE(State::instance().driver->rewrite_hook_called) + << "dispatch rewrote the driver into a cluster table function; it must be sent as a plain table name"; size_t driver_mentions = 0; for (size_t pos = plan_text.find("driver"); pos != String::npos; pos = plan_text.find("driver", pos + 1)) @@ -512,8 +467,8 @@ TEST(DistributedObjectStorageJoinDispatch, DispatchesBuriedDriverWithoutRewritin "GROUP BY transaction_event.id", state.context); - EXPECT_EQ(plan_text.find("fakeDriverFunction("), String::npos) - << "expected no table to be rewritten into a cluster function, got:\n" << plan_text; + EXPECT_FALSE(State::instance().driver->rewrite_hook_called) + << "dispatch rewrote the driver into a cluster table function; it must be sent as a plain table name"; EXPECT_NE(plan_text.find("safe_lookup"), String::npos) << "expected safe_lookup to remain an ordinary catalog identifier, got:\n" << plan_text; EXPECT_NE(plan_text.find("dim2"), String::npos) << "expected dim2 to remain an ordinary catalog identifier, got:\n" << plan_text; @@ -546,8 +501,8 @@ TEST(DistributedObjectStorageJoinDispatch, BuriedDriverWithCommonTableExpression << "expected stock final-merge aggregation on top of the dispatched read, got:\n" << plan_text; /// No dangling CTE name: every CTE body must appear inlined in the forwarded query, and nothing is rewritten. - EXPECT_EQ(plan_text.find("fakeDriverFunction("), String::npos) - << "expected no table to be rewritten into a cluster function, got:\n" << plan_text; + EXPECT_FALSE(State::instance().driver->rewrite_hook_called) + << "dispatch rewrote the driver into a cluster table function; it must be sent as a plain table name"; EXPECT_NE(plan_text.find("safe_lookup"), String::npos) << plan_text; EXPECT_NE(plan_text.find("dim2"), String::npos) << plan_text; diff --git a/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp b/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp index 683cd5e04d79..16b903734b4e 100644 --- a/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp +++ b/src/Planner/tests/gtest_find_distributed_object_storage_candidate.cpp @@ -290,8 +290,8 @@ TEST(FindDistributedObjectStorageCandidate, RejectsBuriedDriverWithoutSelectAcce EXPECT_FALSE(findDistributedObjectStorageCandidate(query_tree, context).has_value()); } -/// A row policy on the driver itself: the driver is rewritten to its explicit *Cluster() form and no longer -/// resolves via its catalog identity, so the policy can never be reattached on the worker. +/// A row policy on the driver itself: a worker re-resolves the driver from its own catalog and the initiator's +/// policy does not travel with the query text, so it could never be reapplied there. TEST(FindDistributedObjectStorageCandidate, RejectsDriverWithRowPolicy) { const auto & state = State::instance(); diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 7cde0ee5abf3..001b4a90f488 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1108,49 +1108,62 @@ def _setup_distributed_join_tables(started_cluster, nodes, test_ref): ) -def _assert_dispatched_whole(nodes, query_id): - """Checks the three runtime invariants of a whole-query dispatch, from system.query_log: - - 1. the whole query -- JOIN and GROUP BY included -- reached a worker as a secondary query; - 2. exactly one table in it is a cluster function, i.e. only the driver was rewritten; - 3. no secondary query is a bare single-table cluster read, which is what a partner table - fanning out again from a worker would look like. +def _driver_announcement(qualified_name): + """Splits ``catalog.`ns.table` `` into the database/table pair the initiator announces, which is + what `DatabaseDataLake` builds the driver's StorageID from.""" + database, table = qualified_name.split(".", 1) + return database, table.strip("`") + + +def _assert_dispatched_whole(nodes, query_id, driver_database, driver_table): + """Checks the runtime invariants of a whole-query dispatch, from system.query_log: + + 1. the whole query -- JOIN and GROUP BY included -- reached a worker as a secondary query, carrying + the driver announcement the initiator chose; + 2. the driver crossed the wire as the plain catalog table the user wrote. Nothing is rewritten into a + `*Cluster` table function; an earlier design did that, and this pins that it is not coming back; + 3. every secondary query of this dispatch is that whole query. A partner table fanning out again from + a worker would show up as a further secondary query without the JOIN in it. """ for node in nodes: node.query("SYSTEM FLUSH LOGS system.query_log") - secondary_with_join = 0 + announced_whole_query = 0 for node in nodes: - secondary_with_join += int( + announced_whole_query += int( node.query( f""" SELECT count() FROM system.query_log WHERE type = 'QueryStart' AND NOT is_initial_query AND initial_query_id = '{query_id}' - AND positionCaseInsensitive(query, 'icebergs3cluster') != 0 AND positionCaseInsensitive(query, 'join') != 0 AND positionCaseInsensitive(query, 'group by') != 0 + AND Settings['object_storage_distributed_driver_database'] = '{driver_database}' + AND Settings['object_storage_distributed_driver_table'] = '{driver_table}' """ ).strip() ) - assert secondary_with_join > 0, f"query {query_id} was not dispatched whole to the cluster" + assert announced_whole_query > 0, ( + f"query {query_id} was not dispatched whole to the cluster, or reached a worker without the " + f"driver announcement naming {driver_database}.{driver_table}" + ) for node in nodes: - multi_driver = int( + rewritten = int( node.query( f""" SELECT count() FROM system.query_log WHERE type = 'QueryStart' AND NOT is_initial_query AND initial_query_id = '{query_id}' - AND countSubstringsCaseInsensitive(query, 'icebergs3cluster') > 1 + AND positionCaseInsensitive(query, 'icebergs3cluster') != 0 """ ).strip() ) - assert multi_driver == 0, ( - f"query {query_id}: more than one cluster function in a dispatched query on {node.name} -- " - "a partner table was rewritten as a driver" + assert rewritten == 0, ( + f"query {query_id}: a dispatched query on {node.name} contains a cluster table function -- " + "the driver must travel as the catalog table the user wrote, named in the settings" ) partner_fanout = int( @@ -1160,14 +1173,13 @@ def _assert_dispatched_whole(nodes, query_id): FROM system.query_log WHERE type = 'QueryStart' AND NOT is_initial_query AND initial_query_id = '{query_id}' - AND positionCaseInsensitive(query, 'icebergs3cluster') != 0 AND positionCaseInsensitive(query, 'join') = 0 """ ).strip() ) assert partner_fanout == 0, ( - f"query {query_id}: a single-table cluster read was issued on {node.name} -- " - "a partner table fanned out again instead of being read locally" + f"query {query_id}: a secondary query without the JOIN was issued on {node.name} -- " + "a table fanned out again instead of being read locally" ) @@ -1272,7 +1284,7 @@ def run(query, join_mode, query_id=None): query_id = uuid.uuid4().hex assert run(query, "distributed", query_id=query_id) == expected, f"{name} differs under dispatch" - _assert_dispatched_whole(nodes, query_id) + _assert_dispatched_whole(nodes, query_id, *_driver_announcement(fact)) def test_distributed_join_dispatch_falls_back(started_cluster): @@ -1317,11 +1329,10 @@ def test_distributed_join_dispatch_ignores_parallel_replicas_settings(started_cl owns that queue, so a partner answering yes as well would read the driver's files under its own schema. - Two guards currently prevent that, and this test exists to keep them: `tryGetTableImpl` only falls - back to the parallel-replicas cluster when `!is_secondary_query`, and a dispatched worker query - contains a `*Cluster` table function, which makes the context distributed. Remove either and a - partner becomes a queue consumer. The settings below are the combination that makes the rest of the - constructor's condition true. + What prevents that is the constructor asking, first, whether this is a dispatched worker at all: if it + is, only the announced driver consumes the queue and the parallel-replica settings get no say. Make + those two conditions independent again -- the earlier `||` -- and a partner becomes a queue consumer. + The settings below are the combination that makes the parallel-replica half true. """ node1 = started_cluster.instances["node1"] node2 = started_cluster.instances["node2"] @@ -1370,7 +1381,7 @@ def test_distributed_join_dispatch_ignores_parallel_replicas_settings(started_cl # Without this the test would silently stop covering anything if the candidate were rejected # whenever the parallel-replicas settings are set. - _assert_dispatched_whole(nodes, query_id) + _assert_dispatched_whole(nodes, query_id, *_driver_announcement(fact)) def test_used_storages_in_query_log(started_cluster): node1 = started_cluster.instances["node1"] From 288e3efb518220d8a7b99a050a784ee6e9f80784 Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Thu, 24 Sep 2026 17:01:21 +0530 Subject: [PATCH 15/15] Prune the driver's file listing by its own predicate The dispatch handed out every file of the driver. The step's own filter is no help -- its output is the whole query's result, so a predicate over that says nothing about which files are needed -- and nothing went looking for the driver's predicate elsewhere, so a query selecting one day out of sixty still distributed all sixty days' files and had each worker open its share only to discard them. The predicate is in the query tree, in the `WHERE` of the query nodes crossed on the way down to the driver. Collect those, split them into `and` atoms, keep the atoms whose every column comes from the driver, and build an `ActionsDAG` from them with `buildFilterInfo` -- the same helper row policies and additional table filters use. An atom that reads a partner's column, or contains a subquery, is left alone and simply does not prune. Safe because of two restrictions the candidate finder already enforces: the driver sits on the left spine of INNER ALL / LEFT joins only, so a driver row dropped here cannot have produced a result row; and every query node crossed to reach it is partition-preserving, so dropping a row cannot change what the surviving rows compute. Relaxing either would make this unsound, and the comment says so. Measured on a 600-file, 60-day-partitioned driver with a one-day window: 600 file tasks before, 10 after, with the result unchanged. A query with no driver predicate, and one whose only predicate mixes driver and partner columns, both still distribute all 600. Co-Authored-By: Claude Opus 5 (1M context) --- ...buildDistributedObjectStorageQueryPlan.cpp | 94 ++++++++++++++++++- .../findDistributedObjectStorageCandidate.cpp | 14 +++ .../findDistributedObjectStorageCandidate.h | 11 +++ src/Storages/IStorageCluster.cpp | 15 +-- src/Storages/IStorageCluster.h | 9 +- 5 files changed, 134 insertions(+), 9 deletions(-) diff --git a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp index adae4817d1d2..5b57e393c978 100644 --- a/src/Planner/buildDistributedObjectStorageQueryPlan.cpp +++ b/src/Planner/buildDistributedObjectStorageQueryPlan.cpp @@ -1,8 +1,12 @@ #include +#include +#include #include #include +#include #include +#include #include #include #include @@ -13,6 +17,7 @@ #include #include #include +#include #include namespace DB @@ -47,6 +52,83 @@ size_t countTableReferences(const ASTPtr & ast, const StorageID & storage_id) return count; } + +/// Splits a condition into its top-level `and` operands. Anything that is not an `and` is one atom. +void collectConjunctionAtoms(const QueryTreeNodePtr & node, QueryTreeNodes & atoms) +{ + if (const auto * function_node = node->as(); function_node && function_node->getFunctionName() == "and") + { + for (const auto & argument : function_node->getArguments().getNodes()) + collectConjunctionAtoms(argument, atoms); + return; + } + + atoms.push_back(node); +} + +/// True when every column this condition reads comes from `driver` and nothing in it has to be executed to be +/// understood. A subquery is rejected outright: this condition is evaluated while listing the driver's files, +/// long before there is a pipeline to run one in. +bool readsOnlyDriverColumns(const QueryTreeNodePtr & node, const TableNode * driver) +{ + if (node->as() || node->as()) + return false; + + if (const auto * column_node = node->as()) + return column_node->getColumnSource().get() == driver; + + for (const auto & child : node->getChildren()) + if (child && !readsOnlyDriverColumns(child, driver)) + return false; + + return true; +} + +/// The predicate over the driver's own columns, as an ActionsDAG the file listing can prune with. +/// +/// Correctness rests on two restrictions `findDistributedObjectStorageCandidate` already enforces, and would +/// break if either were relaxed: the driver sits on the left spine of INNER ALL / LEFT joins only, so a driver +/// row dropped here cannot have produced a result row; and every QueryNode crossed to reach it is +/// partition-preserving (`isSafeIntermediateSubquery` -- no GROUP BY, DISTINCT, LIMIT, window), so dropping a +/// row cannot change what the surviving rows compute. +/// +/// Returns nothing when no atom qualifies, which simply means every file is listed. +std::optional buildDriverOnlyFilter( + const DistributedObjectStorageCandidate & candidate, const PlannerContextPtr & planner_context) +{ + QueryTreeNodes atoms; + for (const auto * query_node : candidate.query_nodes_on_driver_path) + { + if (query_node->hasPrewhere()) + collectConjunctionAtoms(query_node->getPrewhere(), atoms); + if (query_node->hasWhere()) + collectConjunctionAtoms(query_node->getWhere(), atoms); + } + + QueryTreeNodes driver_atoms; + for (const auto & atom : atoms) + if (readsOnlyDriverColumns(atom, candidate.driver)) + driver_atoms.push_back(atom->clone()); + + if (driver_atoms.empty()) + return {}; + + const auto context = planner_context->getQueryContext(); + /// mergeConditionNodes always builds an `and`, which needs at least two arguments. + auto condition = driver_atoms.size() == 1 ? driver_atoms.front() : mergeConditionNodes(driver_atoms, context); + + /// Passed explicitly so buildFilterInfo does not go looking for this table expression in the planner + /// context: the dispatch boundary is planned as one unit and never registers the driver on its own. + const auto driver_columns = candidate.driver->getStorageSnapshot()->metadata->getColumns().getNamesOfPhysical(); + NameSet required_names(driver_columns.begin(), driver_columns.end()); + + auto mutable_planner_context = planner_context; + auto filter_info = buildFilterInfo( + std::move(condition), candidate.driver_table_expression, mutable_planner_context, std::move(required_names)); + + return std::move(filter_info.actions); +} + } /// This mirrors buildQueryPlanForParallelReplicas (Planner/findParallelReplicasQuery.cpp) step for step: @@ -103,6 +185,15 @@ std::optional buildDistributedObjectStorageQueryPlan( JoinTreeQueryPlan result; result.stage = processed_stage; + /// Prunes the driver's file listing. Without it every file of the driver is handed out and each worker + /// opens the ones its partitions cannot match only to discard them. + std::shared_ptr driver_filter; + if (auto filter_dag = buildDriverOnlyFilter(candidate, planner_context)) + { + VirtualColumnUtils::buildSetsForDAGExcludingGlobalIn(*filter_dag, context); + driver_filter = std::make_shared(std::move(*filter_dag)); + } + driver_storage->readPreparedClusterQuery( result.query_plan, driver_storage_snapshot, @@ -110,7 +201,8 @@ std::optional buildDistributedObjectStorageQueryPlan( dispatch_context, processed_stage, query_to_send, - remote_header); + remote_header, + std::move(driver_filter)); /// No converting step, unlike buildQueryPlanForParallelReplicas: that one rewrites the table expression /// before serializing, so its two headers are built from different trees and can diverge. Here the query diff --git a/src/Planner/findDistributedObjectStorageCandidate.cpp b/src/Planner/findDistributedObjectStorageCandidate.cpp index 470167d1b473..92a3c3606564 100644 --- a/src/Planner/findDistributedObjectStorageCandidate.cpp +++ b/src/Planner/findDistributedObjectStorageCandidate.cpp @@ -116,8 +116,12 @@ struct DriverPathResult { bool unusable = false; const TableNode * driver = nullptr; + QueryTreeNodePtr driver_table_expression; IStorageCluster * driver_storage = nullptr; + /// Innermost first; the caller reverses and prepends the dispatch boundary. + std::vector query_nodes_on_driver_path; + /// No JOIN on the path means there is nothing here for this mode to optimize; stock `IStorageCluster::read` /// already handles a plain single-table cluster read. bool has_join = false; @@ -143,6 +147,7 @@ DriverPathResult findDriverOnLeftSpine(const QueryTreeNodePtr & node, const Cont DriverPathResult result; result.driver = table_node; + result.driver_table_expression = node; result.driver_storage = storage; return result; } @@ -162,6 +167,7 @@ DriverPathResult findDriverOnLeftSpine(const QueryTreeNodePtr & node, const Cont if (!isSafeIntermediateSubquery(*query_node)) return unusableDriverPath(); + result.query_nodes_on_driver_path.push_back(query_node); return result; } @@ -245,7 +251,15 @@ std::optional findDistributedObjectStorageCan DistributedObjectStorageCandidate candidate; candidate.driver = driver_path.driver; + candidate.driver_table_expression = driver_path.driver_table_expression; candidate.driver_storage = driver_path.driver_storage; + + candidate.query_nodes_on_driver_path.push_back(query_node_typed); + candidate.query_nodes_on_driver_path.insert( + candidate.query_nodes_on_driver_path.end(), + driver_path.query_nodes_on_driver_path.rbegin(), + driver_path.query_nodes_on_driver_path.rend()); + return candidate; } diff --git a/src/Planner/findDistributedObjectStorageCandidate.h b/src/Planner/findDistributedObjectStorageCandidate.h index ee48ed7e4f1a..ab12d2b0ec1c 100644 --- a/src/Planner/findDistributedObjectStorageCandidate.h +++ b/src/Planner/findDistributedObjectStorageCandidate.h @@ -1,12 +1,14 @@ #pragma once #include #include +#include #include namespace DB { class TableNode; +class QueryNode; class IStorageCluster; class IQueryTreeNode; @@ -23,7 +25,16 @@ struct DistributedObjectStorageCandidate /// crossing. const TableNode * driver = nullptr; + /// The same node, kept as a pointer the planner can hand to APIs that take a table expression. + QueryTreeNodePtr driver_table_expression; + IStorageCluster * driver_storage = nullptr; + + /// Every QueryNode crossed on the way down to the driver, outermost first, including the dispatch + /// boundary itself. Their `WHERE`/`PREWHERE` are the only places a predicate over the driver's own + /// columns can appear such that a driver row failing it cannot reach the result -- which is what makes + /// it safe to prune the driver's files by it. See buildDistributedObjectStorageQueryPlan. + std::vector query_nodes_on_driver_path; }; /// Decides whether `query_node` as a whole can be executed on a single DataLake-catalog driver's cluster. diff --git a/src/Storages/IStorageCluster.cpp b/src/Storages/IStorageCluster.cpp index c743dd9fe518..436ef17967b4 100644 --- a/src/Storages/IStorageCluster.cpp +++ b/src/Storages/IStorageCluster.cpp @@ -653,7 +653,8 @@ void IStorageCluster::readPreparedClusterQuery( ContextPtr context, QueryProcessingStage::Enum processed_stage, ASTPtr query_to_send, - SharedHeader sample_block) + SharedHeader sample_block, + std::shared_ptr driver_filter) { auto cluster_name_from_settings = getClusterName(context); const auto & settings = context->getSettingsRef(); @@ -677,7 +678,8 @@ void IStorageCluster::readPreparedClusterQuery( processed_stage, cluster, log, - std::move(external_tables)); + std::move(external_tables), + std::move(driver_filter)); query_plan.addStep(std::move(reading)); } @@ -900,11 +902,12 @@ void ReadFromClusterQuery::initializePipeline(QueryPipelineBuilder & pipeline, c auto new_context = updateSettings(); prepareWholeQueryDispatchForRemoteExecution(query_to_send); - /// No predicate: this step's output is the whole query's result, so a filter over it says nothing about - /// which of the driver's files are needed. Recovering driver-only pruning means extracting the conjuncts - /// whose columns all come from the driver, which is not done yet -- every file of the driver is listed. + /// Not this step's own filter: its output is the whole query's result, so a predicate over that says + /// nothing about which of the driver's files are needed. The planner extracts the driver's own predicate + /// from the query tree instead and hands it over -- see buildDistributedObjectStorageQueryPlan. + const ActionsDAG::Node * predicate = driver_filter ? driver_filter->getOutputs().front() : nullptr; auto extension = driver_storage->getTaskIteratorExtension( - /*predicate=*/nullptr, /*filter=*/nullptr, new_context, cluster, driver_snapshot->metadata); + predicate, driver_filter.get(), new_context, cluster, driver_snapshot->metadata); auto pipe = buildClusterFunctionRemotePipe( query_to_send, getOutputHeader(), new_context, cluster, processed_stage, extension, external_tables, log); diff --git a/src/Storages/IStorageCluster.h b/src/Storages/IStorageCluster.h index 4b276bf121f6..878ec3704acc 100644 --- a/src/Storages/IStorageCluster.h +++ b/src/Storages/IStorageCluster.h @@ -66,7 +66,8 @@ class IStorageCluster : public IStorage ContextPtr context, QueryProcessingStage::Enum processed_stage, ASTPtr query_to_send, - SharedHeader sample_block); + SharedHeader sample_block, + std::shared_ptr driver_filter); bool isRemote() const final { return true; } bool supportsSubcolumns() const override { return true; } @@ -217,7 +218,8 @@ class ReadFromClusterQuery : public ISourceStep QueryProcessingStage::Enum processed_stage_, ClusterPtr cluster_, LoggerPtr log_, - std::optional external_tables_) + std::optional external_tables_, + std::shared_ptr driver_filter_) : ISourceStep(std::move(output_header_)) , driver_storage(std::move(driver_storage_)) , driver_snapshot(std::move(driver_snapshot_)) @@ -227,6 +229,7 @@ class ReadFromClusterQuery : public ISourceStep , cluster(std::move(cluster_)) , log(log_) , external_tables(std::move(external_tables_)) + , driver_filter(std::move(driver_filter_)) { } @@ -239,6 +242,8 @@ class ReadFromClusterQuery : public ISourceStep ClusterPtr cluster; LoggerPtr log; std::optional external_tables; + /// Predicate over the driver's own columns, used to prune its file listing. Null means list everything. + std::shared_ptr driver_filter; ContextPtr updateSettings() const; };