Fix join filter pushdown - #2249
Conversation
…in header Unused-column removal and `JoinStepLogical` aliases can hide a one-sided `WHERE` from `get_available_columns_for_filter`. Include those names so existing split and remap can push the predicate under the JOIN. Co-authored-by: Cursor <cursoragent@cursor.com>
…r can prune files Initiator listing runs on the wrap subquery (`SELECT cols FROM icebergCluster`), which previously had no WHERE. A left-only predicate on `count()` of `SELECT * … JOIN` never reached min/max file listing. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the duplicated WHERE walker and the PK-walk-through-JOIN remapping. Wrap listing still uses collectFiltersForAnalysis and tryAddClusterWrapFilter. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e1b754451
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (parent_query->hasWhere()) | ||
| { | ||
| if (auto pred = copy_left_only(parent_query->getWhere())) | ||
| wrap_query.getWhere() = std::move(pred); |
There was a problem hiding this comment.
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 LEFT JOIN, WHERE isNull(r.value) is copied below the join; rows matching a non-null r.value are then removed before the join, become null-extended unmatched rows, and incorrectly pass the original outer predicate. Check parent_join_tree and the join kind/side before copying a predicate, rather than treating every wrapped table expression as safe.
Useful? React with 👍 / 👎.
| if (parent_query->hasWhere()) | ||
| { | ||
| if (auto pred = copy_left_only(parent_query->getWhere())) | ||
| wrap_query.getWhere() = std::move(pred); |
There was a problem hiding this comment.
The copied predicate is added to the wrapper while the original remains above the join, and removeExpressionsThatDoNotDependOnTableIdentifiers does not reject nondeterministic expressions. Thus an IStorageCluster join with a left-only condition such as WHERE rand() % 2 = 0 evaluates independent rand calls in the wrapper and again after the join, changing the expected cardinality from roughly one half to one quarter. Listing predicates must not become an additional execution filter unless they are proven safe to duplicate.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,86 @@ | |||
| -- Tags: no-parallel-replicas | |||
There was a problem hiding this comment.
The test explicitly executes SET enable_parallel_replicas = 0, so the no-parallel-replicas tag is not needed to establish its required execution mode. Remove the tag rather than excluding the test from that runner, as repository guidance prohibits adding no-* tags unless they are strictly necessary.
AGENTS.md reference: AGENTS.md:L143-L143
Useful? React with 👍 / 👎.
Does it also work for non count cases? |
…JOIN Copying a table-local WHERE such as `isNull(r.x)` under a LEFT JOIN remote right table changes join semantics. Use the same `isLeftOrFull` / `isRightOrFull` sides as JOIN filter pushdown. Related: #2249 Co-authored-by: Cursor <cursoragent@cursor.com>
Share the existing `and`-conjunct filter in Analyzer/Utils so wrap copy can drop `rand` and similar after the table-local strip. Those conjuncts stay only in the original WHERE above the JOIN. Related: #2249 Co-authored-by: Cursor <cursoragent@cursor.com>
…wn test `SET enable_parallel_replicas = 0` already pins the EXPLAIN plan, so skipping the ParallelReplicas suite is not needed. Co-authored-by: Cursor <cursoragent@cursor.com>
Issue (and fix) also cover cases with any aggregation functions that do not use columns from 'where' condition. I have column Combination of |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78a27b3767
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
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 INNER ASOF JOIN has a right-only WHERE, copying it into the wrapper can discard the nearest match and select an older matching row instead; similarly, prefiltering either side of a PASTE JOIN shifts positional alignment. The normal pushdown path in filterPushDown.cpp explicitly disables right-side ASOF pushdown and all PASTE pushdown, so this wrapper path needs equivalent restrictions before copying the predicate.
Useful? React with 👍 / 👎.
| removeExpressionsThatDoNotDependOnTableIdentifiers(cloned, original_table_expression, query_context); | ||
| removeExpressionsThatAreNotDeterministicInScopeOfQuery(cloned, query_context); |
There was a problem hiding this comment.
This removes functions that are nondeterministic within a query, but stateful functions can still report themselves as deterministic; aiEmbed, for example, returns true from isDeterministicInScopeOfQuery while documenting that each call consumes quota and performs a potentially expensive external request. In an IStorageCluster join, such a WHERE is consequently evaluated in the wrapper and again above the join, doubling external calls and potentially exceeding ai_function_max_api_calls_per_query. The ordinary filter-pushdown path avoids this through ActionsDAG::hasStatefulFunctions, so copied predicates should apply the same exclusion.
Useful? React with 👍 / 👎.
Prefiltering those sides changes nearest-match and positional pairing. Skip the same sides for wrap listing so icebergCluster cannot drop the matching file either. Co-authored-by: Cursor <cursoragent@cursor.com>
Functions such as `aiEmbed` and `timeSeriesStoreTags` can be deterministic in a query while still having side effects. Skip them in the cluster wrap the same way JOIN filter pushdown uses `hasStatefulFunctions`. Co-authored-by: Cursor <cursoragent@cursor.com>
Wrap copy restated the same outer/`ASOF`/`PASTE`/`FULL` checks as filter pushdown, and stripped wrap-unsafe conjuncts in two walks. Co-authored-by: Cursor <cursoragent@cursor.com>
…andom settings `EXPLAIN` `Prewhere` is absent when parallel replicas rewrite the plan or when `optimize_move_to_prewhere` / `query_plan_optimize_prewhere` are randomized off. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20181690d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
When an eligible IStorageCluster table participates in an inner join and the outer predicate is a bare column from the other side, such as WHERE r.flag, this call leaves the predicate unchanged: filterConjunctions returns immediately when its root is not a FunctionNode, so removeExpressionsThatDoNotDependOnTableIdentifiers never invokes its dependency check. The predicate is then attached to the cluster-only wrapper despite referencing r, which is absent from that wrapper's FROM clause, causing a valid join query to fail during planning; apply the keep predicate to non-function roots as well.
Useful? React with 👍 / 👎.
`filterConjunctions` skipped a ColumnNode root, so `WHERE r.flag` was copied onto the `IStorageCluster` wrap and planning failed. Related: #2249 Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1d4502159
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (extension && !(predicate && !extension_has_predicate)) | ||
| return; |
There was a problem hiding this comment.
In the new wrapped-IStorageCluster path, tryAddClusterWrapFilter calls applyFilters while constructing the branch, before the overall query-plan optimizer can push additional predicates from enclosing subqueries down to the same source. If that early call creates an extension with predicate a, a later applyFilters carrying a AND b reaches this guard with both extension and extension_has_predicate set and is ignored, so object-storage listing never benefits from b (for example, an outer WHERE around the wrapped JOIN). Track predicate/filter changes or recreate the extension on subsequent filter applications rather than only for the null-to-non-null transition.
Useful? React with 👍 / 👎.
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix join filter pushdown
Documentation entry for user-facing changes
Solved #2245
Push left-only JOIN filters when column names do not match the join header
A left-only
WHEREoncount()ofSELECT * … JOINwas not pushed under the JOIN. The filtered column often disappeared from the JOIN output (unused-column removal aftercount()ofSELECT *), while the Filter DAG still referenced it — sometimes under an identifier name such as__table1.arather thana.get_available_columns_for_filterrequired the name to appear in the JOIN header, sosplitActionsForJOINFilterPushDownnever saw it.That is not Iceberg-specific. On MergeTree the same shape skipped
Prewhere/ index analysis on the left read. The filter still ran after the JOIN, so the result was correct but the left table was scanned without the predicate.JoinStepLogicalcan also alias a side input (a) to a JOIN-output / filter name (__table1.a). Pushdown matches filter inputs against the available-column list, thenfix_predicate_for_join_logical_stepremaps aliases back to input names. The output alias has to be listed or the split never runs.JOIN filter pushdown
In
tryPushDownOverJoinStep:JoinStepLogical, output actions thatfromLeft()/fromRight()are added to that list (including aliases). The existing split and remap then push the predicate under the JOIN.Covered by
04673_join_filter_pushdown_count_subquery.sql: MergeTree left + Memory right,count()ofSELECT * … LEFT JOIN … WHERE foo.a < 40(and the same with an extra(SELECT * FROM t_left) AS foowrap).EXPLAIN actions = 1must containPrewhere.icebergClusterfile listingicebergCluster(IStorageCluster) lists files on the initiator. The planner wraps the left cluster table so remotes do not get the JOIN (SELECT cols FROM icebergCluster). That wrap had noWHERE, so initiator listing stayed unfiltered even after JOIN pushdown.Left-only
WHERE/PREWHEREis copied onto the wrap withremoveExpressionsThatDoNotDependOnTableIdentifiers(same helper asIStorageCluster::updateQueryWithJoinToSendIfNeeded). Wrap planning runscollectFiltersForAnalysis. Listing-only filters are attached on the wrap source without adding aFilterStepthat would drop unused wrap columns.Covered by
test_cluster_join_filter_minmax_pruning.py:IcebergMinMaxIndexPrunedFilesforicebergS3Clusterwith a plainWHERE, withJOIN … WHERE, and with outercount()ofSELECT * … JOIN … WHERE.CI/CD Options
Exclude tests:
Regression jobs to run: