-
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 6 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,83 @@ void checkAccessRightsForSubquery(const QueryTreeNodePtr & subquery_node, const | |
| } | ||
| } | ||
|
|
||
| /// Same outer-join sides as JOIN filter pushdown / `FunctionToSubcolumnsPass`: | ||
| /// do not copy a predicate onto the null-producing side. | ||
| bool joinTreePreservesRowsForTable(const QueryTreeNodePtr & join_tree, const QueryTreeNodePtr & table) | ||
| { | ||
| std::vector<QueryTreeNodePtr> stack = {join_tree}; | ||
| while (!stack.empty()) | ||
| { | ||
| auto node = std::move(stack.back()); | ||
| stack.pop_back(); | ||
| if (!node) | ||
| continue; | ||
|
|
||
| if (const auto * join = node->as<JoinNode>()) | ||
| { | ||
| if (isRightOrFull(join->getKind()) && extractTableExpressionsSet(join->getLeftTableExpression()).contains(table.get())) | ||
| return false; | ||
| if (isLeftOrFull(join->getKind()) && extractTableExpressionsSet(join->getRightTableExpression()).contains(table.get())) | ||
| return false; | ||
| stack.push_back(join->getLeftTableExpression()); | ||
| stack.push_back(join->getRightTableExpression()); | ||
| } | ||
| else if (const auto * array_join = node->as<ArrayJoinNode>()) | ||
| { | ||
| stack.push_back(array_join->getTableExpression()); | ||
| } | ||
| else if (const auto * cross_join = node->as<CrossJoinNode>()) | ||
| { | ||
| for (const auto & expr : cross_join->getTableExpressions()) | ||
| stack.push_back(expr); | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /// `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; | ||
|
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. nit: I prefer to check size before use
Author
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. Add an exception |
||
| 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 +998,49 @@ 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`. | ||
| /// Skip the null-producing side of an outer JOIN (`WHERE isNull(r.x)` on a `LEFT JOIN`). | ||
| if (const auto * parent_query = select_query_info.query_tree->as<QueryNode>(); | ||
| parent_query && joinTreePreservesRowsForTable(parent_query->getJoinTree(), original_table_expression)) | ||
|
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 new guard excludes null-producing outer-join sides, but it still admits join shapes where prefiltering changes which rows are joined. For example, when the remote right side of an Useful? React with 👍 / 👎. |
||
| { | ||
| 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 👍 / 👎. |
||
| removeExpressionsThatAreNotDeterministicInScopeOfQuery(cloned, 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.
This removes functions that are nondeterministic within a query, but stateful functions can still report themselves as deterministic; 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 +1610,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(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
What we should to do if function not
isOrdinaryFunction? Just ignoring?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.
Window and aggregate functions are checked above, so here can be only ordinary or unresolved functions.
But all functions must be resolved when this method is called.
I rewrite test to be more clean, without logic change.