Repository navigation
feat: support multi-column NOT IN subqueries with null-aware joins - #19857
Merged
Merged
Conversation
viirya
changed the title
github-actions
Bot
added
sql
viirya
force-pushed
the
multi-column-null-aware-anti-join
branch
2 times, most recently
from
January 16, 2026 22:25
f61eabb to
f6db769
Compare
viirya
force-pushed
the
multi-column-null-aware-anti-join
branch
from
February 8, 2026 22:26
d07982f to
5662114
Compare
Contributor
|
Thanks @viirya I'll check it soon, afaik there is no TPC* like queries that cover multiple column null aware anti joins, so it would be prob nice to have a bench in future to make sure no performance regression introduced with future PRs |
Contributor
viirya
force-pushed
the
multi-column-null-aware-anti-join
branch
from
March 26, 2026 07:19
5662114 to
6613da5
Compare
Contributor
|
run benchmark tpcds |
|
🤖 Criterion benchmark running (GKE) | trigger File an issue against this benchmark runner |
|
Benchmark for this request failed. Last 20 lines of output: Click to expandFile an issue against this benchmark runner |
Contributor
|
run benchmark tpcds |
Contributor
|
Not sure if tpcds contains NA Anti Join, but it at least contains many join types |
|
🤖 Benchmark running (GKE) | trigger File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
viirya
force-pushed
the
multi-column-null-aware-anti-join
branch
from
April 8, 2026 06:08
9510be2 to
94265ba
Compare
neilconway
requested changes
Apr 17, 2026
Member
Author
|
I cross-validated all test cases (Test 1–25) against Postgres 16 running in a Docker container. I translated each .slt test into equivalent Postgres SQL and compared the output row-by-row against DataFusion's expected results. All 25 tests matched Postgres exactly, covering single-column NOT IN (Tests 1–18), multi-column NOT IN (Tests 19–23), correlated multi-column NOT IN (Test 24), and empty subquery (Test 25). |
viirya
force-pushed
the
multi-column-null-aware-anti-join
branch
from
May 3, 2026 07:18
1f579fd to
d672544
Compare
viirya
force-pushed
the
multi-column-null-aware-anti-join
branch
from
June 21, 2026 00:20
d672544 to
0b25fde
Compare
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Contributor
|
run benchmark tpch tpcds |
Member
Author
|
I will resolve the conflicts. |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing multi-column-null-aware-anti-join (295c056) to 696eaf5 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing multi-column-null-aware-anti-join (295c056) to 696eaf5 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
# Conflicts: # datafusion/optimizer/src/decorrelate_predicate_subquery.rs # datafusion/physical-plan/src/joins/hash_join/exec.rs # datafusion/sqllogictest/test_files/subquery.slt # docs/source/library-user-guide/upgrading/56.0.0.md
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing multi-column-null-aware-anti-join (295c056) to 696eaf5 (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 multi-column-null-aware-anti-join (295c056) to 696eaf5 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
Member
Author
|
@sunchao Can you take a look? Thanks! |
Contributor
|
@viirya checking this again |
comphead
reviewed
Oct 6, 2026
comphead
left a comment
comphead
left a comment
Contributor
There was a problem hiding this comment.
Thanks @viirya. I re-read the changes since 8eb53c4 and the two upstream merges. The -0.0, non-hashable key, all-literal tuple and per-chunk comparison fixes look right to me. I also checked the merge resolution against the new MarkJoin code in decorrelate_predicate_subquery.rs, and every expected row in null_aware_multi_column.slt against three-valued logic. I found nothing wrong there.
Verdict: the approach and the hash join side look good to me. I would hold the merge for the two inline points, the tuple nullability in Expr::nullable and the conflict with main in type_coercion.rs. The points below are smaller. All of them come from reading the code, not from running the branch.
Tests
- Nothing covers
IS [NOT] NULLover a tupleIN, and every tuple case uses nullable subquery columns. That is why the nullability issue inline onin_subquery_tuple_valuesstays hidden. subquery_output_exprhas an arm for a subquery whose top is not aProjection(DISTINCT,GROUP BY,ORDER BY ... LIMIT). Only a plainSELECT x, yis tested.- Tuple elements are only
INT,BIGINTandDOUBLE. AUtf8element, the most common shape, would also runtakeplusapply_cmpon a non-primitive array inretain_value_mismatch_free. - The mark join is only tested with an equality correlation. A
LeftMarktuple withi.k < o.kwould cover the join filter branch there. - No fuzz test covers null-aware joins. The
NOT EXISTSformulations in the Q10 to Q12 canaries are a ready oracle for a randomized comparison across batch sizes and NULL densities.
Docs
docs/source/user-guide/sql/subqueries.mdstill says non-scalar subqueries "can only return a single column" (twice) and documents tupleINonly in its list form. The list form uses struct equality, so per that page(1, NULL) IN ((1, NULL))istrue. Here(1, NULL) IN (SELECT 1, NULL)is now NULL under SQL row comparison. That difference deserves a sentence there.
Follow-ups
- I could only find the two follow-ups agreed in the earlier thread (NOT NULL elements as scope keys, and building on the subquery side) in the doc comment on
HashJoinExec::null_aware_value_keys. Could you file issues for them and link those from the comment? The outer side always being the build side and the quadratic NULL pairing are the user-visible costs. Q10 to Q12 numbers at twoNAJ_ROWSvalues would also make the follow-ups measurable.
Nit
- Placeholder inference builds the per-element column with
Column::new_unqualified(field.name()). That is ambiguous when two subquery columns share a name under different qualifiers (SELECT t1.id, t2.id ...).Column::from(subquery_schema.qualified_field(i))avoids it, assubquery_output_expralready does.
| /// | ||
| /// Errors when the number of tuple elements does not match the number of | ||
| /// subquery columns. | ||
| pub fn in_subquery_tuple_values<'a>( |
Contributor
There was a problem hiding this comment.
This helper decides when an InSubquery is a tuple, but Expr::nullable (expr_schema.rs, InSubquery arm) still assumes one value. It returns expr.nullable() | <first subquery column>.nullable(). For a tuple, expr is a struct(..) call, and struct always reports a non-nullable field (return_field_from_args). So the elements never count, and only the first subquery column does. (a, b) IN (SELECT 1, 5) is typed NOT NULL, although it is UNKNOWN for (NULL, 5).
simplify_expressions runs before decorrelation and folds IS NULL and IS UNKNOWN to false, and IS NOT NULL to true, from that flag. By my reading this gives a wrong result. I did not run it:
CREATE TABLE o(a INT, b INT) AS VALUES (NULL, 5), (1, 5);
SELECT a, ((a, b) IN (SELECT 1, 5)) IS NULL AS unk FROM o;
-- expected: (NULL, true) and (1, false)
-- by my reading: false for both rowsThe scalar form is exact because a.nullable | x.nullable covers both sides. For a tuple the arm could return true when any element or any subquery column is nullable. An SLT case with IS NULL over a tuple IN and a NOT NULL first subquery column would pin it.
Member
Author
There was a problem hiding this comment.
Confirmed: ((a, b) IN (SELECT 1, 5)) IS NULL folded to false for (NULL, 5). Fixed in ab3120c. The InSubquery arm of Expr::nullable now treats a tuple IN as nullable when any element or any subquery column is nullable. null_aware_multi_column.slt covers IS NULL and IS NOT NULL over a tuple IN whose subquery columns are both NOT NULL.
| /// Each tuple element and the subquery column at the same position are cast | ||
| /// to their common comparison type, like the single-column form does for its | ||
| /// one pair. | ||
| fn coerce_multi_column_in_subquery( |
Contributor
There was a problem hiding this comment.
This PR now conflicts with main in this file. #25094 changed the scalar branch of this arm to comparison_coercion_with_session_timezone(.., self.session_time_zone). This helper is a free function, so a resolution that only patches the scalar branch still compiles and leaves tuple elements on the timezone-unaware comparison_coercion. It needs the session time zone passed in as well. A tuple with a naive and a tz-aware timestamp element, with the session time zone set, would catch a miss.
Member
Author
There was a problem hiding this comment.
Merged main and passed the session time zone into coerce_multi_column_in_subquery, so the scalar branch and every tuple element both use comparison_coercion_with_session_timezone. The new case in null_aware_multi_column.slt compares a naive and a time-zone-aware timestamp element under SET TIME ZONE = '+08'. It fails if the tuple path drops the session time zone, because the naive value is then read in the aware value's own zone and matches.
Member
Author
|
Thanks @comphead. ab3120c addresses the review: Tests
Docs
Follow-ups
Nit
|
Contributor
|
Thanks @viirya |
Member
Author
|
Thank you @comphead |
Contributor
|
Won't get a chance to do a careful review right now, but the points I raised before have been addressed -- no objections to landing this from me! |
Member
Author
Thank you @neilconway ! Could you remove the requested changes request because seems it will block merging? Thanks! |
neilconway
approved these changes
Oct 6, 2026
neilconway
left a comment
neilconway
left a comment
Contributor
There was a problem hiding this comment.
I had some old pending comments, not sure if they still apply :) Feel free to ignore if so.
| let is_struct = matches!(*expr, Expr::ScalarFunction(ref func) if func.func.name() == "struct"); | ||
|
|
||
| if is_struct { | ||
| // For multi-column IN, we don't need type coercion at this level |
Contributor
There was a problem hiding this comment.
Can you elaborate on this? It isn't clear to me why the type coercion requirements for multi-column IN would be different than for a single-column IN.
Member
Author
There was a problem hiding this comment.
This one is out of date: the multi-column form now coerces each tuple element against its subquery column, the same way the single-column form coerces its one pair (coerce_multi_column_in_subquery, which also applies the session time zone).
| if subquery.subquery.subquery.schema().fields().len() > 1 { | ||
| // InSubquery should only return one column UNLESS the left expression is a struct | ||
| // (multi-column IN like: (a, b) NOT IN (SELECT x, y FROM ...)) | ||
| let is_struct = matches!(*subquery.expr, Expr::ScalarFunction(ref func) if func.func.name() == "struct"); |
Contributor
There was a problem hiding this comment.
Matching based on the function name here seems a bit fragile, which is unfortunate because we do it repeatedly in this PR. Is there a way to make the distinction more principled / manifest in the type system?
Member
Author
There was a problem hiding this comment.
Agreed. The name check now lives in one place, in_subquery_tuple_values, which every caller goes through (SQL planning, invariants, type coercion, nullability, placeholder inference and decorrelation). It also recognizes the struct literal that simplify_expressions folds an all-literal tuple into. Making the tuple explicit in the type system, for example as a dedicated field on InSubquery, would be cleaner but changes a public expression type.
comphead
approved these changes
Oct 6, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 7, 2026
Member
Author
|
Thanks all for reviewing this! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Follow-up to #10583, which made single-column
NOT INa null-aware anti join.Rationale for this change
Multi-column
IN/NOT INsubqueries fail to plan:Supporting them requires SQL three-valued logic over the whole tuple. A NULL in one element does not make every comparison UNKNOWN the way a scalar NULL does:
(NULL, 8) = (1, 2)isUNKNOWN AND FALSE, which is FALSE, so the outer row is kept.(NULL, 2) = (1, 2)is UNKNOWN, so it is dropped.What changes are included in this PR?
A null-aware join already reads its equi-join keys by position:
on[0]is theNOT INvalue key andon[1..]are the correlation scope keys of a correlated subquery. A tuple needs several value keys, so both the logicalJoinandHashJoinExecnow record how many leading keys are value keys (null_aware_value_keys). The layout becomeson[..V]for the tuple elements andon[V..]for the correlation keys.(a, b) IN (SELECT x, y ...)is accepted when the tuple and subquery column counts match. Type coercion and placeholder inference work element by element. A tuple compared with a single struct-typed column ((a, b) IN (SELECT s ...)) is still a scalar struct comparison.IN/NOT INinside a projection or anORgoes through a null-aware mark join.HashJoinExec:V > 1uses the existing per-build-row path of correlated null-aware joins. A row counts as NULL-valued when any value key is NULL. Candidate pairs are then kept only when their value tuples have no definite mismatch, i.e. every element pair is equal or involves a NULL. This works for bothLeftAntiandLeftMark, with or without scope keys and join filters.JoinNodeandHashJoinExecNodecarrynull_aware_value_keys. A missing field (0) decodes as1.What is the testing strategy for this PR?
null_aware_anti_join.sltcovers:NOT INwith NULLs on either side, an empty subquery, and correlated equality and non-equality correlationsNOT INas a nullable projected value and underORsubquery.sltcovers multi-columnIN.hash_join/exec.rsunit tests runLeftAntiandLeftMarkwith probe-side and build-side NULLs, three columns, an empty probe side, and correlated scope keys, across batch sizes. Further unit tests cover value-key validation and the protobuf round trip.shared_bounds.rstests the dynamic filter NULL escape over every value key.roundtrip_logical_plan.rsround-trips a correlated multi-columnNOT INplan.Are there any user-facing changes?
Multi-column
IN/NOT INsubqueries are now supported.API change:
JoinandHashJoinExechave a new publicnull_aware_value_keysfield, and the generated protobufJoinNodeandHashJoinExecNodehave a new field. Code that builds these with struct literals must set it (1keeps the previous behavior). The 56.0.0 upgrade guide describes the change.