Conversation
…ession test The regression test for the extracted-alias generator relied on an IN subquery in the SELECT list surviving until ExtractLeafExpressions runs. IN subqueries in projections are now decorrelated, so make the subquery correlated with a LIMIT, which DecorrelatePredicateSubquery cannot pull up. The generator still starts at 2 inside the subquery, so the test guards the same behaviour. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ections DecorrelatePredicateSubquery materialized every projected IN and NOT IN subquery with three mark joins. Two of them have no join predicate for an uncorrelated subquery and plan as nested-loop mark joins over outer x inner rows. A single LeftMark join already carries SQL three-valued logic when its filter is hashable-only: the join is null-aware when the keys may be NULL, and exact otherwise. Use that join alone in the hashable case and keep the three-join materialization only when a residual non-equality filter remains. Co-Authored-By: Christian McArthur <christian@energywell.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… subqueries Add plan guards for the two paths of a projected IN subquery: one null-aware LeftMark join per subquery when the join filter is hashable only, and the three-join materialization when a non-equality correlation stays as a residual join filter. Add result tests for IN, NOT IN, EXISTS, an aggregate probe, a CASE wrapper and a COALESCE wrapper over tables that hold NULL keys. The expected values agree with DuckDB 1.5.2 and PostgreSQL 17. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
9e94b59 to
04f3d7d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25338 +/- ##
=========================================
Coverage 81.92% 81.93%
=========================================
Files 1135 1136 +1
Lines 427772 428919 +1147
Branches 427772 428919 +1147
=========================================
+ Hits 350456 351420 +964
- Misses 56373 56463 +90
- Partials 20943 21036 +93 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@kosiew since you reviewed #24972 would you mind taking a look here? cc @neilconway since you were involved in #21363 |
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing in-exists-subquery-projection (04f3d7d) to 22651d2 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing in-exists-subquery-projection (04f3d7d) to 22651d2 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing in-exists-subquery-projection (04f3d7d) to 22651d2 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
There was a problem hiding this comment.
🟡 Changes recommended
Expression-level nullability can be missed, causing projected IN to return false instead of UNKNOWN.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Optimizes projected IN/NOT IN subqueries by using a single null-aware mark join when possible.
Changes:
- Tracks whether mark joins preserve three-valued logic.
- Retains three-join fallback for residual predicates.
- Adds plan and NULL-semantics regression tests.
File summaries
| File | Description |
|---|---|
datafusion/optimizer/src/decorrelate_predicate_subquery.rs |
Implements single-mark-join optimization. |
datafusion/sqllogictest/test_files/subquery_projection.slt |
Tests plans and NULL semantics. |
datafusion/sqllogictest/test_files/projection_pushdown.slt |
Updates alias-collision regression coverage. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return Ok(Some(new_plan)); | ||
| return Ok(Some(BuiltJoin { | ||
| plan: new_plan, | ||
| mark_is_three_valued_exact: mark_filter_is_hashable_only, |
There was a problem hiding this comment.
Good catch, confirmed. With id INT NOT NULL, NULLIF(id, 1) IN (SELECT ...) returned false and NULLIF(id, 1) NOT IN (SELECT ...) returned true where SQL gives NULL. DuckDB and PostgreSQL both give NULL.
Fixed in b44d847. join_keys_may_be_null now takes the equijoin key expressions from split_eq_and_noneq_join_predicate and asks ExprSchemable::nullable for each key against the schema of its own side. That covers NULLIF, TRY_CAST, a CASE with no ELSE, and a key that a cast wraps. The residual filter keeps the older column-based check, so no case is less conservative than before. For these keys the projection path stays one null-aware mark join, so the plan shape and the timings do not change.
The same helper feeds the LeftAnti path. On main, SELECT id FROM nn WHERE NULLIF(id, 1) NOT IN (SELECT id FROM r3) keeps the id = 1 row that it must drop. The same commit fixes that too.
Tests: two unit tests with a nullable key expression over a non-nullable column, one for IN in a projection and one for NOT IN in a filter, both plan null_aware. A new section in subquery_projection.slt runs the NULLIF left key, the TRY_CAST left key, a NULLIF right key, NOT IN, and the WHERE form, and pins the single-join EXPLAIN.
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing in-exists-subquery-projection (04f3d7d) to 22651d2 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing in-exists-subquery-projection (04f3d7d) to 22651d2 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing in-exists-subquery-projection (04f3d7d) to 22651d2 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 22651d2 (22651d2) to 22651d2 (merge-base) diff Run configurationrun benchmark clickbench_partitioned
changed:
ref: "22651d24cc8196f3206e09a36a437eda9e766b87"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing 22651d2 (22651d2) to 22651d2 (merge-base) diff Run configurationrun benchmark clickbench_partitioned
changed:
ref: "22651d24cc8196f3206e09a36a437eda9e766b87"CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing in-exists-subquery-projection (04f3d7d) to 22651d2 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
I opened #25346 to add benchmarks |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing in-exists-subquery-projection (04f3d7d) to 22651d2 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
`join_keys_may_be_null` decided whether a `LeftMark` or `LeftAnti` join must be null-aware from the nullability of the columns that the join filter refers to. A join key is an expression, not only a column, and an expression can be NULL although all of its columns are not nullable. Examples are `NULLIF(id, 1)`, `TRY_CAST(s AS INT)` and a `CASE` expression with no `ELSE` branch. With such a key the join was not null-aware, so the mark column was `false` where SQL asks for NULL. Over a table with a column `id INT NOT NULL` that holds 1, 2 and 4, and a subquery that gives 1 and 2: - `NULLIF(id, 1) IN (SELECT id FROM r3)` gave `false` for `id = 1`. The correct answer is UNKNOWN. - `TRY_CAST(s AS INT) IN (SELECT id FROM r3)` gave `false` where the text is not a number. The correct answer is UNKNOWN. The `NOT IN` filter path has the same gap, which is older than the projection path. `SELECT id FROM nn WHERE NULLIF(id, 1) NOT IN (SELECT id FROM r3)` returned the row for `id = 1`, which UNKNOWN must remove. The helper now takes the equijoin keys and the residual filter from `split_eq_and_noneq_join_predicate` and asks each key expression for its nullability against the schema of its own side. A cast around a key, such as the `CAST(id AS Int64)` that type coercion adds, keeps the nullability of the expression in it. For a residual filter there is no key expression, so the helper keeps the older column test there and is never less conservative than before. The `LeftMark` path already split the filter to find out whether it is hashable only. It now reuses that split instead of splitting twice. The projection path thus stays one null-aware mark join, and remains as fast as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. Reusing the exact hashable LeftMark join for projected IN / NOT IN is a nice improvement, and the added NULL-semantics coverage is helpful.
I found one blocking issue in the correlated NOT IN path. The expression-level nullability change can now make a LeftAnti join null-aware when there is more than one hash key, but physical planning only supports a single key for null-aware LeftAnti joins. I left a repro and suggested adding an execution regression test below.
I also left one non-blocking suggestion for the empty-subquery boundary on the new single-mark-join path.
| && join_keys_may_be_null(&join_filter, left.schema(), sub_query_alias.schema())?; | ||
| // Additionally, if no join key can be NULL on either side, we don't need | ||
| // null-aware semantics because NULLs cannot exist in the keys. | ||
| let null_aware = if join_type == JoinType::LeftAnti && in_predicate_opt.is_some() { |
There was a problem hiding this comment.
I think this introduces a planning failure for correlated NOT IN when the nullable expression key is combined with another correlated equality key.
For example, SELECT id FROM o WHERE NULLIF(id, 1) NOT IN (SELECT id FROM r WHERE r.grp = o.grp) with non-nullable o(id, grp) and r(id, grp) now makes the LeftAnti join null-aware because NULLIF(id, 1) is nullable. The correlation adds grp as a second hash key, so physical planning rejects the resulting join with null_aware LeftAnti joins only support single column join key, got 2 columns.
This looks newly reachable through the expression-level nullability change. Could we preserve the correct correlated NOT IN semantics using a supported fallback or plan shape here? It would also be good to add this query as an execution regression test and assert that only the SQL-true rows are returned.
There was a problem hiding this comment.
Thanks, confirmed. The query failed with got 2 columns. It plans again in fe408e3: a LeftAnti join with more than one key keeps the column test from main.
This does not give the correct result for your NULLIF example. The join has two keys and is not null-aware, so the k = 1 row stays, the same as on main. The test in subquery_projection.slt records this, with a comment that links apache/datafusion#25347. A correct plan needs a null-aware LeftAnti join with more than one key. apache/datafusion#25339 (approved) adds that. When it merges, I will remove this special case and update the expected output to the correct rows. I did not want to add a second fallback plan here, because apache/datafusion#25339 is the fix for this.
| 05)----ProjectionExec: expr=[CAST(id@0 AS Int64) as r3.id] | ||
| 06)------DataSourceExec: partitions=1, partition_sizes=[1] | ||
|
|
||
| # `NULLIF(id, 1)` is NULL for `id = 1`, and `r3` has no NULL, so the answer is |
There was a problem hiding this comment.
Could we also add an empty-subquery case for the new single null-aware mark path using a typed nullable key expression, for example NULLIF(id, 1)?
In particular, it would be useful to assert that the NULL-key row produces false, not NULL, when the subquery is empty, and that the plan still uses a single mark join. The existing top-level NULL IN (empty) test covers the SQL result semantics, but it takes the legacy three-join path, so it does not protect this boundary of the new optimization.
|
Thanks @adriangb , here is a suggestion: Correlated The For a correlated CREATE TABLE t1(k INT NOT NULL, s VARCHAR NOT NULL) AS VALUES (1, 'a');
CREATE TABLE t2(k INT NOT NULL, s VARCHAR NOT NULL) AS VALUES (1, 'B');
SELECT * FROM t1 WHERE upper(t1.s) NOT IN (SELECT t2.s FROM t2 WHERE t2.k = t1.k);
-- main: plans a plain LeftAnti and returns (1, 'a')
-- this PR: expected "null_aware LeftAnti joins only support single column join key"Nullable columns already fail like this on main (#25347), but this change extends the failure to common function keys over non-nullable data. It also pins uncorrelated Suggest limiting the expression-level check on the let null_aware = if join_type == JoinType::LeftAnti && in_predicate_opt.is_some() {
let (equijoin_keys, residual_filter) = split_eq_and_noneq_join_predicate(
join_filter.clone(),
left.schema(),
sub_query_alias.schema(),
)?;
- join_keys_may_be_null(
- &equijoin_keys,
- residual_filter.as_ref(),
- left.schema(),
- sub_query_alias.schema(),
- )?
+ if equijoin_keys.len() == 1 {
+ join_keys_may_be_null(
+ &equijoin_keys,
+ residual_filter.as_ref(),
+ left.schema(),
+ sub_query_alias.schema(),
+ )?
+ } else {
+ // Null-aware LeftAnti supports a single key only (#25347); keep the
+ // previous column-based test so correlated NOT IN still plans.
+ join_filter_columns_may_be_null(&join_filter, left.schema(), sub_query_alias.schema())?
+ }
} else {Please also add an slt case with the correlated query above. |
The previous commit made a `LeftAnti` join for `NOT IN` null-aware when a key
expression can be NULL. Most scalar functions report a nullable result, so a
key such as `upper(s)` over a `NOT NULL` column made the join null-aware. A
correlated `NOT IN` has two keys (the value and the correlation), and a
null-aware `LeftAnti` hash join supports one key only. Thus this query failed
to plan:
SELECT * FROM t1 WHERE upper(t1.s) NOT IN (SELECT t2.s FROM t2 WHERE t2.k = t1.k);
Error during planning: null_aware LeftAnti joins only support single column join key, got 2 columns
For a `LeftAnti` join with more than one key, use the column test on the whole
join filter again, as on `main`. The expression test stays for one key and for
all mark joins.
Also add tests for an empty subquery with a `NULLIF` key on the single mark
join path, and for the correlated `NOT IN` above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks @jayzhan211, good catch. I applied your suggestion in fe408e3. |
|
@jayzhan211 @kosiew could we merge the benchmarks in #25346 before this change so we can look at perf numbers? |
Which issue does this PR close?
This PR supersedes #21363 by @crm26. It reuses the single mark join approach from that PR, and @crm26 is a co-author of the main commit.
It also builds on #24972, which added the decorrelation of
INsubqueries in a projection.Rationale for this change
An
INorNOT INsubquery in a SELECT list gives correct results today, but the plan is quadratic. The optimizer builds three mark joins for each subquery. Two of them have no join predicate, so they run as nested loop joins over all outer rows and all inner rows.COALESCEduplicates the expression, so that shape gets six mark joins.This PR keeps one hash mark join for each subquery when the join filter is hashable. The three join fallback stays only for a non-equality correlation (Q6 below), which is not changed by this PR and is tracked separately in #25336.
INin the SELECT listCOALESCE((x IN (...))::boolean, false)INwith an equality predicateINsubqueries in separate columnsEXISTSINwith a non-equality predicate (fallback path, unchanged)PR to add these as benchmarks: #25346
Reproduction with datafusion-cli: script, plans and timings for each shape
Run the script with
datafusion-cli -f mre.sql. It creates two tables with 200000 rows each, then for each shape it printsEXPLAINand runs the query.datafusion-cliprints the elapsed time after each statement. The numbers above come from one run of this script on an Apple Silicon laptop,mainat 22651d2 and this PR built withcargo build -p datafusion-cli --profile ci. The result rows of every query are identical between the two builds.Q1: Bare `IN` in the SELECT list, plans on main and on this PR
main, query time 190.730 s
this PR, query time 0.034 s
Q2: `COALESCE((x IN (...))::boolean, false)`, plans on main and on this PR
main, query time 351.579 s
this PR, query time 0.054 s
Q3: Correlated `IN` with an equality predicate, plans on main and on this PR
main, query time 1.372 s
this PR, query time 0.091 s
Q4: Two `IN` subqueries in separate columns, plans on main and on this PR
main, query time 296.676 s
this PR, query time 0.063 s
Q5: Correlated `EXISTS`, plans on main and on this PR
main, query time 0.021 s
this PR, query time 0.021 s
Q6: Correlated `IN` with a non-equality predicate (fallback path, unchanged), plans on main and on this PR
main, query time 307.565 s
this PR, query time 261.582 s
What changes are included in this PR?
build_joinnow reports whether the mark column of theLeftMarkjoin is exact under three-valued logic. It is exact when the join filter is hashable only. The join is null aware when the keys can be NULL.in_subquery_value_mark_joinindatafusion/optimizer/src/decorrelate_predicate_subquery.rsnow builds thematchedjoin first. If that join is exact, it returns the mark column alone, orNOT markforNOT IN. The three join materialization is kept only when a residual non-equality filter remains, because the hash join cannot mark UNKNOWN for a residual predicate.The regression test for fix: scan subqueries when advancing the extracted-alias generator #24574 in
projection_pushdown.sltnow uses a correlated subquery withLIMIT 1. The subquery then still reachesExtractLeafExpressions, and the alias generator still starts at 2 inside it. The earlier version of the test let the subquery be flattened, so it no longer exercised the scan inside a subquery path.New sqllogictest cases in
subquery_projection.slt:EXPLAINguard that shows one hash mark join for each subquery,EXPLAINguard that shows the three join fallback when a residual filter remains,INandNOT INwith a NULL in the subquery, a NULL outer value,NOTinsideCASE, a projection over an aggregate, theCOALESCEand cast shape, and results for the residual filter shape.All of these were checked against DuckDB and PostgreSQL.
join_keys_may_be_nullnow decides null-awareness from the equijoin key expressions, not from the columns they reference. A key such asNULLIF(id, 1)orTRY_CAST(s AS INT)can beNULLover aNOT NULLcolumn. Before this change such a key gave a plain mark join, so the projectedINreturnedfalseinstead ofNULL. The same helper feeds theLeftAntipath, soWHERE NULLIF(id, 1) NOT IN (SELECT ...)onmainkept a row that it must drop. Both paths now plan a null-aware join for these keys. The residual filter keeps the older column-based check, so no case is less conservative than before.What is the testing strategy for this PR?
datafusion/optimizer/src/decorrelate_predicate_subquery.rs. The updated snapshots now show a single mark join. There are new tests for the residual filter fallback and forNOT IN.subquery_projection.sltlisted above, checked against DuckDB and PostgreSQL.subquery_projection.sltcases pass without any change.subquery_projection.sltsection cover nullable key expressions over non-nullable columns: aNULLIFleft key, aTRY_CASTleft key, aNULLIFright key,NOT IN, and theWHEREform. The expected values agree with DuckDB and PostgreSQL.Are there any user-facing changes?
There are no changes to query results and no changes to any public API. Plans for projected
INandNOT INsubqueries are much faster. No documentation change is needed.🤖 Generated with Claude Code