-
Notifications
You must be signed in to change notification settings - Fork 19
Fix join filter pushdown #2249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix join filter pushdown #2249
Changes from 3 commits
6f91aa6
af87b90
4e1b754
a54a78a
dc2f772
78a27b3
28b3de8
0e14162
707dcdc
2018169
a1d4502
cdc4b28
d9ebb0b
cf210cb
18a667b
523f4a0
f9203a7
f459b98
a5e8776
cc31f79
6a44388
9925af7
59af131
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,7 @@ | |
| #include <Processors/QueryPlan/ReadFromTableStep.h> | ||
| #include <Processors/QueryPlan/ReadFromTableFunctionStep.h> | ||
| #include <Processors/QueryPlan/ReadNothingStep.h> | ||
| #include <Processors/QueryPlan/SourceStepWithFilter.h> | ||
| #include <Processors/QueryPlan/Optimizations/Utils.h> | ||
| #include <Processors/QueryPlan/ParallelReplicasLocalPlan.h> | ||
| #include <Processors/Sources/SourceFromSingleChunk.h> | ||
|
|
@@ -215,6 +216,49 @@ void checkAccessRightsForSubquery(const QueryTreeNodePtr & subquery_node, const | |
| } | ||
| } | ||
|
|
||
| /// `IStorageCluster` JOINs wrap the left table in a subquery. Attach dummy-analysis | ||
| /// filters to the wrap source for listing only; do not add a FilterStep, which would | ||
| /// drop unused columns from the wrap header. | ||
| void tryAddClusterWrapFilter(QueryPlan & query_plan, const TableExpressionData & table_expression_data) | ||
| { | ||
| const auto & filter_actions = table_expression_data.getFilterActions(); | ||
| if (!filter_actions || !query_plan.isInitialized()) | ||
| return; | ||
|
|
||
| QueryPlan::Node * node = query_plan.getRootNode(); | ||
| while (node && !node->children.empty()) | ||
| node = node->children.front(); | ||
|
|
||
| auto * source = node ? dynamic_cast<SourceStepWithFilter *>(node->step.get()) : nullptr; | ||
| if (!source) | ||
| return; | ||
|
|
||
| auto filter_dag = filter_actions->clone(); | ||
| const auto filter_column_name = filter_dag.getOutputs().at(0)->result_name; | ||
| const auto & header = source->getOutputHeader(); | ||
| ActionsDAG rename_dag(header->getColumnsWithTypeAndName()); | ||
| const auto & identifier_to_name = table_expression_data.getColumnIdentifierToColumnName(); | ||
|
|
||
| for (const auto * input : filter_dag.getInputs()) | ||
| { | ||
| if (header->has(input->result_name)) | ||
| continue; | ||
|
|
||
| auto it = identifier_to_name.find(input->result_name); | ||
| if (it == identifier_to_name.end() || !header->has(it->second)) | ||
| continue; | ||
|
|
||
| const auto & physical = rename_dag.findInOutputs(it->second); | ||
| rename_dag.addOrReplaceInOutputs(rename_dag.addAlias(physical, input->result_name)); | ||
| } | ||
|
|
||
| filter_dag = ActionsDAG::merge(std::move(rename_dag), std::move(filter_dag)); | ||
| source->addFilter(std::move(filter_dag), filter_column_name); | ||
| /// Wrap subquery planning already called `applyFilters` with no predicate. | ||
| /// Apply now so icebergCluster listing is recreated with the WHERE. | ||
| source->SourceStepWithFilterBase::applyFilters(); | ||
| } | ||
|
|
||
| bool shouldIgnoreQuotaAndLimits(const TableNode & table_node) | ||
| { | ||
| const auto & storage_id = table_node.getStorageID(); | ||
|
|
@@ -920,8 +964,46 @@ JoinTreeQueryPlan buildQueryPlanForTableExpression(QueryTreeNodePtr table_expres | |
|
|
||
| if (wrap_read_columns_in_subquery) | ||
| { | ||
| auto original_table_expression = table_expression; | ||
|
|
||
| /// Subqueries inherit the outer GlobalPlannerContext, whose filter map is keyed by | ||
| /// outer table nodes. Collect filters for this JOIN query so icebergCluster listing | ||
| /// still sees left-only WHERE after the wrap. | ||
| if (!table_expression_data.getFilterActions() && select_query_info.query_tree) | ||
| { | ||
| auto collected = collectFiltersForAnalysis(select_query_info.query_tree, select_query_options, nullptr); | ||
| auto it = collected.find(table_expression); | ||
| if (it != collected.end() && it->second.filter_actions) | ||
| table_expression_data.setFilterActions(it->second.filter_actions->clone()); | ||
| } | ||
|
|
||
| auto columns = table_expression_data.getColumns(); | ||
| table_expression = buildSubqueryToReadColumnsFromTableExpression(columns, table_expression, query_context); | ||
| table_expression = buildSubqueryToReadColumnsFromTableExpression(columns, original_table_expression, query_context); | ||
|
|
||
| /// Wrap is planned as `SELECT cols FROM icebergCluster` with no JOIN. Copy left-only | ||
| /// WHERE/PREWHERE so initiator file listing sees the same predicate as a single-table | ||
| /// `icebergCluster` read. Same helper as `IStorageCluster::updateQueryWithJoinToSendIfNeeded`. | ||
| if (const auto * parent_query = select_query_info.query_tree->as<QueryNode>()) | ||
| { | ||
| auto copy_left_only = [&](const QueryTreeNodePtr & predicate) -> QueryTreeNodePtr | ||
| { | ||
| auto cloned = predicate->clone(); | ||
| removeExpressionsThatDoNotDependOnTableIdentifiers(cloned, original_table_expression, query_context); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an eligible Useful? React with 👍 / 👎. |
||
| return cloned; | ||
| }; | ||
|
|
||
| auto & wrap_query = table_expression->as<QueryNode &>(); | ||
| if (parent_query->hasWhere()) | ||
| { | ||
| if (auto pred = copy_left_only(parent_query->getWhere())) | ||
| wrap_query.getWhere() = std::move(pred); | ||
|
Comment on lines
+1047
to
+1050
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a wrapped remote table is on the null-producing side of an outer join, copying every table-local predicate into its subquery changes join semantics. For example, with a remote right side of a Useful? React with 👍 / 👎.
Comment on lines
+1047
to
+1050
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The copied predicate is added to the wrapper while the original remains above the join, and Useful? React with 👍 / 👎. |
||
| } | ||
| if (parent_query->hasPrewhere()) | ||
| { | ||
| if (auto pred = copy_left_only(parent_query->getPrewhere())) | ||
| wrap_query.getPrewhere() = std::move(pred); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| auto * table_node = table_expression->as<TableNode>(); | ||
|
|
@@ -1491,19 +1573,24 @@ JoinTreeQueryPlan buildQueryPlanForTableExpression(QueryTreeNodePtr table_expres | |
| else | ||
| { | ||
| std::shared_ptr<GlobalPlannerContext> subquery_planner_context; | ||
| auto subquery_options = select_query_options.subquery(); | ||
| if (wrap_read_columns_in_subquery) | ||
| subquery_planner_context = std::make_shared<GlobalPlannerContext>(nullptr, nullptr, nullptr, FiltersForTableExpressionMap{}); | ||
| { | ||
| subquery_planner_context = std::make_shared<GlobalPlannerContext>( | ||
| nullptr, nullptr, nullptr, collectFiltersForAnalysis(table_expression, subquery_options, nullptr)); | ||
| } | ||
| else | ||
| subquery_planner_context = planner_context->getGlobalPlannerContext(); | ||
|
|
||
| auto subquery_options = select_query_options.subquery(); | ||
| Planner subquery_planner(table_expression, subquery_options, subquery_planner_context); | ||
| /// Propagate storage limits to subquery | ||
| subquery_planner.addStorageLimits(*select_query_info.storage_limits); | ||
| subquery_planner.buildQueryPlanIfNeeded(); | ||
| const auto & mapping = subquery_planner.getQueryNodeToPlanStepMapping(); | ||
| query_node_to_plan_step_mapping.insert(mapping.begin(), mapping.end()); | ||
| query_plan = std::move(subquery_planner).extractQueryPlan(); | ||
| if (wrap_read_columns_in_subquery && till_stage == QueryProcessingStage::FetchColumns) | ||
| tryAddClusterWrapFilter(query_plan, table_expression_data); | ||
| } | ||
|
|
||
| auto & alias_column_expressions = table_expression_data.getAliasColumnExpressions(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,7 +98,9 @@ void ReadFromCluster::applyFilters(ActionDAGNodes added_filter_nodes) | |
|
|
||
| void ReadFromCluster::createExtension(const ActionsDAG::Node * predicate) | ||
| { | ||
| if (extension) | ||
| /// Listing is one-shot. Recreate only when a real predicate arrives after an | ||
| /// empty listing (e.g. `initializePipeline` ran before `applyFilters`). | ||
| if (extension && !(predicate && !extension_has_predicate)) | ||
| return; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the new wrapped- Useful? React with 👍 / 👎. |
||
|
|
||
| extension = storage->getTaskIteratorExtension( | ||
|
|
@@ -107,6 +109,7 @@ void ReadFromCluster::createExtension(const ActionsDAG::Node * predicate) | |
| context, | ||
| cluster, | ||
| getStorageSnapshot()->metadata); | ||
| extension_has_predicate = predicate != nullptr; | ||
| } | ||
|
|
||
| namespace | ||
|
|
@@ -596,7 +599,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(nullptr); | ||
| const ActionsDAG * filter = filter_actions_dag ? filter_actions_dag.get() : query_info.filter_actions_dag.get(); | ||
| const ActionsDAG::Node * predicate = filter ? filter->getOutputs().at(0) : nullptr; | ||
| createExtension(predicate); | ||
|
|
||
| ProfileEvents::increment(ProfileEvents::Shards, max_replicas_to_use); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I prefer to check size before use
atif we receive vector outside.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an exception